Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Slowdown in 5.10.0 regexes with atomic patterns #9751

Open
p5pRT opened this issue May 28, 2009 · 14 comments
Open

Slowdown in 5.10.0 regexes with atomic patterns #9751

p5pRT opened this issue May 28, 2009 · 14 comments

Comments

@p5pRT
Copy link

p5pRT commented May 28, 2009

Migrated from rt.perl.org#66106 (status was 'open')

Searchable as RT66106$

@p5pRT
Copy link
Author

p5pRT commented May 28, 2009

From @nwc10

Moritz Lenz mailed p5p in 479DE845.20803@​casella.verplant.org
http​://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2008-01/msg01314.html

Following up on a perlmonks thread​: https://www.perlmonks.org/?node_id=664545

Summary​: some regexps with atomic groups are much slower on 5.10.0 than
on 5.8.

Dave notes​:

+ve superlinear cache issue

@p5pRT
Copy link
Author

p5pRT commented Jun 18, 2009

From p5p@spam.wizbit.be

Bisect from the thread​: (from andreas)

----Program----

use strict;
use warnings;
my $str = "bea" x 100;
my $re = qr/(?​:be|ea|a)/;
use Benchmark qw(timeit);
use Time​::HiRes qw(time);
my $start = time;
timeit(500, sub { die if $str =~ m/(?>$re+)\d/ });
my $x = time - $start;
print $x, "\n";

----Output of .../p3BBpDI/perl-5.9.3@​27902/bin/perl----
0.653790950775146

----EOF ($?='0')----
----Output of .../pcL5RMw/perl-5.9.3@​27903/bin/perl----
23.4941339492798

----EOF ($?='0')----

Change 27903 by davem@​davem-cyril on 2006/04/19 13​:56​:07

  regmatch()​: make IFMATCH use PUSH_STACK rather than fake
recursion

@p5pRT
Copy link
Author

p5pRT commented Jun 18, 2009

p5p@spam.wizbit.be - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Jul 11, 2009

From @schwern

This is a duplicate of 58280.

@p5pRT
Copy link
Author

p5pRT commented Jul 14, 2009

From @nwc10

On Sat Jul 11 02​:04​:13 2009, schwern wrote​:

This is a duplicate of 58280.

It does seem to be related. Comparing a perl built immediately before
change 27903 with 27903, I see slowdowns from both bug's test cases.

$ /home/nick/Sandpit/snap5.9.x-perl-5.8.0-7777-g20f9f80/bin/perl5.9.4 58280
  Rate regex regex_opt eq
regex 426178/s -- -3% -79%
regex_opt 441264/s 4% -- -78%
eq 1998449/s 369% 353% --
$ /home/nick/Sandpit/snap5.9.x-perl-5.8.0-7778-gdad7902/bin/perl5.9.4 58280
  Rate regex_opt regex eq
regex_opt 450584/s -- -10% -89%
regex 503130/s 12% -- -88%
eq 4251069/s 843% 745% --

$ /home/nick/Sandpit/snap5.9.x-perl-5.8.0-7777-g20f9f80/bin/perl5.9.4 66106
0.914209127426147
$ /home/nick/Sandpit/snap5.9.x-perl-5.8.0-7778-gdad7902/bin/perl5.9.4 66106
17.3551239967346

(perls admittedly not ideal, as built with -DDEBUGGING and without any
optimisation. To repeat this, you'll need to ./Configure and fix things
up roughly like this​:

./Configure ... -Dnoextensions=IPC/SysV && perl -i -ne 'print unless
/command-line/' makefile x2p/makefile && make all

change 27093 is
http​://perl5.git.perl.org/perl.git/commit/dad790286e318c5c7f4b6ccd52b4fd512c87c763

regmatch()​: make IFMATCH use PUSH_STACK rather than fake recursion

)

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Jul 15, 2009

From @hvds

