Created attachment 4167 [details]
sample mail

> Seems like the trunk code is even slower than 3.2.3(4), don't know why.

Actually it isn't slower, it is the same speed in 3.2.3 and in trunk.
I just happened not to have SARE rules loaded in 3.2.3
(after switching versions back and forth), which it made it
about twice as fast.

trunk (without additional rulesets) seems reasonably fast:

: jm 103...; time ./spamassassin -Lt < /home/jm/DL/1.msg  > o

real    0m6.988s
user    0m4.908s
sys     0m0.240s

I think it's a SARE bug.  do they have a bug tracker?

Created attachment 4168 [details]
gzipped small test perl program, simulating the case

Depends on what one considers a bug. I'd say it is a SpamAssassin bug.

Attached is a small perl program which defines an array of text chunks
from the very same sample mail in question, and runs them through
two of the most time-consuming rules as reported in the timing report.
The string encoding and its utf8 flag is the same as it really happens
in the real command-line spamassassin run, eval'ed under compiled body tests.

What is amazing is that if utf8 flag in the text(!) is turned off
it runs 30 times faster!

  bytes t = 0.120686 s
  chars t = 3.617508 s

(btw, I gzipped the test program to make sure the 8-bit characters
will remain intact)

(In reply to comment #4)
> Depends on what one considers a bug. I'd say it is a SpamAssassin bug.

how would you propose to fix it in SA?  by re-adding "use bytes"?

interestingly it seems to work a bit better with blead:

: jm 157...; perl -T 0.lis
bytes t = 0.753940 s
chars t = 4.609974 s
: jm 158...; bleadperl -T 0.lis
bytes t = 0.062525 s
chars t = 1.208566 s

Created attachment 4169 [details]
proposed patch

> how would you propose to fix it in SA?  by re-adding "use bytes"?

No. By using a supported method that actually works,
instead of utf8::downgrade, which according to Encode man page:

  NOTE: this function is experimental and may change
	or be removed without notice.

and which turns out doesn't do its job.  See attached patch.

Created attachment 4170 [details]
actually, this one

Created attachment 4173 [details]
same patch but without a "use Encode"

The Encode is already loaded by Conf.pm and need not be
loaded here - avoids having to go through the same decisions
on loading / not loading it, as already handled in Conf
in order to still be able to run code under Perl 5.6

I wonder if perhaps this bug should be closed and the Bug 4636
reopened, as it doesn't do its job well.

I'm not sure I know what is the true purpose of utf8::downgrade
in r366594 (comment no.12 in Bug 4636). If the intention was to
clear the utf8 flag for speed, it doesn't work this way, as the
utf8::downgrade aborts its operation leaving the utf8 flag on
(with or without throwing a signal, depending on its second arg)
if a string contains a character that does not have a representation
in the current locale charset.

My proposed patch just makes sure that a conversion is made
from characters to utf-8 octets, this making sure the regexps
in rules need not deal with 16-bit characters, which makes them
terribly slow.

If the intention of the original utf8::downgrade call was to really
convert to a locale character set, it needs to be documented and
probably the target charset needs to be configurable. Also not to forget
that utf8::downgrade is unsupported/experimental in Perl and needs
to be replaced with something else anyway.

Reopening Bug 4636 or keeping this topic here?

(In reply to comment #9)
> I wonder if perhaps this bug should be closed and the Bug 4636
> reopened, as it doesn't do its job well.
> 
> I'm not sure I know what is the true purpose of utf8::downgrade
> in r366594 (comment no.12 in Bug 4636). If the intention was to
> clear the utf8 flag for speed, it doesn't work this way, as the
> utf8::downgrade aborts its operation leaving the utf8 flag on
> (with or without throwing a signal, depending on its second arg)
> if a string contains a character that does not have a representation
> in the current locale charset

aha.  that's not good. :(


> My proposed patch just makes sure that a conversion is made
> from characters to utf-8 octets, this making sure the regexps
> in rules need not deal with 16-bit characters, which makes them
> terribly slow.
> 
> If the intention of the original utf8::downgrade call was to really
> convert to a locale character set, it needs to be documented and
> probably the target charset needs to be configurable. Also not to forget
> that utf8::downgrade is unsupported/experimental in Perl and needs
> to be replaced with something else anyway.
> 
> Reopening Bug 4636 or keeping this topic here?

I would suggest 

(a) retitling this bug to be more specific about utf8::downgrade
causing the issue

and (b) and commenting on bug 4636, indicating that there's a
bug in that code, and pointing at this.

That assumes that, if we replace the utf8::downgrade usage, we
have fixed this bug in its entirety -- is that the case?  If not,
then the utf8::downgrade bug should be opened as a new issue.

> That assumes that, if we replace the utf8::downgrade usage, we
> have fixed this bug in its entirety -- is that the case?  If not,
> then the utf8::downgrade bug should be opened as a new issue.

Yes, it is true - as far as this bug is concerned (i.e. the utf8 flag
remains enabled in certain text as passed to rules when normalize_charset
is on), the proposed patch does fix it.

The proposed patch is incorrect and this bug is INVALID.

The purpose of the utf8::downgrade call is to get the speed benefits of having
the utf8 flag cleared when it is possible to represent the characters without
using the utf8 flag.  In this particular case, it is not possible to represent
the characters without having the utf8 flag set, so the call leaves the utf8
flag set as it is intended to do.

Many rules are not charset-normalization-aware and thus may perform poorly or
incorrectly with charset normalization enabled.  For example, I have seen rules
test for non-ASCII by using [\x80-\xff].  With charset normalization, they need
to instead use [^\x00-\x7f].  Similarly, rules might need to use [0-9] instead
of \d.  Similarly \s and \w might catch more characters than intended.

I'm not sure that closing this bug was the appropriate action, certainly not as 
INVALID.  The original complaint that some rules are abnormally slow still 
seems to be a quite valid complaint.

It should also be noted that these rules have existed for years, and were NOT 
slow until recently.  That is because previously the module contained a 'use 
bytes' directive, which is no longer present.

Presumably the use bytes was forcing byte encoding rather than utf-8 encoding 
on strings, whether they could be validly represented as bytes or not.  And the 
rules in question (and many, probably most other existing rules) were tested 
under this regieme and found to not FP excessively.

Did normalize_charset get turned on by default recently?  With normalize_charset
disabled, the code in question won't be run and nothing will ever get the utf8
flag set.

With normalize_charset enabled, one has to pay the associated cost and be
careful about the set of rules used.

The old "use bytes" did nothing useful as nothing ever had the utf8 flag set.

The body pattern test doesn't match even if the body pattern rule written in
Japanese, which encoding is UTF-8, is used. 

However, the body pattern test matches when rewriting to use Encode. 


The following attention is written in 'perldoc utf8'
       * $success = utf8::downgrade($string[, FAIL_OK])
           
               snip...

           Note that this function does not handle arbitrary encodings.
           Therefore Encode.pm is recommended for the general purposes.

           Not affected by the encoding pragma.

           NOTE: this function is experimental and may change or be removed
           without notice.

(In reply to comment #15)
> The body pattern test doesn't match even if the body pattern rule written in
> Japanese, which encoding is UTF-8, is used. 

SA rules files are encoded in ISO-8859-1, not UTF-8.  You have to either encode
Japanese characters in pattern tests using \x sequences or develop a new feature
adding support for UTF-8 config files to SA.

> The following attention is written in 'perldoc utf8'
>        * $success = utf8::downgrade($string[, FAIL_OK])
>            
>                snip...
> 
>            Note that this function does not handle arbitrary encodings.
>            Therefore Encode.pm is recommended for the general purposes.
> 
>            Not affected by the encoding pragma.
> 
>            NOTE: this function is experimental and may change or be removed
>            without notice.

The "NOTE" has been there for years and is most likely out of date.  The only
alternatives to using that function are to reimplement the same semantics some
other way or to impose the performance penalty in cases where it is not necessary.

FYI, the UTF-8 parsing inefficiencies were noted and discussed (with no real results) about five months ago, see the open ticket at 
http://rt.perl.org/rt3//Public/Bug/Display.html?id=66852