Re: Please help adding ModPerl::Interpreter

Hi,

for testing purposes I need an interface to the
modperl_interp_t. That means 
I'd like to check the current interp's refcnt and
num_requests members from 
perl level.

So I thought of adding a ModPerl::Interpreter XS module. But
after reading 
http://perl.apache.org/docs/2.0/devel/core/explained.ht
ml I am only more 
confused.

Why is it so complicated to add a new XS module? Is it
really necessary to 
edit all these tables and maps? Further, somewhere is said
that the source 
scanning is not yet stable. In fact I have tried it some
time ago and got 
completely different results.

I know how to create such a module outside modperl but it
would depend on 
modperl. But I need it to test modperl itself. Hence I
cannot build the 
module outside modperl because I would not be able to
install it without 
modperl.

So, if someone knows better than me please help. A patch
that adds a framework 
for the new module to modperl so that I could simply fill in
my stuff would 
be best.

An explanation what to would also be most appreciated.

Thanks,
Torsten

Torsten Foertsch wrote:
> Hi,
> 
> for testing purposes I need an interface to the
modperl_interp_t. That means 
> I'd like to check the current interp's refcnt and
num_requests members from 
> perl level.
> 
> So I thought of adding a ModPerl::Interpreter XS
module. But after reading 
> http://perl.apache.org/docs/2.0/devel/core/explained.ht
ml I am only more 
> confused.
> 
> Why is it so complicated to add a new XS module? Is it
really necessary to 
> edit all these tables and maps? 

yeah 

if you're really confused, see the commits when I added
Apache::MPM

  http://marc.info/?l=apache-modperl-cvs&m
=106978727408877&w=2
  http://marc.info/?l=apache-modperl-cvs&m
=106978912311523&w=2

the bulk of the main commit consists of the tests, but the
interesting
changes are at the bottom.  if you just follow that pattern
you should
be able to add the module of your choice.

> Further, somewhere is said that the source 
> scanning is not yet stable. In fact I have tried it
some time ago and got 
> completely different results.

yeah.  IIRC we were leaving it to doug and his hacked
version of C::Scan
to generate the official scan, with the rest of us modifying
it as you
see in the commits.

> 
> I know how to create such a module outside modperl but
it would depend on 
> modperl. But I need it to test modperl itself. Hence I
cannot build the 
> module outside modperl because I would not be able to
install it without 
> modperl.
> 
> So, if someone knows better than me please help. A
patch that adds a framework 
> for the new module to modperl so that I could simply
fill in my stuff would 
> be best.
> 
> An explanation what to would also be most appreciated.

HTH

--Geoff

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribeperl.apache.org
For additional commands, e-mail: dev-helpperl.apache.org

Torsten Foertsch wrote:
> Hi Geoff,
> 
> On Tuesday 09 October 2007 19:19, Geoffrey Young
wrote:
>> if you're really confused, see the commits when I
added Apache::MPM
>>
>>   http://marc.info/?l=apache-modperl-cvs&m
=106978727408877&w=2
>>   http://marc.info/?l=apache-modperl-cvs&m
=106978912311523&w=2
>>
>> the bulk of the main commit consists of the tests,
but the interesting
>> changes are at the bottom.  if you just follow that
pattern you should
>> be able to add the module of your choice.
> 
> I think I did it. Please have a look at the attached
patch to see if it is all 
> that is needed to be done.

it looks ok.  the proof is in the tests, of course 

> 
> The patch is a bit edited to suppress other changes in
xs/. So I don't think 
> it would apply to trunk without warnings.

I don't understand that.  what other changes in xs/?  if you
mean
missing directories, etc, you can use utils/getdiff.pl to
generate a
reasonable diff that adds them.

--Geoff

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribeperl.apache.org
For additional commands, e-mail: dev-helpperl.apache.org

Torsten Foertsch wrote:
> Hi Geoff,
> 
> On Tuesday 09 October 2007 19:19, Geoffrey Young
wrote:
>> if you're really confused, see the commits when I
added Apache::MPM
>>
>>   http://marc.info/?l=apache-modperl-cvs&m
=106978727408877&w=2
>>   http://marc.info/?l=apache-modperl-cvs&m
=106978912311523&w=2
>>
>> the bulk of the main commit consists of the tests,
but the interesting
>> changes are at the bottom.  if you just follow that
pattern you should
>> be able to add the module of your choice.
> 
> I think I did it. Please have a look at the attached
patch to see if it is all 
> that is needed to be done.

