Radoslaw Zielinski writes:
> Justin Mason <[EMAIL PROTECTED]> [17-07-2006 15:10]:
> > Radoslaw Zielinski writes:
> >> [EMAIL PROTECTED] <[EMAIL PROTECTED]> [14-07-2006 12:34]:
> [...]
> >>> bug 4777: evalstr version of run_eval_tests, thanks Michael
> [...]
> >> Whoa, that's a hackish one. Wouldn't storing references to anonymous
> >> subs be better (maybe with help of an AUTOLOAD)? Modifying namespace
> >> at run time is sooo far from OO... And situation where foo->can("x")
> >> is true, but foo->x() throws an exception... That's just plain wrong.
> > Yep -- it would be nicer. Sadly, last time I checked, it was faster this
> > way, and that's the #1 priority for these methods. The overhead
>
> So, correct output is #2 and lack of memory leaks at mere #3...? ;-)
oh, definitely ;)
> > of dereferencing the subroutine is just enough to cause a slowdown.
>
> > For a demonstration, see "bench_subs" at
> > http://taint.org/xfer/2006/bench_subs.txt --
>
> >> jm 125...; ~/ftp/bench/bench_subs
> > Rate sub_ref evaled_sub
> > sub_ref 568181/s -- -11%
> > evaled_sub 641024/s 13% --
> > 4499991 4499991
>
> I'll take a look later; now, judging from the rates, you're comparing
> just the calling time; probably way smaller than execution time.
Yes.
> Let's assume SA is really fast (so this optimization actually does
> matter) and processes an average message in 0.1 second. Assuming (I
> don't feel like digging around there now) this code is called... well,
> let's be generous: 1000 times per message, so for the sub_ref it takes
> 1000/568181 = 0.00176s, which makes it 1.76% of time spent processing a
> message. But, since evaled_sub is 13% faster, it saves us, like,
> 0.00176 - (1000/641024) = 0.0002s! For every message! That means
> (unless I have messed something up while calculating this) you can
> process 501 messages instead of 500 in a period of time!
>
> Awesome, I like benchmarks; say, do you have some more? ;-)
Sure. But it all adds up -- right now, the biggest hotspot method
(_meta_tests_500) consumes only 2.57% of the time spent.
> > Now, these subs aren't performance-critical, so it would be possible to
> > move to code refs for *these* functions, but leave the body subs (which
> > are generated from the body ruleset and are the most speed-critical part)
> > as namespace-modifying functions. But then we'd have 2 separate methods of
> > doing the same thing and we'd still need that namespace-cleanup code; in
> > my opinion, better to just have just one kludgy method, instead of one
> > elegant *and* one kludgy one. ;)
>
> Yeah, two would be even uglier.
>
> [...]
> >>> + # Some of the rules are scoreset specific, so we need additional
> >>> + # subroutines to handle those
> >>> + if (defined &{'Mail::SpamAssassin::PerMsgStatus::'.$methodname}
> >> Prettier (IMHO):
> >> if (Mail::SpamAssassin::PerMsgStatus->can($methodname)
> > Disappointingly, that's a defined test for a reason. can() and defined()
> > don't always produce the same output! That's why there's this double-test
> > at
> > line 2901:
>
> Yes, I figured it when I saw that; just forgot to delete the comment
> about it.
>
> >>> + && !$doing_user_rules)
> >>> + {
> >>> + no strict "refs";
> >>> +
> >>> &{'Mail::SpamAssassin::PerMsgStatus::'.$methodname}($self,@extraevalargs);
> >>> + use strict "refs";
> >> No need for 'no strict':
> >> my $method = 'Mail::SpamAssassin::PerMsgStatus::' . $methodname;
> >> $self->$method(@extraevalargs);
> > Sorry -- I'd need to see more platform checks to ensure that really *is* a
> > safe change now; it was added years ago by Matt for a reason, as I recall.
>
> This syntax seems perfectly safe for me. Maybe a ,,benchmark reason'':
>
> Rate oostyle stricthack
> oostyle 277557/s -- -21%
> stricthack 349650/s 26% --
>
> How many times is it called for each message? Do we get to process one
> more per year? ;-)
Doesn't this mean that the "strict" thing is faster? ;)
> [...]
> >>> + package Mail::SpamAssassin::PerMsgStatus;
> [...]
> >> sub Mail::SpamAssassin::PerMsgStatus::$methodname {
> > Not equivalent; the latter does not have access to package-global
> > vars in Mail::SpamAssassin::PerMsgStatus scope without the "package"
> > line. see:
>
> Ah, I forgot about these.
--j.
