On Mon, 2003-02-10 at 22:37, Stas Bekman wrote:
> [...]
>
> >>The patch needs some minor tweaking to conform with our coding style, but
> >>overall it looks good. Though before we move on with this, won't it be simpler
> >>to use the 'fields' pragma?
> >
> >
> > Sorry about that - I'll take another look at the mod_perl coding
> > guidelines before submitting another patch.
>
> That's not a problem. let's first figure out what's the best approach to take,
> before wasting time on fixing the style.
Agreed.
>
> > Using fields would definitely be a good way of going about it. I didn't
> > choose that approach in the code I submitted because:
> > 1) I felt that implementing RegistryCooker as an array based object was
> > a good design philosophy, and my changes (hopefully) shouldn't have much
> > of an impact on the current working of the module unless its used in a
> > subclass (and we have a win in that situation.)
>
> A good design philosophy is when the parent class can change its internal
> object implementation without affecting its sub-classes ;) I simply didn't
> think of sub-classes wanting to extend the object itself, but only to override
> some methods.
>
I think both solutions probably meet this goal - with my original patch
it can be done by changing a couple of subs (public and make_accessor).
With the fields pragma, we could change the underlying implementation as
well (although without changing any subclasses we could really only
change to a hash implementation, or something else that emulated one.)
> > 2) Anything that looks like pseudo-hashes scares me. :-) Pseudo-hashes
> > seem to be in flux, and as a result I went with the more "old fashioned"
> > approach.
>
> Yes, pseudo-hashes are out of question.
>
I was referring to the fact that AFAIK the fields pragma is currently
(with 5.61 anyway, haven't checked with 5.8) implemented in terms of
pseudo-hashes. Not really important though since we don't have to mess
with them directly.
> > I can also see it either way though:
> > 1) It would be less code to implement this change as a fields pragma.
> > 2) The fields pragma is promised to work as advertised even when the
> > current pseudo-hash implementation goes away (as per the perlref doco.)
> >
> > I think overall I currently feel more comfortable with the array based
> > patch mostly because even though its more lines, it seems to remain more
> > familiarity to the original code. Also I felt that any performance or
> > integration testing that has already been done with Registry scripts may
> > be slightly invalidated with a change to the implementation structure of
> > the module.
>
> But this introduces a custom non-standard extending technique, which should
> rather stay away from if we can.
>
I definitely agree with this point. Overall, using a fields pragma may
be cleaner in that regard.
On the other hand, with this extending technique we also gain the
flexibility of changing any of those accessors/mutators in the future to
do input parameter checking, side effects, etc. To do that we would just
make a accessor/mutator by hand. For example:
sub FILENAME {
my $self = shift;
my $val = undef;
if(@_) {
$val = $_[0];
... code to check value, change $val, return, etc ...
$self->[_FILENAME] = $val;
}
return $self->[_FILENAME];
}
And rename the current FILENAME accessor/mutator in the call to public.
We could also decide later to make some of the members read only this
way, deprecate them using warnings, etc.
> The test suite is covering enough to safely verify any refactorings, so I'm
> not worried about completely changing the internal object implementation. And
> no performance benchmarking was done so far.
>
> > While I was researching your question I mostly coded a solution to make
> > this change with the fields pragma, just haven't tested it yet. If you
> > want, I can also post that version to compare the current proposed patch
> > with.
>
> It'd be nice to keep the extension mechanism as simple as possible, as long as
> we don't lose in performance. The fields manpage says that the fields are
> compiled, so perhaps using those will give us the same performance (or equal
> enough) compared with array-based objects. Is there some document that
> compares these two? I'm sure Damian's book talks about them all, but I'm not
> sure if there is a performance comparison there.
>
I'm wrestling with this right now - the documentation for fields says:
"The effect of all this is that you can have objects with named fields
which are as compact as arrays and as fast to access. This only works as
long as the objects are accessed through properly typed lexical
variables, though. If the variables are not typed, access is only
checked at run time, so your program runs slower because it has to do
both a hash access and an array access."
Since this is compile time does this mean it has to be done throughout?
If they need to be typed throughout we could lose some performance as I
assume mod_perl can't type the lexical when calling handler as a method,
unless the following (in ModPerl::RegistryCooker) would suffice to
satisfy this:
my ModPerl::RegistryCooker $class = (@_ >= 2) ? shift : __PACKAGE__;
I've tested re-implementing the changes using the fields pragma, and it
seems to run fine for registry scripts. I've run into another problem
while testing using a fields pragma implementation however - since the
class fields are setup at compile time, it looks like its not working
with my Apache::PAR::Registry module which modifies @ISA when called (I
know, bad idea anyway, but its the only way I've found so far to support
both mod_perl 1.x Apache::Registry and ModPerl::RegistryCooker from the
same module.) Basically, my module does something like the following (in
Apache::PAR::Registry):
if(eval "Apache::exists_config_define('MODPERL2')") {
@ISA = qw(Exporter Apache::PAR::RegistryCooker);
require Apache::PAR::RegistryCooker;
}
else {
@ISA = qw(Exporter Apache::PAR::ScriptBase Apache::RegistryNG);
require Apache::RegistryNG;
require Apache::PAR::ScriptBase;
}
The original patch works with this because all of the real work is done
in Apache::PAR::RegistryCooker, which is a sub-class of
ModPerl::RegistryCooker, and so creates the accessors correctly.
Unfortunately, I'm running out of time tonight to make a simpler test
case, I'll have to pick this up tomorrow.
Thanks,
--
Nathan Byrd <[EMAIL PROTECTED]>
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