It looks pretty good, awesome!

> The patch is a bit edited to suppress other changes in
xs/. So I don't think 
> it would apply to trunk without warnings.

It did apply clean for me.

The only question I have is wherever it's necessary to also
expose the
following 2 items:

modperl_interp_pool_t * | IV
PerlInterpreter *       | IV

Especially in the current patch, what you get out will
pretty much
be a useless blessed scalar you can't do anything with.

Apart that, looks great. At this point, I'd be happy to see
it go in,
without documentation even, but at least with a minimal set
of tests,
something like:

my $int = ModPerl::Interpreter->current;
ok($int);
is($int, 'ModPerl::Intereter');

ok($int->mip);
ok($int->refcnt);
[...]

To at least cover normal usage cases.

Good work !
------------------------------------------------------------
------------
Philippe M. Chiasson     GPG: F9BFE0C2480E7680
1AE53631CB32A107 88C3A5A5
http://gozer.ectoplasm.or
g/       m/gozer(apache|cpan|ectoplasm).org/

On Wednesday 10 October 2007 21:20, Philippe M. Chiasson
wrote:
> The only question I have is wherever it's necessary to
also expose the
> following 2 items:
>
> modperl_interp_pool_t * | IV
> PerlInterpreter *       | IV
>
> Especially in the current patch, what you get out will
pretty much
> be a useless blessed scalar you can't do anything
with.

Without these 2 lines no accessor methods would be created
because the wrapxs 
process obviously doesn't know how to convert it. In my
first version I had 
declared mip and perl as void* in 
xs/tables/current/Apache2/StructureTable.pm. This way I also
got these 
members compiled in. But then I suspected that a source scan
would never 
generate that. Am I right?

BTW, I forgot to mention, the patch creates a new map: 
xs/maps/modperl_structures.map. Is that OK?

> Apart that, looks great. At this point, I'd be happy to
see it go in,
> without documentation even, but at least with a minimal
set of tests,
> something like:
>
> my $int = ModPerl::Interpreter->current;
> ok($int);
> is($int, 'ModPerl::Intereter');
>
> ok($int->mip);
> ok($int->refcnt);
> [...]
>
> To at least cover normal usage cases.

Of course.

I need this module mainly for testing the interpreter
management. The pair 
($$interp, $interp->num_requests) can be used as unique
identifier for one 
particular usage (the time between interp_select and
interp_unselect) of that 
interpreter.

refcnt is used to check for example if creating pnotes would
lock the interp.

mip and perl are currently not used. They can be dropped if
you think that 
would be better. I am aware that if later on someone decides
that a perl 
interface to the mip structure would be good he has to
change the API. But 
that can be documented. On the other hand mip may be a
useful tool to check 
if a vhost with +Parent has really got a separate mip. But
then an interface 
to the mip->tipool might be useful to check how much
interpreters are in use 
at a certain point in time. Maybe I should provide a perl
interface to that 
too.

Torsten

Torsten Foertsch wrote:
> On Wednesday 10 October 2007 21:20, Philippe M.
Chiasson wrote:
>> The only question I have is wherever it's necessary
to also expose the
>> following 2 items:
>>
>> modperl_interp_pool_t * | IV
>> PerlInterpreter *       | IV
>>
>> Especially in the current patch, what you get out
will pretty much
>> be a useless blessed scalar you can't do anything
with.
> 
> Without these 2 lines no accessor methods would be
created because the wrapxs 
> process obviously doesn't know how to convert it. In my
first version I had 
> declared mip and perl as void* in 
> xs/tables/current/Apache2/StructureTable.pm. This way I
also got these 
> members compiled in. But then I suspected that a source
scan would never 
> generate that. Am I right?

Yes, absolutely correct!

> BTW, I forgot to mention, the patch creates a new map:

> xs/maps/modperl_structures.map. Is that OK?

That's more than okay, that was the correct thing to do.

>> Apart that, looks great. At this point, I'd be
happy to see it go in,
>> without documentation even, but at least with a
minimal set of tests,
>> something like:
>>
>> my $int = ModPerl::Interpreter->current;
>> ok($int);
>> is($int, 'ModPerl::Intereter');
>>
>> ok($int->mip);
>> ok($int->refcnt);
>> [...]
>>
>> To at least cover normal usage cases.
> 
> Of course.

In the meantime, I've created a threading branch here:

http://svn.apache.org/repos/asf/perl/modperl/bran
ches/threading

