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 overheadSo, correct output is #2 and lack of memory leaks at mere #3...? ;-) > 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. 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? ;-) > 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? ;-) [...] >>> + 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. -- Radosław Zieliński <[EMAIL PROTECTED]>
pgpaxIIis3m57.pgp
Description: PGP signature