"Nicholas Clark via RT" <perlbug-followup@​perl.org> wrote​:
:On Sat Jul 11 02​:04​:13 2009, schwern wrote​:
:> This is a duplicate of 58280.
:
:It does seem to be related. Comparing a perl built immediately before
:change 27903 with 27903, I see slowdowns from both bug's test cases.
:
:
:$ /home/nick/Sandpit/snap5.9.x-perl-5.8.0-7777-g20f9f80/bin/perl5.9.4 58280
: Rate regex regex_opt eq
:regex 426178/s -- -3% -79%
:regex_opt 441264/s 4% -- -78%
:eq 1998449/s 369% 353% --
:$ /home/nick/Sandpit/snap5.9.x-perl-5.8.0-7778-gdad7902/bin/perl5.9.4 58280
: Rate regex_opt regex eq
:regex_opt 450584/s -- -10% -89%
:regex 503130/s 12% -- -88%
:eq 4251069/s 843% 745% --
:
:$ /home/nick/Sandpit/snap5.9.x-perl-5.8.0-7777-g20f9f80/bin/perl5.9.4 66106
:0.914209127426147
:$ /home/nick/Sandpit/snap5.9.x-perl-5.8.0-7778-gdad7902/bin/perl5.9.4 66106
:17.3551239967346
:
:(perls admittedly not ideal, as built with -DDEBUGGING and without any
:optimisation. To repeat this, you'll need to ./Configure and fix things
:up roughly like this​:
:
:./Configure ... -Dnoextensions=IPC/SysV && perl -i -ne 'print unless
:/command-line/' makefile x2p/makefile && make all
:
:change 27093 is
:http​://perl5.git.perl.org/perl.git/commit/dad790286e318c5c7f4b6ccd52b4fd512c87c763
:
:regmatch()​: make IFMATCH use PUSH_STACK rather than fake recursion
:
:)

I investigated this a bit, using the test cases:

   perl -Dr -we '$s="aab"x shift;die if $s=~/(?:aa|[bc])+\d/' 4
   perl -Dr -we '$s="aab"x shift;die if $s=~/(?>(?:aa|[bc])+)\d/' 4

.. and the two perls 5.8.8 and blead.[1]

The -Dr differences show that in all 4 cases we turn on superlinear caching:
whilem: Detected a super-linear match, switching on caching...
but in the slow case, we never spot a match in the cache even when we should.
I would guess the writer and reader of the cache are for some reason looking
in different places, but I have not yet determined that.

I'll look through the code to try to understand why this fails, but
I don't have any time left today so I'm sharing what I've found so far.

Hugo

[1] The artificial '[bc]' is to avoid TRIE conversion, so as to keep the
two perl versions acting as similarly as possible.[2]

[2] In this case tries are also a big lose: 'aa|b' takes 50-100% longer
than 'aa|[bc]' in all 4 cases. I'll open a bug for this - I think for 5.12
there could be value in attempting to develop heuristics to determine
whether BRANCH or TRIE would be faster, so as to avoid the conversion
where it would hurt.

@p5pRT
Copy link
Author

p5pRT commented Jul 16, 2009

From @hvds

hv@​crypt.org wrote​:
:I investigated this a bit, using the test cases​:
: perl -Dr -we '$s="aab"x shift;die if $s=/(?​:aa|[bc])+\d/' 4
: perl -Dr -we '$s="aab"x shift;die if $s=
/(?>(?​:aa|[bc])+)\d/' 4
:.. and the two perls 5.8.8 and blead.[1]
:
:The -Dr differences show that in all 4 cases we turn on superlinear caching​:
: whilem​: Detected a super-linear match, switching on caching...
:but in the slow case, we never spot a match in the cache even when we should.
:I would guess the writer and reader of the cache are for some reason looking
:in different places, but I have not yet determined that.

Ah no, it's a bit trickier than that.

I've confused myself quite a bit, so some of the below may be wrong.
But this is what I *think* I've found.

