Bug 5291 - Bayes' use_ignores code is inefficient
Summary: Bayes' use_ignores code is inefficient
Status: NEW
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Libraries (show other bugs)
Version: SVN Trunk (Latest Devel Version)
Hardware: Other other
: P5 normal
Target Milestone: Undefined
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-01-10 11:10 UTC by Theo Van Dinter
Modified: 2013-03-01 00:51 UTC (History)
0 users


Attachment Type Modified Status Actions Submitter/CLA Status
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 Theo Van Dinter 2007-01-10 11:10:40 UTC 
Anytime we have to create a new PMS object just to do a simple function call and
then destroy it is, well, stupid, imo...

Bayes::learn:
  if( $self->{use_ignores} )  # Remove test when PerMsgStatus available.
  { 
    # DMK, koppel@ece.lsu.edu:  Hoping that the ultimate fix to bug 2263 will
    # make it unnecessary to construct a PerMsgStatus here.
    my $PMS = new Mail::SpamAssassin::PerMsgStatus $self->{main}, $msg;
    my $ignore = $self->ignore_message($PMS);
    $PMS->finish();
    return 0 if $ignore;
  } 


I would really like to fix this before 3.2 goes out.
Comment 1 Theo Van Dinter 2007-01-10 13:04:15 UTC 
In doing some quick digging, here's what happens in 3.2:

Bayes::learn calls Bayes::ignore_message.  ignore_message generates a new PMS
object and then calls plugins for check_wb_list.  WLBLEval is the the default
plugin that has that function.  It ends up calling check_to_in_list and
check_from_in_list in the plugin, which ends up using:

$pms->{from_in_whitelist} (set in the plugin)
$pms->all_to_addrs()
$pms->all_from_addrs()
$pms->{num_relays_untrusted} (won't this be mis-set due to incorrect pms?)
$pms->{num_relays_trusted} (ditto)
$pms->{relays_untrusted} (ditto)
$pms->{relays_trusted} (ditto)

so 1) it's horribly klugy, 2) may not work anyway due to the new pms object, and
3) is yet another reason why we really ought to break Bayes fully out into a
plugin and not have it part of the main engine code.
Comment 2 Justin Mason 2007-01-11 06:04:14 UTC 
could a single, long-lived PerMsgStatus be created and kept around for these checks?

it's still yucky though...
Comment 3 Theo Van Dinter 2007-01-11 09:17:34 UTC 
(In reply to comment #2)
> could a single, long-lived PerMsgStatus be created and kept around for these
checks?
> 
> it's still yucky though...

Yeah, it's yucky.  I was thinking that we could probably move some stuff into
the Bayes plugin (the eval rule portion is plugin-ized), and rearrange some of
the other functions so that this could all be cleaner, but in the end I think
that's going to be hackish as well, though less so than the current code.

So I'm moving this to 3.3 and we'll see about cleaning it up as part of bug 5293.
Comment 4 Justin Mason 2010-01-27 02:20:37 UTC 
moving most remaining 3.3.0 bugs to 3.3.1 milestone
Comment 5 Justin Mason 2010-01-27 03:16:24 UTC 
reassigning, too
Comment 6 Justin Mason 2010-03-23 16:33:39 UTC 
moving all open 3.3.1 bugs to 3.3.2
Comment 7 Karsten Bräckelmann 2010-03-23 17:42:44 UTC 
Moving back off of Security, which got changed by accident during the mass Target Milestone move.