mro "PERFORMANCE CONSIDERATIONS" still true?

I'm reading the docs for mro for the first time and came
across the section on
PERFORMANCE CONSIDERATIONS...

    Specifying the mro type of a class before setting
C<ISA> will
    be faster than the other way around.  Also, making all
of your
    C<ISA> manipulations in a single assignment or push
statement
    will be faster that doing them one by one (which is
what
    C<use base> does currently).

    Examples:

      # The slowest way
      package Foo;
      use base qw/A B C/;
      use mro 'c3';

      # Equivalently slow
      package Foo;
      our ISA;
      require A; push(ISA, 'A');
      require B; push(ISA, 'B');
      require C; push(ISA, 'C');
      use mro 'c3';

      # The fastest way
      # (not exactly equivalent to above,
      #   as base.pm can do other magic)
      package Foo;
      use mro 'c3';
      require A;
      require B;
      require C;
      our ISA = qw/A B C/;

I know there was some work done, is this stuff still true? 
Even if it is,
just how big a class hierarchy does there have to be before
there's a
noticeable difference?  If there's no noticeable difference,
can we just
remove this microoptimization discussion?

I tried all three using Module::Build, Params::Check and
ExtUtils::MakeMaker
and got no discernible difference.  200ms user time each
time.  Removing "use
mro" also makes no difference.

I also note that in the first example "use base"
is run before "use mro", yet
in the "fastest" example mro is run after ISA has
been set up.  Can base just
be run after, as per the instructions at the top, to get the
fastest way and
the shortest invocation?

	package Foo;
	use mro 'c3';
	use base qw/A B C/;


-- 
I have a date with some giant cartoon robots and booze.

On Sep 09 2007, Michael G Schwern wrote:
> 
>       # The slowest way
>       package Foo;
>       use base qw/A B C/;
>       use mro 'c3';
> 
>       # Equivalently slow
>       package Foo;
>       our ISA;
>       require A; push(ISA, 'A');
>       require B; push(ISA, 'B');
>       require C; push(ISA, 'C');
>       use mro 'c3';

> I know there was some work done, is this stuff still
true?

Yes, because of the way base.pm does a push for each class.

> Even if it is,
> just how big a class hierarchy does there have to be
before there's a
> noticeable difference?  If there's no noticeable
difference, can we just
> remove this microoptimization discussion?

Yes, please.  This will just scare people for no good
reason.

Although I see no reason why we shouldn't modify base.pm so
that it
pushes all the classes in one statement at the end of
import().

-- 
Rick Delaney
rickbort.ca

On 9/9/07, Rick Delaney <rickbort.ca> wrote:
> On Sep 09 2007, Michael G Schwern wrote:
> >
> >       # The slowest way
> >       package Foo;
> >       use base qw/A B C/;
> >       use mro 'c3';
> >
> >       # Equivalently slow
> >       package Foo;
> >       our ISA;
> >       require A; push(ISA, 'A');
> >       require B; push(ISA, 'B');
> >       require C; push(ISA, 'C');
> >       use mro 'c3';
>
> > I know there was some work done, is this stuff
still true?
>
> Yes, because of the way base.pm does a push for each
class.
>
> > Even if it is,
> > just how big a class hierarchy does there have to
be before there's a
> > noticeable difference?  If there's no noticeable
difference, can we just
> > remove this microoptimization discussion?
>
> Yes, please.  This will just scare people for no good
reason.
>

Fine with me.  It really does take a fairly gigantic
hierarchy to see
the difference.

> Although I see no reason why we shouldn't modify
base.pm so that it
> pushes all the classes in one statement at the end of
import().
>

I tried to do this once without changing the behavior of
base.pm, and
I kinda gave up after a while.  It can be done, but it will
cause some
subtle changes that I'm sure would trip up someone relying
on "use
base".

The core issue (ignore fields stuff) is that base.pm wants
to check
->isa() before pushing each class, to see if it's really
necessary.
It will skip classes you explicitly asked for if they're
already
inherited from via some other inheritance path.  It can't
really do
this sanely without stepping through and pushing each one. 
I suspect
this was an optimization to remove redundant bases.  The
5.10 mro code
eliminates redundancy internally (and it does so more
effectively - it
can see through the whole tree at once), so it's no longer
an issue
there.

But still, given this example scenario:

package A; our ISA = qw//;
package B; our ISA = qw//;
package C; use base qw/A/;
package D; use base qw/C A B/;

With how base.pm works now, D::ISA ends up being ('C',
'B'), because
"A" was skipped (it had already pushed
"C", and so D already isa "A"
when it gets to that).  If we kill the ->isa() checks and
push them
all at once, now D::ISA will be exactly what the user asked
for:
('C', 'A', 'B').  The risk is that someone actually cares
about this
and would complain of breakage.  For inheritance purposes,
under both
traditional and C3 mro's, it doesn't make a difference
either way.

-- Brandon

On 10/09/2007, Brandon Black <blblackgmail.com> wrote:
> > > Even if it is,
> > > just how big a class hierarchy does there
have to be before there's a
> > > noticeable difference?  If there's no
noticeable difference, can we just
> > > remove this microoptimization discussion?
> >
> > Yes, please.  This will just scare people for no
good reason.
> >
>
> Fine with me.  It really does take a fairly gigantic
hierarchy to see
> the difference.

OK, I made the following change to mro.pm :

Change 31836 on 2007/09/10 by rgsstcosmo

	Remove the "performance considerations"
paragraph.

On Sep 09 2007, Brandon Black wrote:
> On 9/9/07, Rick Delaney <rickbort.ca> wrote:
> > Although I see no reason why we shouldn't modify
base.pm so that it
> > pushes all the classes in one statement at the end
of import().
> >
> 
> I tried to do this once without changing the behavior
of base.pm, and
> I kinda gave up after a while.  It can be done, but it
will cause some
> subtle changes that I'm sure would trip up someone
relying on "use
> base".