And I've applied this change (rev 584377) as the first one
in hopefully many more of
your patches. Only change is that I added minimal tests.

Note, this patch/test doesn't behave correctly with a
non-threaded Apache.

> I need this module mainly for testing the interpreter
management. The pair 
> ($$interp, $interp->num_requests) can be used as
unique identifier for one 
> particular usage (the time between interp_select and
interp_unselect) of that 
> interpreter.
> 
> refcnt is used to check for example if creating pnotes
would lock the interp.
> 
> mip and perl are currently not used. They can be
dropped if you think that 
> would be better

It's not much work to include them, so might as well keep
them in.

> I am aware that if later on someone decides that a perl

> interface to the mip structure would be good he has to
change the API.

It's a change that would add to the existing API, so I
wouldn't worry about
that case.

> But 
> that can be documented. On the other hand mip may be a
useful tool to check 
> if a vhost with +Parent has really got a separate mip.
But then an interface 
> to the mip->tipool might be useful to check how much
interpreters are in use 
> at a certain point in time. Maybe I should provide a
perl interface to that 
> too.

Maybe, yes. At some point I remember thinking that it would
have simplified
things to just expose Perl interpreters to Perl, and do
interpreter managment
in perl, but that's for another time 

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

Torsten Foertsch wrote:
> On Saturday 13 October 2007 08:31, Philippe M. Chiasson
wrote:
>> In the meantime, I've created a threading branch
here:
>>
>> http://svn.apache.org/repos/asf/perl/modperl/bran
ches/threading
>>
>> And I've applied this change (rev 584377) as the
first one in hopefully
>> many more of your patches. Only change is that I
added minimal tests.
>>
>> Note, this patch/test doesn't behave correctly with
a non-threaded Apache.
> 
> Thanks!

No problems! While you are at it, why don't you submit a
patch for:

http://perl.apache.org/about/contributors/people.html

Source here:

http//svn.apache.org/repos/asf/perl/modperl/docs/trunk/src/a
bout/contributors/

> Please apply the enclosed x.patch to the threading
branch. I have tested it 
> with perl 5.8.8 (with threads), apache 2.2.6 (worker
and prefork) on linux.
> 
> I think it needs some polishing to work with a perl
without ithreads.

It certainly needs some #ifdef foo to be friendly to
non-ithreads perls

> The patch contains all my findings so far including the
pnotes refcount 
> problem. Pnotes now lock the interpreter like pools
do.

Any chance you can break the patch into multiple patches,
one for each
feature/fix? Ideally with an accompanying entry in Changes ?
It'll be simpler
to merge these one at a time back to the trunk/

> There is a new ${r|c}->pnotes_kill function that can
be used to prematurely 
> delete pnotes.

Not sure about kill, how aobut:

->pnotes_reset() ?
->pnotes_destroy() ?

> It is useful if you want to use pnotes say from trans
to fixup 
> and thus bind the interp to the request but remove that
binding before 
> response. So, PerlInterpScope is advisory now.
> 
> Also new interfaces incl. tests (but without docs) for
ModPerl::Interpreter, 
> ModPerl::InterpPool, ModPerl::TiPool and
ModPerl::TiPoolConfig are included.

Cool, will need to have a look at all that.

> The PUTBACK flag for interpreters is removed since it
is not used anymore.

Good riddance.

> The attached xx patch deletes an unused structure
member from 
> modperl_interp_pool_t.

Comitted on the trunk/ as revision 585724.

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

On Thursday 18 October 2007, Philippe M. Chiasson wrote:
> > The patch contains all my findings so far
including the pnotes refcount
> > problem. Pnotes now lock the interpreter like
pools do.
>
> Any chance you can break the patch into multiple
patches, one for each
> feature/fix? Ideally with an accompanying entry in
Changes ? It'll be
> simpler to merge these one at a time back to the
trunk/

Is the test suite expected to succeed after each patch? I
can think of a few 
minor patches like pnotes, cleanuphandler, logging the pid
with modperl_trace 
plus one big chunk with the basic interpreter management.
Otherwise it 
doesn't make sense for me.

> > There is a new ${r|c}->pnotes_kill function
that can be used to
> > prematurely delete pnotes.
>
> Not sure about kill, how aobut:
>
> ->pnotes_reset() ?
> ->pnotes_destroy() ?

It was named after apr_pool_cleanup_kill(). If you don't
like it then what do 
you prefer _destroy or _reset? To me it's all the same.

Torsten