[PATCH] please review: interpreter allocation

[Torsten Foertsch <to...@gmx.net>]

Hi Gozer,

this patch is a complete rework of the interpreter allocation. I have tested 
it with apache 2.2.6 with prefork and worker MPM but only with a perl 5.8.8 
that has defined USE_ITHREADS. The rest of this text concerns only the worker 
MPM.

The main idea behind the patch is to eliminate hash lookups as much as 
possible while selecting an interpreter.

In previous versions of mod_perl the interpreter currently in use was saved as 
userdata in a request or connection pool according to PerlInterpScope or in 
the rcfg structure if scope==handler. This means each phase does a hash 
lookup in the pool's userdata to get the interpreter.

This is now eliminated. The current interpreter is saved in the ccfg 
structure. modperl_interp_select has only to there once to discover whether a 
new interpreter needs to be allocated or there is one that can be used.

However, there is one case where I could not eliminate the hash lookup. 
modperl_module_cmd_take123 and modperl_module_config_merge don't work on a 
request_rec or conn_rec but only on a pool. While at startup time this pool 
is pconf at request time it is allways the request pool. So in 
modperl_hook_create_request I put a pointer to the request_rec as userdata in 
the request pool just in case a custom configuration directive is used 
somewhere in the request cycle. This is certainly not an ideal solution. So, 
if someone know of a way to convert a request pool into the corresponding 
request_rec please let me know. So modperl_interp_pool_select called at 
request time does a hash lookup to fetch the request_rec pointer and then 
calls modperl_interp_select.

The lifetime of an interpreter is now completely controlled by its refcnt. If 
it is unselected with a refcnt==0 it is put back into the pool. Otherwise the 
refcnt is simply decremented. The selection process is also quite simple. If 
ccfg->interp is not NULL it means there is already an interpreter assigned to 
this connection. If it is NULL a new interpreter is pulled from the pool and 
put in ccfg->interp. At this stage its refcnt is 0. Now PerlInterpScope is 
consulted to decide what to do. For scope==handler nothing is done. For 
scope==request a pool cleanup that calls modperl_interp_unselect is 
registered with the main request pool and the refcnt is incremented. For 
scope==subrequest or scope==connection a the cleanup function is registered 
with the appropriate pool.

This has a few consequences. Mainly, the first time an interpreter is needed 
decides about its scope. If a URI translation handler needs an interpreter 
and the server wide scope is request the interpreter is locked for the whole 
request time even if a subsequent <Location> says otherwise. Further, once an 
interpreter is allocated for a request, subrequest or connection it is used 
in all cases that need one. That means a <Location> that sets scope==handler 
will not acquire a new one but use the one already assigned to the request.

A word about cleanup functions. In previous versions in case of scope==handler 
a PerlCleanupHandler was called every time the interpreter was unselected. 
But it has never really worked and it somehow contradicts with the docs that 
say "It is used to execute some code immediately after the request has been 
served (the client went away) and before the request object is destroyed."
The patched modperl behaves according to the docs in that a PerlCleanupHandler 
is called at the end of a request even with scope==handler. That means also 
that as with each other phase with scope==handler a PerlCleanupHandler is not 
guaranteed to be called with a specific (previously used) interpreter.

However, there are ways to extend the lifetime of an interpreter from perl 
level. A new pool adds a reference to the current interpreter. So even with 
scope==handler one can say:

sub fixup {
  $_[0]->pnotes->{lock_interpreter}=APR::Pool->new;
  $_[0]->push_handlers(PerlCleanupHandler=>sub {
    delete $_[0]->pnotes->{lock_interpreter};
    return Apache2::Const::DECLINED;
  });
  return Apache2::Const::DECLINED;
}

This guarantees the same interpreter from fixup to cleanup. Do not use 
$r->pool->new in this case. It creates a sub-pool of the request pool which 
is destroyed before the PerlCleanupHandler is called.

Also, a pool cleanup function locks the interpreter until the pool is 
destroyed. So avoid $r->pool->cleanup_register if scope==handler is what you 
want.

I think the patch is fairly complete regarding interpreter 
allocation/deallocation. It may have flaws with other resources. For example 
I think pnotes need to be dropped whenever an interpreter is freed. I still 
have to check that.

Also, there are calls to PERL_SET_CONTEXT(aTHX) throughout the code. Since I 
do not really know what they are for I cannot say if there are some missing.

Please review the patch.

Thanks,
Torsten

Re: [PATCH] please review: interpreter allocation

[Torsten Foertsch <to...@gmx.net>]

On Tuesday 25 September 2007 15:52, Torsten Foertsch wrote:
> It may have flaws with other resources. For example
> I think pnotes need to be dropped whenever an interpreter is freed. I still
> have to check that.

This is an update to the yesterday patch. It cures a problem that occurred to 
me last night.

In modperl_callback_run_handlers() there is this comment:

-    /* XXX: would like to do this in modperl_hook_create_request()
-     * but modperl_interp_select() is what figures out if
-     * PerlInterpScope eq handler, in which case we do not register
-     * a cleanup.  modperl_hook_create_request() is also currently always
-     * run even if modperl isn't handling any part of the request
-     */
-    modperl_config_req_cleanup_register(r, rcfg);

