Stories
Slash Boxes
Comments
NOTE: use Perl; is on undef hiatus. You can read content, but you can't post it. More info will be forthcoming forthcomingly.

All the Perl that's Practical to Extract and Report

use Perl Log In

Log In

[ Create a new account ]

rjbs (4671)

rjbs
  (email not shown publicly)
http://rjbs.manxome.org/
AOL IM: RicardoJBSignes (Add Buddy, Send Message)
Yahoo! ID: RicardoSignes (Add User, Send Message)

I'm a Perl coder living in Bethlehem, PA and working Philadelphia. I'm a philosopher and theologan by training, but I was shocked to learn upon my graduation that these skills don't have many associated careers. Now I write code.

Journal of rjbs (4671)

Tuesday November 11, 2008
09:33 PM

astounding optimization! thanks nytprof!

[ #37861 ]

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%.

The Fine Print: The following comments are owned by whoever posted them. We are not responsible for them in any way.
 Full
 Abbreviated
 Hidden
More | Login | Reply
Loading... please wait.
  • 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.

    • 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!

      --
      rjbs
      • 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 can when 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.

        • That’s true. However, it doesn’t alter the conclusion: if you can avoid AUTOLOAD, you should. It is much like eval and goto LABEL in that regard.

        • People who override can when they think it necessary almost always get it wrong. But we've had this discussion before.

          • 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 AUTOLOAD is overused, and often poorly.

            • 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.

    • 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.