Wouldn't the attached patch be sufficient?  It will not have
any of the
problems you mentioned.

The only user-visible difference will be if someone's doing
something
weird like:

    package BBB;
    sub b;

    package CCC;
    eval q{ use base qw(BBB no_such_module); };

    print ISA;

Before the patch this would print "BBB" and after
it would print
nothing.  I think the unaltered ISA is an improvement over
a half-set
state left after an exception.

-- 
Rick Delaney
rickbort.ca

Rick Delaney wrote:
> On Sep 09 2007, Brandon Black wrote:
>> On 9/9/07, Rick Delaney <rickbort.ca> wrote:
>>> Although I see no reason why we shouldn't
modify base.pm so that it
>>> pushes all the classes in one statement at the
end of import().
>>>
>> I tried to do this once without changing the
behavior of base.pm, and
>> I kinda gave up after a while.  It can be done, but
it will cause some
>> subtle changes that I'm sure would trip up someone
relying on "use
>> base".
> 
> Wouldn't the attached patch be sufficient?  It will not
have any of the
> problems you mentioned.

Yeah, that's just what I was thinking.


-- 
Robrt:   People can't win
Schwern: No, but they can riot after the game.

On 9/10/07, Michael G Schwern <schwernpobox.com> wrote:
> Rick Delaney wrote:
> > On Sep 09 2007, Brandon Black wrote:
> >> On 9/9/07, Rick Delaney <rickbort.ca> wrote:
> >>> Although I see no reason why we shouldn't
modify base.pm so that it
> >>> pushes all the classes in one statement at
the end of import().
> >>>
> >> I tried to do this once without changing the
behavior of base.pm, and
> >> I kinda gave up after a while.  It can be
done, but it will cause some
> >> subtle changes that I'm sure would trip up
someone relying on "use
> >> base".
> >
> > Wouldn't the attached patch be sufficient?  It
will not have any of the
> > problems you mentioned.
>
> Yeah, that's just what I was thinking.
>
>

Yes, I personally like that patch.  I'm just worried that in
some
cases the resulting ISA will be different than it was before,
and
someone for some odd esoteric reason will be relying on
that.

-- Brandon

Brandon Black wrote:
> On 9/10/07, Michael G Schwern <schwernpobox.com> wrote:
>> Rick Delaney wrote:
>>> On Sep 09 2007, Brandon Black wrote:
>>>> On 9/9/07, Rick Delaney <rickbort.ca> wrote:
>>>>> Although I see no reason why we
shouldn't modify base.pm so that it
>>>>> pushes all the classes in one statement
at the end of import().
>>>>>
>>>> I tried to do this once without changing
the behavior of base.pm, and
>>>> I kinda gave up after a while.  It can be
done, but it will cause some
>>>> subtle changes that I'm sure would trip up
someone relying on "use
>>>> base".
>>> Wouldn't the attached patch be sufficient?  It
will not have any of the
>>> problems you mentioned.
>> Yeah, that's just what I was thinking.
> 
> Yes, I personally like that patch.  I'm just worried
that in some
> cases the resulting ISA will be different than
it was before, and
> someone for some odd esoteric reason will be relying on
that.

Ya know, if someone manages to recover from a 'use base'
exception, which is
tricky in and of itself because an eval BLOCK won't catch
it, and keeps going
and doesn't expect some odd things to happen... well,
they're loony.


-- 
Whip me, beat me, make my code compatible with VMS!

On 9/10/07, Michael G Schwern <schwernpobox.com> wrote:
> > Yes, I personally like that patch.  I'm just
worried that in some
> > cases the resulting ISA will be different than
it was before, and
> > someone for some odd esoteric reason will be
relying on that.
>
> Ya know, if someone manages to recover from a 'use
base' exception, which is
> tricky in and of itself because an eval BLOCK won't
catch it, and keeps going
> and doesn't expect some odd things to happen... well,
they're loony.
>
>

I don't mean the exception thing.  The behavior of the
current base
and the proposed change is different even without exceptions
(although
in ways you wouldn't think a sane module would care about,
but
still...).  When you do the following with the current
base.pm:

perl -wle 'package AA; ISA=(); package BB; use base qw/AA/;
package
CC; use base qw/BB AA/; package main; print join(q{,}, CC::ISA)'

It prints just "BB".  CC::ISA does not contain
"AA" even though it
was explicitly asked for.  This is due to the "next if
$inheritor->isa($base)" line.  With the proposed
push-all-at-once
logic, CC::ISA would instead be qw/BB AA/ as asked for.

Again, this doesn't actually affect method inheritance under
the
standard or C3 MROs, so only modules that actually go
digging in ISAs
for other reasons (like anal test suites maybe) might notice
the
change.

On the other hand, one can envision future MROs more exotic
than C3
that might actually be confused by the current behavior, so
it would
be more forward-thinking to follow what the user asked for
explicitly.

-- Brandon

On 10/09/2007, Rick Delaney <rickbort.ca> wrote:
> On Sep 09 2007, Brandon Black wrote:
> > On 9/9/07, Rick Delaney <rickbort.ca> wrote:
> > > Although I see no reason why we shouldn't
modify base.pm so that it
> > > pushes all the classes in one statement at
the end of import().
> > >
> >
> > I tried to do this once without changing the
behavior of base.pm, and
> > I kinda gave up after a while.  It can be done,
but it will cause some
> > subtle changes that I'm sure would trip up someone
relying on "use
> > base".
>
> Wouldn't the attached patch be sufficient?  It will not
have any of the
> problems you mentioned.

Thanks, applied as #31851.