This is the place where the function that calls a PerlCleanupHandler is 
registered. It occurred to me that modperl_callback_run_handlers() is called 
only if at least one Perl*Handler is called in the request cycle. So a 
PerlCleanupHandler cannot be called unless there is an other Perl*Handler 
previously called in the same request cycle.

On top of the interpreter allocation patch it is very easy to fix that. Simply 
do what the comment suggests.

The attached "x" adds a test case for the bug.

The attached trunk-threads.patch contains the fix plus the test case.

Torsten

Re: [PATCH] please review: interpreter allocation

["Philippe M. Chiasson" <go...@ectoplasm.org>]

Torsten Foertsch wrote:
> Hi Gozer,
> 
> On Tuesday 25 September 2007 15:52, Torsten Foertsch wrote:
>> I think pnotes need to be dropped whenever an interpreter is freed.
> 
> Currently $c->pnotes are dropped in a connection pool cleanup while $r->pnotes 
> are dropped just after PerlCleanupHandler. A comment in 
> modperl_util.c:modperl_pnotes says:
> 
>         /* XXX: It would be nice to be able to do this with r->pnotes, but
>          * it's currently impossible, as modperl_config_request_cleanup()
>          * is responsible for running the CleanupHandlers, and it's cleanup
>          * callback is registered very early. If we register our cleanup here, 
>          * we'll be running *before* the CleanupHandlers, and they might still
>          * want to use pnotes...
>          */
> 
> My idea to resolve this problem is why not create a subpool of $r->pool and 
> register the PerlCleanupHandler with this pool?

That would work.

> Destruction of subpools is the first thing apr_pool_destroy does. That way a 
> PerlCleanupHandler is run always before any $r->pool cleanups.

I like it, that's very simple and could move the cleanup logic out of there,
as well as that long, annoying comment ;-)

> Another question, with the previous logic with scope==handler a 
> PerlCleanupHandler was run each time the interpreter was put back to the 
> interpreter pool.

And I am not sure this was a planed feature.

> This was also the time when pnotes were dropped.

Definitely not a feature then, IMO.

> Now the 
> CleanupHandler is called at the end of the request even with scope==handler. 
> So if for example a transhandler uses pnotes and the interpreter is put back 
> after the translation phase these pnotes are not dropped.
> 
> What do you think would be better:
> 
> - if a populated pnotes hash locks the interpreter just like a new pool does

+1

> or
> 
> - if we add some logic to drop $r->pnotes and $c->pnotes if the interpreter is 
> freed?

Nah, that's not what would be expected of pnotes. You expect pnotes to be a safe
place to stash stuff for the duration of $r/$c

> The question thus is does the InterpScope overrule allocated resources or do 
> they overrule the scope?

I would see InterpScope as advisory, really.

When I say InterpScope handler, I am asking for as much concurrency
as possible, important word: possible.

So if you stick stuff in pnotes, all bets are off. We try our best, but
it might mean that particular interpreter is tied up for the request.

Sounds to me like that's the only way to keep doing the right thing with the
overall behaviour of things (like pnotes).

------------------------------------------------------------------------
Philippe M. Chiasson     GPG: F9BFE0C2480E7680 1AE53631CB32A107 88C3A5A5
http://gozer.ectoplasm.org/       m/gozer\@(apache|cpan|ectoplasm)\.org/

Re: [PATCH] please review: interpreter allocation

[Torsten Foertsch <to...@gmx.net>]

Hi Gozer,

On Tuesday 25 September 2007 15:52, Torsten Foertsch wrote:
> I think pnotes need to be dropped whenever an interpreter is freed.

Currently $c->pnotes are dropped in a connection pool cleanup while $r->pnotes 
are dropped just after PerlCleanupHandler. A comment in 
modperl_util.c:modperl_pnotes says:

        /* XXX: It would be nice to be able to do this with r->pnotes, but
         * it's currently impossible, as modperl_config_request_cleanup()
         * is responsible for running the CleanupHandlers, and it's cleanup
         * callback is registered very early. If we register our cleanup here, 
         * we'll be running *before* the CleanupHandlers, and they might still
         * want to use pnotes...
         */

My idea to resolve this problem is why not create a subpool of $r->pool and 
register the PerlCleanupHandler with this pool?

Destruction of subpools is the first thing apr_pool_destroy does. That way a 
PerlCleanupHandler is run always before any $r->pool cleanups.


Another question, with the previous logic with scope==handler a 
PerlCleanupHandler was run each time the interpreter was put back to the 
interpreter pool. This was also the time when pnotes were dropped. Now the 
CleanupHandler is called at the end of the request even with scope==handler. 
So if for example a transhandler uses pnotes and the interpreter is put back 
after the translation phase these pnotes are not dropped.

What do you think would be better:

- if a populated pnotes hash locks the interpreter just like a new pool does

or

- if we add some logic to drop $r->pnotes and $c->pnotes if the interpreter is 
freed?

The question thus is does the InterpScope overrule allocated resources or do 
they overrule the scope?

Torsten