We've been unhappy with the performance of some code, recently. I was pretty sure I knew where the problem was, but I thought I'd run NYTProf just to see how things looked. I'm running an older NYTprof, so it's not 100% clear that my SQL-level optimization is what I need to do -- but it's the right thing to do anyway. Anyway, I figured I might see something sort of interesting, but I never expected this:
Calls InclTime ExclTime Subroutine 27,908 406 406 DBI::st::execute 543,412 79 31 Carp::caller_info
...and let's not go any further. The program took almost exactly 600s to run. Of that, nearly five percent was because the program called Carp, and it called it a half million times! What?!
I won't be coy, because I'm writing this while waiting for a test suite to run and while watching House. It turns out that it was related to this line:
Calls InclTime ExclTime Subroutine 139,340 222 6 SUPER::get_all_parents
That subroutine looks like this:
sub get_all_parents {
my ($invocant, $class) = @_;
my @parents = eval { $invocant->__get_parents() };
unless ( @parents ) {
no strict 'refs';
@parents = @{ $class . '::ISA' };
}
return 'UNIVERSAL' unless @parents;
return @parents, map { get_all_parents( $_, $_ ) } @parents;
}
See how it calls $invocant->__get_parents? Well, that's great, except that
our internal ORM has an AUTOLOAD subroutine that looks like this:
sub AUTOLOAD {
my $self = $_[0];
my $class = (ref $self) || $self;
(my $method = $AUTOLOAD) =~ s/.*:://;
return if $method eq "DESTROY";
unless (blessed($self)) {
confess qq(AUTOLOAD: \$self for ->$method is not a blessed reference: )
. Dumper($self);
}
...
}
Now, to avoid hitting the database too much, we have a mixin that makes it talk
to a memcached. That mixin (like many such modules) uses SUPER.pm.
SUPER then calls __get_parents on our ORM, but hits the AUTOLOAD instead,
and since that's only supposed to work on objects, it confesses. It does this
every time we check consider using a cached copy of an object, causing us to
invoke Carp::confess a half million times.
The solution? I added this line to our ORM:
sub __get_parents { return; }
Shaved two minutes off the test case. That's about 20%.
I could patch SUPER to call $invocant->can( '__get_parents' ) before calling it in the eval, but this seems like a corner case. I'll think about it.
Re:More Defense
rjbs on 2008-11-12T12:56:49
I thought about that a little bit myself. It's definitely a corner case, but it can (demonstratedly) be a pretty serious benefit. Then again, "can" is a lying jerk and it means more overhead for all the code that would've worked just fine otherwise...
I think the best solution might just be for people to STOP USING AUTOLOAD... at least when it's not really the right (read: only) way to do something. This was clearly a gratuitous use of AUTOLOAD.
If you patch SUPER, though, I won't mind one bit!
Re:More Defense
chromatic on 2008-11-12T18:28:48
Then again, "can" is a lying jerk and it means more overhead for all the code that would've worked just fine otherwise...
People who don't override
canwhen necessary are like people who don't check the return calls of system calls. Sure, you hope things will go right, but if you want to play nicely with the rest of the system, you ought to be a good neighbor.Re:More Defense
Aristotle on 2008-11-12T19:08:48
That’s true. However, it doesn’t alter the conclusion: if you can avoid
AUTOLOAD, you should. It is much likeevalandgoto LABELin that regard.Re:More Defense
btilly on 2008-11-13T07:45:19
People who override can when they think it necessary almost always get it wrong. But we've had this discussion before.
Re:More Defense
chromatic on 2008-11-13T18:01:11
Yes, we have, and you're still wrong to suggest that people break the way Perl objects work because you don't think they're smart enough to do it right.
I think we can all agree that
AUTOLOADis overused, and often poorly.Re:More Defense
btilly on 2008-11-13T19:37:42
Considering how many competent programmers I have seen try and fail, I think the ball is in your court to demonstrate that it is both possible and likely enough to happen that it is worth insisting on.
Re:More Defense
markjugg on 2008-11-17T13:05:12
I favor adding a call to can() here.
I think the outcome would be less surprising. "__get_parents()" does not strike me as the kind of method which would intentionally be served through "AUTOLOAD", so I would say "can" is likely to return accurate result for it.