1. The superlinear cache is an optimisation specific to a single opcode,
CURLYX, which handles subpatterns of the form /A{min,max}B/.

1a. In the first program the B pattern is /\d/, while in the second it
is /<nothing>/ due to the bracketing of the (?>...). But that appears to
be a red herring.

2. Once superlinearity is detected, and the cache is enabled, cache bits
are set each time on backtracking from a failure.

3. When wrapped in the cut operator (?>...), the backtracking doesn't
happen, so failures never get to be recorded in the cache.

This is why the second program exhibits exponential behaviour in blead.

4. In 5.8.8, we supported caching of either failure or success - we
picked whichever we saw first as the type to cache. In some manner
I don't understand, the second program caches successes under 5.8.8,
and thus avoids the slowdown we get in blead.

5. That dual-mode cache was removed in patch #28571), because as far
as we could tell it was always either wrong or useless.

I don't have a clear idea how to fix this​: I feel the right answer may
be to put the cache-setting logic also into the SUCCEED stack unwinding,
but as currently written I think that would only be possible by going
with the slower DEBUG version of the unwinding code.

Dave, I think most of the current version of this code is yours - does
this make sense?

Hugo

@p5pRT
Copy link
Author

p5pRT commented Dec 16, 2009

From @obra

~dupe of 58280. Removing as a 5.12 blocker.

@toddr
Copy link
Member

toddr commented Feb 4, 2020

This continues to be the case in 5.30. Is this something still worth fixing @iabyn or @hvds ?

$>perl foo
            Rate   atomic1 posessive   atomic2    normal
atomic1    367/s        --       -0%      -96%      -96%
posessive  369/s        0%        --      -96%      -96%
atomic2   8253/s     2146%     2139%        --      -14%
normal    9635/s     2522%     2513%       17%        --

@toddr
Copy link
Member

toddr commented Feb 4, 2020

#!/usr/bin/perl
use strict;
use warnings;
use 5.010;
use Benchmark qw(cmpthese);

my $str = "bea" x 100;
my $re = qr/(?:be|ea|a)/;
cmpthese(-2, {
    atomic1   => sub { die if $str =~ m/(?>$re+)\d/ },
    atomic2   => sub { die if $str =~ m/(?>$re)+\d/ },
    normal    => sub { die if $str =~ m/$re+\d/ },
    posessive => sub { die if $str =~ m/$re++\d/ },
});
__END__

@toddr toddr added the Closable? We might be able to close this ticket, but we need to check with the reporter label Feb 4, 2020
@demerphq
Copy link
Collaborator

demerphq commented Feb 5, 2020 via email

@toddr toddr removed the Closable? We might be able to close this ticket, but we need to check with the reporter label Feb 5, 2020
@xenu xenu removed the Severity Low label Dec 29, 2021
@khwilliamson
Copy link
Contributor

I'll summarize all of the above with this quote from @demerphq from it

So, yes, there is a bug here we should figure out. Why does SUSPEND
break/disable the super-linear cache?

But ultimately the use of SUSPEND/possessive match here is simply not the
right thing to do, and doesn't help that much with an unanchored pattern
anyway.

@demerphq
Copy link
Collaborator

Although I probably should eat my words on this:

The recursive regexp engine design we use is not capable (without great effort which is unlikely to be every provided) to determine that anything that matches (be|ea|a) cannot ever match \d

It occurs me to me reading this today that if the code is TRIE'd then we can. We can't generally, in the sense that if 'be', 'ea' and 'a' are arbitrary patterns, but as written we /could/ tell that they cant match \d. Not sure how much it would help, and if the extra compile time would justify it. But in theory provided the alternation can be converted to a TRIE we could.

But it seems to me reading this now that we should be investigating the core point, why does SUSPEND disable the super linear cache?

@iabyn
Copy link
Contributor

iabyn commented Apr 17, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants