Slowdown in split + list assign #15296
Comments
From [email protected]Created by [email protected]The following snippet loses about 20% speed from 5.22.0 to 5.24.0RC1: my @a = ((split //, "1"x100), "-"); I can't find anything in the changes that is likely to cause this. Some my $s = "1"x100; my $s = "1"x100; my @b = split //, "1"x100; my @b = split //, "1"x100; my @a = ("-"); push @a, split //, "1"x100; # OK my @a = split //, "1"x100; push @a, "-"; # OK Feel free to close if this is an obvious consequence of something I wasn't Perl Info |
From @jkeenanOn Tue Apr 26 10:45:21 2016, sschoeling@linet-services.de wrote:
Here's what I got: ##### $ perl -v $ perl -MBenchmark -e 'timethis(1_000_000, sub {my @a = ((split //, "1"x100), "-");});' $ perl -MBenchmark -e 'timethis(1_000_000, sub {my @a = ("-", (split //, "1"x100));});' $ perl -MBenchmark -e 'timethis(1_000_000, sub {my @b = split //, "1"x100; my @a = (@b, "-");});' $ perl -MBenchmark -e 'timethis(1_000_000, sub {my @b = split //, "1"x100; my @a = ("-", @b);});' ##### # blead $ ./perl -v $ ./perl -Ilib -MBenchmark -e 'timethis(1_000_000, sub {my @a = ((split //, "1"x100), "-");});' $ ./perl -Ilib -MBenchmark -e 'timethis(1_000_000, sub {my @a = ("-", (split //, "1"x100));});' $ ./perl -Ilib -MBenchmark -e 'timethis(1_000_000, sub {my @b = split //, "1"x100; my @a = (@b, "-");});' $ ./perl -Ilib -MBenchmark -e 'timethis(1_000_000, sub {my @b = split //, "1"x100; my @a = ("-", @b);});'
-- |
|
The RT System itself - Status changed from 'new' to 'open' |
From @iabynOn Tue, Apr 26, 2016 at 10:45:22AM -0700, via RT wrote:
It bisects to my commit: commit a5f4850 re-implement OPpASSIGN_COMMON mechanism That commit makes the compile-time detection of possible commonality In this case, it's now detecting a false positive at compile-time, In an case, I don't think it needs fixing for 5.24.0 -- |
From @iabynOn Thu, Apr 28, 2016 at 11:15:47PM +0100, Dave Mitchell wrote:
I forgot to mention: this slowdown only happens when the assign includes -- |
From @cpansproutOn Thu Apr 28 15:16:33 2016, davem wrote:
That is quite close to what OA_DANGEROUS already means. (Or at least, what it meant in 5.22. I am not familiar with your new algorithm.) But I think we ideally need three settings for the flag: normal - ops which never return unsafe values I believe most ops fall under the first two categories, and that most of them are already flagged appropriately (some, such as cond_expr would have to change). So the simplest approach would be to make a static function in op.c returning a bool that tells whether a ‘dangerous’ op type is ‘very dangerous’.
But it *is* a regression that someone encountered presumably in real code when testing a release candidate. Isn’t the d flag bogus on pushre? pushre doesn’t affect what SVs get returned. Cannot the flag just be turned off for 5.24 (and forever after)? -- Father Chrysostomos |
From @cpansproutOn Fri Apr 29 07:53:47 2016, sprout wrote:
Attached is a patch, which is also smoking on the smoke-me/127999 branch. -- Father Chrysostomos |
From @cpansproutFrom: Father Chrysostomos <sprout@cpan.org> Remove OA_DANGEROUS from pushre The OA_DANGEROUS flag indicates that scalars may need temporary Due to a change in 5.23.x to the algorithm that determines Inline Patchdiff --git a/opcode.h b/opcode.h
index 5ec8f58..1cbe001 100644
--- a/opcode.h
+++ b/opcode.h
@@ -1791,7 +1791,7 @@ EXTCONST U32 PL_opargs[] = {
0x00000040, /* padav */
0x00000040, /* padhv */
0x00000040, /* padany */
- 0x00000540, /* pushre */
+ 0x00000500, /* pushre */
0x00000144, /* rv2gv */
0x00000144, /* rv2sv */
0x00000104, /* av2arylen */
diff --git a/regen/opcodes b/regen/opcodes
index 9ea0753..402fc6b 100644
--- a/regen/opcodes
+++ b/regen/opcodes
@@ -57,7 +57,7 @@ padav private array ck_null d0
padhv private hash ck_null d0
padany private value ck_null d0
-pushre push regexp ck_null d/
+pushre push regexp ck_null /
# References and stuff.
|
From @cpansproutOn Sun May 01 05:53:20 2016, sprout wrote:
BTW, I would appreciate it if someone else could benchmark this patch, since my benchmarking skills are not the best. -- Father Chrysostomos |
From @rjbs* Father Chrysostomos via RT <perlbug-followup@perl.org> [2016-05-01T08:53:20]
I will apply this patch and cut an RC4 *immediately* if I can get two other -- |
From @jkeenanOn Sun May 01 15:19:23 2016, perl.p5p@rjbs.manxome.org wrote:
-1 Here are the results I got running the same benchmarks as I did previously on the same machine. ##### $ ./perl -v $ ./perl -Ilib -MBenchmark -e 'timethis(1_000_000, sub {my @a = ((split //, "1"x100), "-");});' $ ./perl -Ilib -MBenchmark -e 'timethis(1_000_000, sub {my @a = ("-", (split //, "1"x100));});' $ ./perl -Ilib -MBenchmark -e 'timethis(1_000_000, sub {my @b = split //, "1"x100; my @a = (@b, "-");});' $ ./perl -Ilib -MBenchmark -e 'timethis(1_000_000, sub {my @b = split //, "1"x100; my @a = ("-", @b);});' These do not present a big improvement over blead and do not get us back to where we were at 5.22. The code correction here is at a depth in the code base where there are few people who know what's going on and where, based on recent years' experience, bugs are not likely to surface for, say, 4 to 6 months. Given where we are in the release cycle, I don't think it should be applied to blead. Thank you very much. -- |
From @iabynOn Sun, May 01, 2016 at 06:18:48PM -0400, Ricardo Signes wrote:
a -1 from me :-(. With the patch, the assign op still gets the OPpASSIGN_COMMON_AGG flag $ ./perl -Ilib -MO=Concise,-exec -e'my @a; @a = ((split //, $s), "-")' | grep aassign so it still gets the slowdown. I think we should leave it as-is for 5.24, then re-address the whole Bear in mind that this only affects the specific case of a split *plus @a = split(/.../, $s); I think trying to tweak it at this late stage it just likely to -- |
From [email protected]
FWIW, I encountered it in funky sudoku code while being hyped about how much faster scope entry is in 5.24. (You guys rock.) This is not production code, nor does it need fixing right away for me, I just thought you'd want to know about it. It stood out because the code heavily relies on @array = map { split(//, $_), 'separator' } @array_of_strings and this regression caused the whole program to run about 7% slower than before, even with the across the board 5.24 gains. -- |
From @cpansproutOn Mon May 02 01:22:23 2016, davem wrote:
I agree. Sorry for the last-minute panic. :-) -- Father Chrysostomos |
From @toddrOn Mon May 02 01:22:23 2016, davem wrote:
Bump. Is this still on the list to fix for the current blead? |
From @iabynOn Mon, Sep 12, 2016 at 04:38:27PM -0700, Todd Rinaldo via RT wrote:
I intend to revisit it soon. -- |
From @iabynOn Fri, Sep 16, 2016 at 11:14:38PM +0100, Dave Mitchell wrote:
I've now revisited it, with the branch smoke-me/davem/aassign currently In fact, it's due to a bug fix in 5.24.0. Formerly, the AASSIGN op would @a = (split())[0,0] where split (and other such functions return temporaries). 5.24.0 started In the branch above I've completely reworked the slurpy part of Benchmarking the two examples in this ticket, using 'perf stat -r 10' to for my $i (1..300_000) { 5.22.0 11,075,364,115 CPU cycles @array_of_strings = qw(stnstntsn stnstnstnstns stnstnstnstn snstnstnstn 5.22.0 12,935,451,207 And here are some further performance comments taken from the commit message: Here are the average expr::aassign:: benchmarks for selected perls -- |
From @demerphqYou are my hero. Thanks dave On 21 Oct 2016 5:24 p.m., "Dave Mitchell" <davem@iabyn.com> wrote:
|
From @xsawyerx[Top-posted] Fantastic! As usual, thanks for adding all the explanations around this. On 10/21/2016 05:23 PM, Dave Mitchell wrote:
|
|
@iabyn - Status changed from 'open' to 'pending release' |
From @iabynOn Fri, Oct 21, 2016 at 04:23:45PM +0100, Dave Mitchell wrote:
It turns out that George Greer's smoker hardware is down, so it hasn't -- |
From @khwilliamsonThank you for filing this report. You have helped make Perl better. With the release today of Perl 5.26.0, this and 210 other issues have been Perl 5.26.0 may be downloaded via: If you find that the problem persists, feel free to reopen this ticket. |
|
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#127999 (status was 'resolved')
Searchable as RT127999$
The text was updated successfully, but these errors were encountered: