perl-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Torsten Foertsch <torsten.foert...@gmx.net>
Subject Re: What can I do ...
Date Mon, 14 May 2007 09:11:56 GMT
On Monday 14 May 2007 08:35, Philippe M. Chiasson wrote:
> >> http://www.gossamer-threads.com/lists/modperl/dev/93415
> >
> > This one looks pretty good, and I think it's a cleaner approach
> > to the currently uncorrect one.
>
> I've just spent the evening looking at this more in details, and the
> more I look, the more things look significantly broken. My latest
> feeling was that your patch was not actually adressing the underlying
> problem.

I was there myself, see 
http://www.gossamer-threads.com/lists/modperl/dev/92674#92674

> Here is where I am at so far. In many cases, once an interpreter has been
> acquired and not yet released, nothing should really need to call
> interp_select() to find an interpreter, as opposed to being handed that
> interpreter directly.

The point is you can go your way and prevent recursive calls. That'd mean 
redesign of the modperl_response_handler* functions at least. Or you can make 
modperl_interp_unselect reenterable and handle the recursion. That is what I 
did. I think refcounting is a quite good approach to that. I see 
interp_select() as the interface that hands you the *right* interpreter. It 
handles all the details of which interp and where it comes from. The thing 
that disturbs me a bit is the PUTBACK flag and its inspection all over the 
code. I think it would be cleaner to call select/unselect each time and let 
them decide what to do. And what disturbs me even more is the way 
select/unselect is called, sometimes though a macro, sometimes directly, 
sometimes conditionally ("if(c||r)" or so). It was very hard to figure out 
what was going on.

The semantics of PerlCleanupHandler is a bit strange in case of scope==handler 
but I think it is quite good the way it is. It needs to be explained as a way 
to deallocate resources that are bound to a certain perl instance while a 
pool cleanup handler handles resources that are bound to the 
request/connection/process whatever the pool belongs to.

> On the topic of interp->refcnt, check out xs/APR/Pool/APR_Pool.h, it's
> the current customer of it, and uses it again in a strange/unobvious
> way.
>
> Anyways, I'll be poking at this some more shortly and post more of my
> findings.

I really like the idea of refcounting. Initially I was looking for a way to 
allocate an interp from trans to fixup and release it just before response. 
This way most of the slow network IO is made without an interpreter. But it 
preserves the possibility to communicate via pnotes between the phases.

With my patch and the way APR::Pool handles refcounts it can be done very 
simple:

in trans:
$r->pnotes->{my_pool}=$r->pool->new;

this preserves the interp from being freed.

Then in fixup:
delete $r->pnotes->{my_pool};

And since the interp is stored in rcfg it is also the faster way compared with 
a scope==request. With scope==request||subrequest the interp is stored in the 
request pools userdata. That means a hash lookup instead of a simple 
structure access. It would be even faster if that initial subpool creation 
could be avoided. For that I think a Apache2::Interp or Modperl::Interp 
module would be good to access the refcnt. Would something like that be 
accepted? What would be the right name?

I'd vote for something like rcfg->interp but not in rcfg but in a connection 
bound structure to always store the interp currently in use. Then decisions 
about the lifetime of that field are made completely in interp_(un)select(). 
This pair of functions is then called every time an interp is needed. For the 
scope==subreq case there must be something special (an apr_array) then I 
think.

Another strangeness is that PerlInterpScope is of directory scope. What to do 
when there is a PerlTransHandler configured with scope==handler but in 
MapToStorage a PerlInterpScope directive says it is scope==subreq or so. This 
needs to be investigated I think.

> Torsten, thanks for the ithreads3 test case, it does correctly exercice
> the problems every time. (I'd like to include it, but it would need to
> be somewhat cleaned up to follow our current coding conventions, apart
>
> >from that, pretty good).

This weekend I made a patch to allow APR::ThreadRWLock by now without test. 
I'll see if it does what I expect it to do and then send it to the list.

Torsten

Mime
View raw message