Bug 7698 - Prevent loading warnings.pm every eval statement
Summary: Prevent loading warnings.pm every eval statement
Status: RESOLVED WONTFIX
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: spamassassin (show other bugs)
Version: SVN Trunk (Latest Devel Version)
Hardware: All All
: P2 normal
Target Milestone: Undefined
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-03-04 22:41 UTC by Todd Rinaldo
Modified: 2019-03-12 17:25 UTC (History)
3 users (show)


Attachment Type Modified Status Actions Submitter/CLA Status
Suggested fix. text/plain None Todd Rinaldo [HasCLA]
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Todd Rinaldo 2019-03-04 22:41:09 UTC 
Created attachment 5645 [details]
Suggested fix.

Currently, lib/Mail/SpamAssassin/Plugin/Check.pm injects 'no warnings q(uninitialized);' in every eval statement it compiles. This executes quite a bit of code just to to set the COP hints at eval compile time. If we move the no warnings statement just outside of the quoted eval call, then it will set the hints on the COP that does the quoted eval call and it will be inherited. This should help speed up the eval calls.

Proof of concept:

perl -w -E'use warnings; no warnings q(uninitialized); eval q{package bar; my $foo; print $foo}'

Patch attached to ticket.
Comment 1 Henrik Krohns 2019-03-05 07:34:06 UTC 
Did you actually benchmark this? I can not see any difference over 1% margin of error in debug timings with a massive ruleset.

Also the original "no warnings" only appear in do_head_tests, but your patch affects _all_ calls of run_generic_tests. So it doesn't seem to make much sense to commit at this time.
Comment 2 Todd Rinaldo 2019-03-05 22:02:27 UTC 
Those are fair points. I think the fix I probably want is to get rid of the need for no warnings. Do you think that would be achievable?
Comment 3 Henrik Krohns 2019-03-06 05:21:40 UTC 
Future plan is to comb through all the eval stuff, perhaps get rid of it all. There's already a big change coming though for security reasons very soon. I know the code pretty well since I was forced to learn it for a month to get a grasp for it, so I'd like to make it much more simple, but we'll see..
Comment 4 Kevin A. McGrail 2019-03-06 05:30:19 UTC 
Yeah, we are back on make test working on 3.4 and trunk and I'm trying to sneak one more feature in to roll 3.4.3
Comment 5 Todd Rinaldo 2019-03-12 17:25:05 UTC 
Closing for now.