Forum OpenACS Development: ad_conn

Posted by Gustaf Neumann on
Hi there,

since the setup cost in the request monitor is quite high, i wonderered about the speed difference, when things are done with xotcl. i did two things: first i implemted an ad_cookie and an ad_signature object such one can do e.g.

ad_cookie get_signed_with_expr "ad_session_id"
instead of
ad_get_signed_cookie_with_expr "ad_session_id"
The xotcl verison turns out to be 20-30% faster than what's currently in oacs 5.2

One of the most frequently commands is ad_conn. I rewrote ad_conn in xotcl, the result is nearly 10 times faster than the current version (ad_conn1 is the xotcl version)

25.0 mms, ad_conn user_id
2.8 mms, ad_conn1 user_id
25.7 mms, ad_conn locale
2.6 mms, ad_conn1 locale
31.7 mms, ad_conn outputheaders
3.6 mms, ad_conn1 outputheaders
Since I wrote this as an extra package that overwrites the existing ad_conn, it is quite easy to try out. The xotcl-version is supposed to be fully compatible, and as far i can tell it works nicely with oacs/dotlrn. after some more tesging, i will make it then available for the brave.

I spent quite a while to make it working for packages that directoy access the global variable ad_conn(). This should not be necessary and should be removed from future versions of oacs.

During testing i have noticed that the following variables / subcommands from ad_conn are initialized, but nowhere set to actual values and nowhere read. Is this some legacy code, which can be safely removed?

{sec_validated ""}
{browser_id ""}
{token ""}
{last_issue ""}
{deferred_dml ""}
{system_p 0}

The new ad_conn is conceptually simpler than the old one and has the advantage that it can be subclassed as well. So, this experiment was quite promising. It would be quite interesting to tackle the list widget with xotcl, since the list widget is quite slow...

2: Re: ad_conn (response to 1)
Posted by Don Baccus on
I know nothing about the internals of the XoTcl implementation, so I'm just making a guess ... I imagine the speedup is due to the fact that Tcl arrays aren't particularly efficient. Handy but not all that fast as they're implemented as a list of name-value pairs.

You might also want to test the speed of "ad_conn -set" in the two implementations, and then the speed of rp_filter which uses "ad_conn -set" heavily ...

Of the unused ad_conn values that are set up that you mention ... AFAIK they can be removed.

4: Re: Re: ad_conn (response to 2)
Posted by Andrew Piskorski on
Don, say what? Tcl's associative arrays are implemented via hash tables last time I looked. Unless I'm mistaken, no lists of name/value pairs are involved, except when you explicitly call commands which generate a list from the array.
3: Re: ad_conn (response to 1)
Posted by Gustaf Neumann on
the differnce is that most of the work in xotcl is implemented in C rather in Tcl (e.g. switch handling, forwarding). I defined an object ad_conn using the following approach: xotcl has a short-form for
someobject set FLAG VALUE
someobject set FLAG
such one can write
ad_conn FLAG VALUE
ad_conn FLAG
for setting and retrieving values stored as instance variables.

Currently "ad_conn -set" is about the same speed as the old version and it needs a kludge similar to the tcl implementation, because xotcl does not support method-names starting with a "-". The -set could be certainly replaced by something more efficient (e.g. ad_conn store FLAG VALUE) at least in the cases where it matters. I would expect a similar speedup. When i added a filter to ad_conn, i saw much more read than store operations...

5: Re: ad_conn (response to 1)
Posted by Don Baccus on
Oh, yes, you're right andrew ...
6: Re: ad_conn (response to 1)
Posted by Gustaf Neumann on
There is an inconsistency with the subcommands of ad_conn: While all other subcommands of [ns_conn ...] are available through [ad_conn ...], [ns_conn request] is not. [ad_conn request] is completely different form [ns_conn request] and returns a oacs-managed request_counter, and not the request line of the request header like ns_conn.

Are there any objections to change the current [ad_conn request] to [ad_conn request_counter], such that [ad_conn request] behaves like [ns_conn request]? [ad_conn request] is only used in DS. Would this require a TIP?

7: Re: ad_conn (response to 1)
Posted by Malte Sussdorff on
Your proposed switch requires a TIP and for backward compatibility we would have to make sure that the old expected behaviour still works or exchange it everywhere in the OpenACS code.

As the same goes for making the XoTCL version of ad_conn the default one (which I'd prefer, if it works), you could provide the new package for testing somewhere and then make a TIP for 5.3. As XoTCL is a standard component in 5.3, this should not be much of an issue.

Looking forward to it.

8: Re: ad_conn (response to 1)
Posted by Gustaf Neumann on
Note, that the current behavior of ad_conn request seems nowhere documented. I do not believe that there are applications aside ds that rely on it. The documentation says:
Valid options for ad_conn are: request, sec_validated, browser_id, session_id, user_id, token, last_issue, deferred_dml, start_clicks, node_id, object_id, object_url, object_type, package_id, package_url, instance_name, package_key, extra_url, system_p, path_info, recursion_count.
Therefore i would not regard this a change of the public interface, but rather a bugfix or an internal change. A switch for backward-comaptibiliy seems a overkill and impractical, since any code relying on the proposed behavior would need the same switch. The current behavior is
   ad_conn request

   ns_conn request
   POST /ds/shell HTTP/1.1
See, this is something completely different. I have no personal interest in changing this, but was simply trapped by this behavior when experimenting with the xotcl-based ad_conn. The proposed change is completely independent from xotcl. Implementing this change with fixing all affected places in the packages of oacs-5-2 costs most probably less time than writing this summary.

Actually, the new xotcl-based ad_conn is more complicated than it needs to be, since it tries to be fully backward compatible with the current one. So, according to the TIP rules, this would not require a TIP.

The new version is more complicated than needed, since it has to deal with e.g. accessing and modifying the global (undocumented) associative array ad_conn()), it should stop forwarding to ns_conn, once the values were overwritten via -set, and it has to be compatible with the approach of the current ad_conn to check whether the command/object was initialzed already (the current ad_conn command is always available and has to be initialized with -reset). Furthermore the method calls to ad_conn have the form of a flag (starting with a -) while reading the values happens through methods. As a consequence, i would not regard the implementation of ad_conn in xotcl as cleanest the most beautiful way expressing the required semantics. One would have done this probably different haveing a more general object framework in mind, without inventing per-command different semantics.

Conceptually, ad_conn is more a connection or a request object than a connection command. As an object, it should be created early in the request processor and deleted when the request is over (in xotcl terminology a volatile object). This would care for initialization, cleanup etc. I would not regard the overwriting of values from ns_conn as a good practice (but we do this as well in learn@wu for getting the peer address from before pound). As object, the initialization is straightworward. ad_conn could be seen as an instance of a subclass from ns_conn and should provide means for further subclassing. All calls should go through the same interface.

So, what i am saying here is that in order to make ad_conn a nice and well designed piece of code, much more is needed to be done than simply reimplementing it in a faster manner. Such a re-design would certainly make some discussions and a tip necessary. But i doubt it makes sense in 5.3 to require a tip for every use of xotcl in core functions ...