Slowdown in split + list assign

p5pRT commented 8 years ago

Migrated from rt.perl.org#127999 (status was 'resolved')

Searchable as RT127999$

p5pRT commented 8 years ago

From sschoeling@linet-services.de

Created by sschoeling@linet-services.de

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 variations with the first line taken out of the Benchmark sub:

my $s = "1"x100; my @a = ((split //\, $s)\, "-"); # same problem

my $s = "1"x100; my @a = ("-"\, split //\, $s); # same problem

my @b = split //\, "1"x100; my @a = ("-"\, @b) # OK

my @b = split //\, "1"x100; my @a = (@b\, "-") # OK

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 aware of.

Perl Info

``` Flags: category=core severity=low Site configuration information for perl 5.24.0: Configured by sschoeling at Thu Apr 14 11:29:43 CEST 2016. Summary of my perl5 (revision 5 version 24 subversion 0) configuration: Commit id: b31d1ef1144adb563cad5228eb7a7a4866b19a50 Platform: osname=linux, osvers=3.13.0-83-generic, archname=x86_64-linux uname='linux plum-chiew 3.13.0-83-generic #127-ubuntu smp fri mar 11 00:25:37 utc 2016 x86_64 x86_64 x86_64 gnulinux ' config_args='' hint=recommended, useposix=true, d_sigaction=define useithreads=undef, usemultiplicity=undef use64bitint=define, use64bitall=define, uselongdouble=undef usemymalloc=n, bincompat5005=undef Compiler: cc='cc', ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64', optimize='-O2', cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include' ccversion='', gccversion='4.8.4', gccosandvers='' intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678, doublekind=3 d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16, longdblkind=3 ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8 alignbytes=8, prototype=define Linker and Libraries: ld='cc', ldflags =' -fstack-protector -L/usr/local/lib' libpth=/usr/local/lib /usr/lib/gcc/x86_64-linux-gnu/4.8/include-fixed /usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib libs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc libc=libc-2.19.so, so=so, useshrplib=false, libperl=libperl.a gnulibc_version='2.19' Dynamic Linking: dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E' cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector' Locally applied patches: RC1 @INC for perl 5.24.0: /home/sschoeling/lib/perl5/site_perl/5.24.0/x86_64-linux /home/sschoeling/lib/perl5/site_perl/5.24.0 /home/sschoeling/lib/perl5/5.24.0/x86_64-linux /home/sschoeling/lib/perl5/5.24.0 /home/sschoeling/lib/perl5/site_perl/5.22.0 /home/sschoeling/lib/perl5/site_perl/5.21.11 /home/sschoeling/lib/perl5/site_perl/5.19.11 /home/sschoeling/lib/perl5/site_perl/5.18.0 /home/sschoeling/lib/perl5/site_perl/5.16.0 /home/sschoeling/lib/perl5/site_perl/5.15.7 /home/sschoeling/lib/perl5/site_perl/5.12.2 /home/sschoeling/lib/perl5/site_perl . Environment for perl 5.24.0: HOME=/home/sschoeling LANG=en_US.UTF-8 LANGUAGE=en_US LC_ADDRESS=de_DE.UTF-8 LC_IDENTIFICATION=de_DE.UTF-8 LC_MEASUREMENT=de_DE.UTF-8 LC_MONETARY=de_DE.UTF-8 LC_NAME=de_DE.UTF-8 LC_NUMERIC=de_DE.UTF-8 LC_PAPER=de_DE.UTF-8 LC_TELEPHONE=de_DE.UTF-8 LC_TIME=de_DE.UTF-8 LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=/opt/sschoeling/perlbrew/bin:/home/sschoeling/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games PERLBREW_BASHRC_VERSION=0.66 PERLBREW_HOME=/home/sschoeling/.perlbrew PERLBREW_ROOT=/opt/sschoeling/perlbrew PERL_BADLANG (unset) SHELL=/bin/bash ```

p5pRT commented 8 years ago

From @jkeenan

On Tue Apr 26 10:45:21 2016\, sschoeling@linet-services.de wrote:

This is a bug report for perl from sschoeling@linet-services.de\, generated with the help of perlbug 1.40 running under perl 5.24.0.

----------------------------------------------------------------- [Please describe your issue here]

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 variations with the first line taken out of the Benchmark sub:

my $s = "1"x100; my @a = ((split //\, $s)\, "-"); # same problem

my $s = "1"x100; my @a = ("-"\, split //\, $s); # same problem

my @b = split //\, "1"x100; my @a = ("-"\, @b) # OK

my @b = split //\, "1"x100; my @a = (@b\, "-") # OK

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 aware of.

Here's what I got:

##### perl-5.22.0

$ perl -v This is perl 5\, version 22\, subversion 0 (v5.22.0) built for x86_64-linux

$ perl -MBenchmark -e 'timethis(1_000_000\, sub {my @a = ((split //\, "1"x100)\, "-");});' timethis 1000000: 11 wallclock secs (10.42 usr + 0.00 sys = 10.42 CPU) @ 95969.29/s (n=1000000)

$ perl -MBenchmark -e 'timethis(1_000_000\, sub {my @a = ("-"\, (split //\, "1"x100));});' timethis 1000000: 11 wallclock secs (10.50 usr + 0.00 sys = 10.50 CPU) @ 95238.10/s (n=1000000)

$ perl -MBenchmark -e 'timethis(1_000_000\, sub {my @b = split //\, "1"x100; my @a = (@b\, "-");});' timethis 1000000: 14 wallclock secs (13.51 usr + 0.00 sys = 13.51 CPU) @ 74019.25/s (n=1000000)

$ perl -MBenchmark -e 'timethis(1_000_000\, sub {my @b = split //\, "1"x100; my @a = ("-"\, @b);});' timethis 1000000: 15 wallclock secs (15.15 usr + 0.00 sys = 15.15 CPU) @ 66006.60/s (n=1000000)

#####

# blead

$ ./perl -v This is perl 5\, version 24\, subversion 0 (v5.24.0-RC2-4-g9baa246) built for x86_64-linux (with 1 registered patch\, see perl -V for more detail)

$ ./perl -Ilib -MBenchmark -e 'timethis(1_000_000\, sub {my @a = ((split //\, "1"x100)\, "-");});' timethis 1000000: 15 wallclock secs (14.32 usr + 0.00 sys = 14.32 CPU) @ 69832.40/s (n=1000000)

$ ./perl -Ilib -MBenchmark -e 'timethis(1_000_000\, sub {my @a = ("-"\, (split //\, "1"x100));});' timethis 1000000: 14 wallclock secs (14.38 usr + 0.00 sys = 14.38 CPU) @ 69541.03/s (n=1000000)

$ ./perl -Ilib -MBenchmark -e 'timethis(1_000_000\, sub {my @b = split //\, "1"x100; my @a = (@b\, "-");});' timethis 1000000: 14 wallclock secs (14.18 usr + 0.00 sys = 14.18 CPU) @ 70521.86/s (n=1000000)

$ ./perl -Ilib -MBenchmark -e 'timethis(1_000_000\, sub {my @b = split //\, "1"x100; my @a = ("-"\, @b);});' timethis 1000000: 13 wallclock secs (13.92 usr + 0.00 sys = 13.92 CPU) @ 71839.08/s (n=1000000) #####

[Please do not change anything below this line] ----------------------------------------------------------------- --- Flags: category=core severity=low --- Site configuration information for perl 5.24.0:

Configured by sschoeling at Thu Apr 14 11:29:43 CEST 2016.

Summary of my perl5 (revision 5 version 24 subversion 0) configuration: Commit id: b31d1ef1144adb563cad5228eb7a7a4866b19a50 Platform: osname=linux\, osvers=3.13.0-83-generic\, archname=x86_64-linux uname='linux plum-chiew 3.13.0-83-generic #127-ubuntu smp fri mar 11 00:25:37 utc 2016 x86_64 x86_64 x86_64 gnulinux ' config_args='' hint=recommended\, useposix=true\, d_sigaction=define useithreads=undef\, usemultiplicity=undef use64bitint=define\, use64bitall=define\, uselongdouble=undef usemymalloc=n\, bincompat5005=undef Compiler: cc='cc'\, ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack- protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'\, optimize='-O2'\, cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include' ccversion=''\, gccversion='4.8.4'\, gccosandvers='' intsize=4\, longsize=8\, ptrsize=8\, doublesize=8\, byteorder=12345678\, doublekind=3 d_longlong=define\, longlongsize=8\, d_longdbl=define\, longdblsize=16\, longdblkind=3 ivtype='long'\, ivsize=8\, nvtype='double'\, nvsize=8\, Off_t='off_t'\, lseeksize=8 alignbytes=8\, prototype=define Linker and Libraries: ld='cc'\, ldflags =' -fstack-protector -L/usr/local/lib' libpth=/usr/local/lib /usr/lib/gcc/x86_64-linux-gnu/4.8/include- fixed /usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib libs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc libc=libc-2.19.so\, so=so\, useshrplib=false\, libperl=libperl.a gnulibc_version='2.19' Dynamic Linking: dlsrc=dl_dlopen.xs\, dlext=so\, d_dlsymun=undef\, ccdlflags='-Wl\,-E' cccdlflags='-fPIC'\, lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector'

Locally applied patches: RC1

--- @INC for perl 5.24.0: /home/sschoeling/lib/perl5/site_perl/5.24.0/x86_64-linux /home/sschoeling/lib/perl5/site_perl/5.24.0 /home/sschoeling/lib/perl5/5.24.0/x86_64-linux /home/sschoeling/lib/perl5/5.24.0 /home/sschoeling/lib/perl5/site_perl/5.22.0 /home/sschoeling/lib/perl5/site_perl/5.21.11 /home/sschoeling/lib/perl5/site_perl/5.19.11 /home/sschoeling/lib/perl5/site_perl/5.18.0 /home/sschoeling/lib/perl5/site_perl/5.16.0 /home/sschoeling/lib/perl5/site_perl/5.15.7 /home/sschoeling/lib/perl5/site_perl/5.12.2 /home/sschoeling/lib/perl5/site_perl .

--- Environment for perl 5.24.0: HOME=/home/sschoeling LANG=en_US.UTF-8 LANGUAGE=en_US LC_ADDRESS=de_DE.UTF-8 LC_IDENTIFICATION=de_DE.UTF-8 LC_MEASUREMENT=de_DE.UTF-8 LC_MONETARY=de_DE.UTF-8 LC_NAME=de_DE.UTF-8 LC_NUMERIC=de_DE.UTF-8 LC_PAPER=de_DE.UTF-8 LC_TELEPHONE=de_DE.UTF-8 LC_TIME=de_DE.UTF-8 LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=/opt/sschoeling/perlbrew/bin:/home/sschoeling/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games PERLBREW_BASHRC_VERSION=0.66 PERLBREW_HOME=/home/sschoeling/.perlbrew PERLBREW_ROOT=/opt/sschoeling/perlbrew PERL_BADLANG (unset) SHELL=/bin/bash

-- James E Keenan (jkeenan@cpan.org)

p5pRT commented 8 years ago

The RT System itself - Status changed from 'new' to 'open'

p5pRT commented 8 years ago

From @iabyn

On Tue\, Apr 26\, 2016 at 10:45:22AM -0700\, via RT wrote:

The following snippet loses about 20% speed from 5.22.0 to 5.24.0RC1:

my @a = ((split //\, "1"x100)\, "-");

It bisects to my commit:

commit a5f48505593c7e1ca478de383e24d5cc2541f3ca Author: David Mitchell \davem@iabyn\.com Date: Thu Aug 13 10:32:42 2015 +0100

re-implement OPpASSIGN_COMMON mechanism
This commit almost completely replaces the current mechanism for detecting and handing common vars in list assignment\, e.g.
($a\,$b) = ($b\,$a);

That commit makes the compile-time detection of possible commonality on the two sides of a list assignment have more false positives\, while eliminating more false negatives. On the converse side it makes handling a positive more efficient at runtime.

In this case\, it's now detecting a false positive at compile-time\, being unsure whether split can return an element of @a. This is indirectly because pushre is flagged as a "dangerous op". It's on my list of things to do to revise the whole concept of dangerous ops (d flag in regen/opcodes) and probably replace it with something indicating whether it can return general SVs that might appear elsewhere\, as opposed to padtmps and SvTEMPs\, and whether it can return args passed to it 9and only examine its childen in that case)\,

In an case\, I don't think it needs fixing for 5.24.0

-- The Enterprise is involved in a bizarre time-warp experience which is in some way unconnected with the Late 20th Century. -- Things That Never Happen in "Star Trek" #14

p5pRT commented 8 years ago

From @iabyn

On Thu\, Apr 28\, 2016 at 11:15:47PM +0100\, Dave Mitchell wrote:

On Tue\, Apr 26\, 2016 at 10:45:22AM -0700\, via RT wrote:

The following snippet loses about 20% speed from 5.22.0 to 5.24.0RC1:

my @a = ((split //\, "1"x100)\, "-");

It bisects to my commit:

I forgot to mention: this slowdown only happens when the assign includes something in addition to the split on the RHS\, e.g. the "-" above. Otherwise split optimises away the assign op and splits directly to @a.

-- You live and learn (although usually you just live).

p5pRT commented 8 years ago

From @cpansprout

On Thu Apr 28 15:16:33 2016\, davem wrote:

On Tue\, Apr 26\, 2016 at 10:45:22AM -0700\, via RT wrote:

The following snippet loses about 20% speed from 5.22.0 to 5.24.0RC1:

my @a = ((split //\, "1"x100)\, "-");

It bisects to my commit:
commit a5f48505593c7e1ca478de383e24d5cc2541f3ca
Author&#8203;: David Mitchell \<davem@&#8203;iabyn\.com>
Date&#8203;:   Thu Aug 13 10&#8203;:32&#8203;:42 2015 \+0100

re\-implement OPpASSIGN\_COMMON mechanism

This commit almost completely replaces the current mechanism
for detecting and handing common vars in list assignment\, e\.g\.

    $$a\,$b$ = $$b\,$a$;
That commit makes the compile-time detection of possible commonality on the two sides of a list assignment have more false positives\, while eliminating more false negatives. On the converse side it makes handling a positive more efficient at runtime.

In this case\, it's now detecting a false positive at compile-time\, being unsure whether split can return an element of @a. This is indirectly because pushre is flagged as a "dangerous op". It's on my list of things to do to revise the whole concept of dangerous ops (d flag in regen/opcodes) and probably replace it with something indicating whether it can return general SVs that might appear elsewhere\, as opposed to padtmps and SvTEMPs\, and whether it can return args passed to it 9and only examine its childen in that case)\,

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 dangerous - ops which may return their arguments very dangerous - ops which could return anything (like entersub\, each\, 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’.

In an case\, I don't think it needs fixing for 5.24.0

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

p5pRT commented 8 years ago

From @cpansprout

On Fri Apr 29 07:53:47 2016\, sprout wrote:

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)?

Attached is a patch\, which is also smoking on the smoke-me/127999 branch.

--

Father Chrysostomos

p5pRT commented 8 years ago

From @cpansprout

From: Father Chrysostomos \sprout@cpan\.org

Remove OA_DANGEROUS from pushre

The OA_DANGEROUS flag indicates that scalars may need temporary copies in list assignment\, in case the scalar is on both sides of the assignment. pushre\, which is only used in conjunction with split\, never affects what SVs split returns\, so the flag on this op type is bogus\, and probably has always been so.

Due to a change in 5.23.x to the algorithm that determines whether temp copies are needed in list assignment\, split is now triggering a false positive\, due to the flag on pushre\, resulting in a noticeable slowdown. So we need the flag gone.

Inline Patch

```diff diff --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. ```

p5pRT commented 8 years ago

From @cpansprout

On Sun May 01 05:53:20 2016\, sprout wrote:

On Fri Apr 29 07:53:47 2016\, sprout wrote:

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)?

Attached is a patch\, which is also smoking on the smoke-me/127999 branch.

BTW\, I would appreciate it if someone else could benchmark this patch\, since my benchmarking skills are not the best.

--

Father Chrysostomos

p5pRT commented 8 years ago

From @rjbs

* Father Chrysostomos via RT \perlbug\-followup@perl\.org [2016-05-01T08:53:20]

Attached is a patch\, which is also smoking on the smoke-me/127999 branch.

I will apply this patch and cut an RC4 *immediately* if I can get two other committers to give it a +1.

-- rjbs

p5pRT commented 8 years ago

From @jkeenan

On Sun May 01 15:19:23 2016\, perl.p5p@rjbs.manxome.org wrote:

* Father Chrysostomos via RT \perlbug\-followup@perl\.org [2016-05- 01T08:53:20]

Attached is a patch\, which is also smoking on the smoke-me/127999 branch.

I will apply this patch and cut an RC4 *immediately* if I can get two other committers to give it a +1.

-1

Here are the results I got running the same benchmarks as I did previously on the same machine.

##### # Father C patch

$ ./perl -v This is perl 5\, version 24\, subversion 0 (v5.24.0-RC3-7-g9cff96d) built for x86_64-linux (with 1 registered patch\, see perl -V for more detail)

$ ./perl -Ilib -MBenchmark -e 'timethis(1_000_000\, sub {my @a = ((split //\, "1"x100)\, "-");});' timethis 1000000: 14 wallclock secs (14.31 usr + 0.00 sys = 14.31 CPU) @ 69881.20/s (n=1000000)

$ ./perl -Ilib -MBenchmark -e 'timethis(1_000_000\, sub {my @a = ("-"\, (split //\, "1"x100));});' timethis 1000000: 14 wallclock secs (14.32 usr + 0.00 sys = 14.32 CPU) @ 69832.40/s (n=1000000)

$ ./perl -Ilib -MBenchmark -e 'timethis(1_000_000\, sub {my @b = split //\, "1"x100; my @a = (@b\, "-");});' timethis 1000000: 14 wallclock secs (13.95 usr + 0.00 sys = 13.95 CPU) @ 71684.59/s (n=1000000)

$ ./perl -Ilib -MBenchmark -e 'timethis(1_000_000\, sub {my @b = split //\, "1"x100; my @a = ("-"\, @b);});' timethis 1000000: 12 wallclock secs (13.89 usr + 0.00 sys = 13.89 CPU) @ 71994.24/s (n=1000000) #####

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.

-- James E Keenan (jkeenan@cpan.org)

p5pRT commented 8 years ago

From @iabyn

On Sun\, May 01\, 2016 at 06:18:48PM -0400\, Ricardo Signes wrote:

* Father Chrysostomos via RT \perlbug\-followup@perl\.org [2016-05-01T08:53:20]

Attached is a patch\, which is also smoking on the smoke-me/127999 branch.

I will apply this patch and cut an RC4 *immediately* if I can get two other committers to give it a +1.

a -1 from me :-(.

With the patch\, the assign op still gets the OPpASSIGN_COMMON_AGG flag set:

$ ./perl -Ilib -MO=Concise\,-exec -e'my @a; @a = ((split //\, $s)\, "-")' | grep aassign -e syntax OK d \<2> aassign[t3] vKS/COM_AGG

so it still gets the slowdown.

I think we should leave it as-is for 5.24\, then re-address the whole OA_DANGEROUS issue for 5.26.

Bear in mind that this only affects the specific case of a split *plus other elements* being assigned to an array. These won't be slowed down:

@a = split(/.../\, $s); foo(split(/.../\, $s)\, '-');

I think trying to tweak it at this late stage it just likely to introduce new common-assign bugs.

-- Nothing ventured\, nothing lost.

p5pRT commented 8 years ago

From s.schoeling@linet-services.de

But it *is* a regression that someone encountered presumably in real code when testing a release candidate.

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.

-- Sven Schöling

p5pRT commented 8 years ago

From @cpansprout

On Mon May 02 01:22:23 2016\, davem wrote:

With the patch\, the assign op still gets the OPpASSIGN_COMMON_AGG flag set:

$ ./perl -Ilib -MO=Concise\,-exec -e'my @a; @a = ((split //\, $s)\, "-")' | grep aassign -e syntax OK d \<2> aassign[t3] vKS/COM_AGG

so it still gets the slowdown.

I think we should leave it as-is for 5.24\, then re-address the whole OA_DANGEROUS issue for 5.26.

Bear in mind that this only affects the specific case of a split *plus other elements* being assigned to an array. These won't be slowed down:

@a = split(/.../\, $s); foo(split(/.../\, $s)\, '-');

I think trying to tweak it at this late stage it just likely to introduce new common-assign bugs.

I agree. Sorry for the last-minute panic. :-)

--

Father Chrysostomos

p5pRT commented 8 years ago

From @toddr

On Mon May 02 01:22:23 2016\, davem wrote:

On Sun\, May 01\, 2016 at 06:18:48PM -0400\, Ricardo Signes wrote:

* Father Chrysostomos via RT \perlbug\-followup@perl\.org [2016-05- 01T08:53:20]

Attached is a patch\, which is also smoking on the smoke-me/127999 branch.

I will apply this patch and cut an RC4 *immediately* if I can get two other committers to give it a +1.

a -1 from me :-(.

With the patch\, the assign op still gets the OPpASSIGN_COMMON_AGG flag set:

$ ./perl -Ilib -MO=Concise\,-exec -e'my @a; @a = ((split //\, $s)\, "-")' | grep aassign -e syntax OK d \<2> aassign[t3] vKS/COM_AGG

so it still gets the slowdown.

I think we should leave it as-is for 5.24\, then re-address the whole OA_DANGEROUS issue for 5.26.

Bear in mind that this only affects the specific case of a split *plus other elements* being assigned to an array. These won't be slowed down:

@a = split(/.../\, $s); foo(split(/.../\, $s)\, '-');

I think trying to tweak it at this late stage it just likely to introduce new common-assign bugs.

Bump. Is this still on the list to fix for the current blead?

p5pRT commented 8 years ago

From @iabyn

On Mon\, Sep 12\, 2016 at 04:38:27PM -0700\, Todd Rinaldo via RT wrote:

On Mon May 02 01:22:23 2016\, davem wrote:

On Sun\, May 01\, 2016 at 06:18:48PM -0400\, Ricardo Signes wrote:

* Father Chrysostomos via RT \perlbug\-followup@perl\.org [2016-05- 01T08:53:20]

Attached is a patch\, which is also smoking on the smoke-me/127999 branch.

I will apply this patch and cut an RC4 *immediately* if I can get two other committers to give it a +1.

a -1 from me :-(.

With the patch\, the assign op still gets the OPpASSIGN_COMMON_AGG flag set:

$ ./perl -Ilib -MO=Concise\,-exec -e'my @a; @a = ((split //\, $s)\, "-")' | grep aassign -e syntax OK d \<2> aassign[t3] vKS/COM_AGG

so it still gets the slowdown.

I think we should leave it as-is for 5.24\, then re-address the whole OA_DANGEROUS issue for 5.26.

Bear in mind that this only affects the specific case of a split *plus other elements* being assigned to an array. These won't be slowed down:

@a = split(/.../\, $s); foo(split(/.../\, $s)\, '-');

I think trying to tweak it at this late stage it just likely to introduce new common-assign bugs.

Bump. Is this still on the list to fix for the current blead?

I intend to revisit it soon.

-- The warp engines start playing up a bit\, but seem to sort themselves out after a while without any intervention from boy genius Wesley Crusher. -- Things That Never Happen in "Star Trek" #17

p5pRT commented 8 years ago

From @iabyn

On Fri\, Sep 16\, 2016 at 11:14:38PM +0100\, Dave Mitchell wrote:

I intend to revisit it soon.

I've now revisited it\, with the branch smoke-me/davem/aassign currently being smoked. It turned out that the slowdown in split between 5.22.0 and 5.24.0 was (mostly) not in fact due to the OPpASSIGN_COMMON_AGG flag being incorrectly set on the AASSIGN op.

In fact\, it's due to a bug fix in 5.24.0. Formerly\, the AASSIGN op would steal the string buffer of RHS temporaries when copying the values. This failed when the same temporary appeared twice on the RHS\, as in e.g.

@a = (split())[0\,0]

where split (and other such functions return temporaries). 5.24.0 started using the SV_NOSTEAL flag for the copy\, which give the correct behaviour\, but slowed down many list assignments where the RHS contained strings.

In the branch above I've completely reworked the slurpy part of pp_aaasign()\, with the result that many lists assigns are now much\, much faster (not just than 5.24.0\, but much faster than 5.22.0 too).

Benchmarking the two examples in this ticket\, using 'perf stat -r 10' to count CPU cycles:

for my $i (1..300_000) { my @a = ((split //\, "1"x100)\, "-"); }

5.22.0 11\,075\,364\,115 CPU cycles 5.24.0 15\,821\,609\,317 5.25.5 15\,648\,800\,025 my branch 7\,641\,656\,944

@array_of_strings = qw(stnstntsn stnstnstnstns stnstnstnstn snstnstnstn ccvdvdvdvzddvd); for my $i (1..300_000) { @array = map { split(//\, $_)\, 'separator' } @array_of_strings; }

5.22.0 12\,935\,451\,207 5.24.0 11\,706\,821\,273 5.25.5 11\,233\,362\,837 my branch 6\,091\,583\,255

And here are some further performance comments taken from the commit message:

Here are the average expr::aassign:: benchmarks for selected perls (raw numbers - lower is better)
5.6.1 5.22.0 5.24.0 5.25.5 this ------ ------ ------ ------ ------ Ir 1355.9 1497.8 1387.0 1382.0 1146.6 Dr 417.2 454.2 410.1 411.1 335.2 Dw 260.6 270.8 249.0 246.8 194.5 COND 193.5 223.2 212.0 207.7 174.4 IND 25.3 17.6 10.8 10.8 10.0
COND_m 4.1 3.1 3.1 3.7 2.8 IND_m 8.9 6.1 5.5 5.5 5.5
And this code:
my @a; for my $i (1..10_000_000) { @a = (1\,2\,3); #@a = (); }
with the empty assign is 33% faster than blead\, and without is 12% faster than blead.

-- "You're so sadly neglected\, and often ignored. A poor second to Belgium\, When going abroad." -- Monty Python\, "Finland"

p5pRT commented 8 years ago

From @demerphq

You are my hero.☺️

Thanks dave

On 21 Oct 2016 5:24 p.m.\, "Dave Mitchell" \davem@iabyn\.com wrote:

On Fri\, Sep 16\, 2016 at 11:14:38PM +0100\, Dave Mitchell wrote:

I intend to revisit it soon.

I've now revisited it\, with the branch smoke-me/davem/aassign currently being smoked. It turned out that the slowdown in split between 5.22.0 and 5.24.0 was (mostly) not in fact due to the OPpASSIGN_COMMON_AGG flag being incorrectly set on the AASSIGN op.

In fact\, it's due to a bug fix in 5.24.0. Formerly\, the AASSIGN op would steal the string buffer of RHS temporaries when copying the values. This failed when the same temporary appeared twice on the RHS\, as in e.g.
@&#8203;a = $split\($\)\[0\,0\]
where split (and other such functions return temporaries). 5.24.0 started using the SV_NOSTEAL flag for the copy\, which give the correct behaviour\, but slowed down many list assignments where the RHS contained strings.

In the branch above I've completely reworked the slurpy part of pp_aaasign()\, with the result that many lists assigns are now much\, much faster (not just than 5.24.0\, but much faster than 5.22.0 too).

Benchmarking the two examples in this ticket\, using 'perf stat -r 10' to count CPU cycles:
for my $i $1\.\.300\_000$ \{
    my @&#8203;a = $\(split //\, "1"x100$\, "\-"\);
\}

    5\.22\.0    11\,075\,364\,115 CPU cycles
    5\.24\.0    15\,821\,609\,317
    5\.25\.5    15\,648\,800\,025
    my branch  7\,641\,656\,944

@&#8203;array\_of\_strings  = qw$stnstntsn stnstnstnstns stnstnstnstn
snstnstnstn ccvdvdvdvzddvd); for my $i (1..300_000) { @array = map { split(//\, $_)\, 'separator' } @array_of_strings; }
    5\.22\.0    12\,935\,451\,207
    5\.24\.0    11\,706\,821\,273
    5\.25\.5    11\,233\,362\,837
    my branch  6\,091\,583\,255
And here are some further performance comments taken from the commit message:
Here are the average expr&#8203;::aassign&#8203;:: benchmarks for selected perls
\(raw numbers \- lower is better$

          5\.6\.1    5\.22\.0    5\.24\.0    5\.25\.5      this
         \-\-\-\-\-\-    \-\-\-\-\-\-    \-\-\-\-\-\-    \-\-\-\-\-\-    \-\-\-\-\-\-
    Ir   1355\.9    1497\.8    1387\.0    1382\.0    1146\.6
    Dr    417\.2     454\.2     410\.1     411\.1     335\.2
    Dw    260\.6     270\.8     249\.0     246\.8     194\.5
  COND    193\.5     223\.2     212\.0     207\.7     174\.4
   IND     25\.3      17\.6      10\.8      10\.8      10\.0

COND\_m      4\.1       3\.1       3\.1       3\.7       2\.8
 IND\_m      8\.9       6\.1       5\.5       5\.5       5\.5

And this code&#8203;:

    my @&#8203;a;
    for my $i $1\.\.10\_000\_000$ \{
        @&#8203;a = $1\,2\,3$;
        \#@&#8203;a = ;
    \}

with the empty assign is 33% faster than blead\, and without is 12%
faster than blead.

-- "You're so sadly neglected\, and often ignored. A poor second to Belgium\, When going abroad." -- Monty Python\, "Finland"

p5pRT commented 8 years ago

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:

On Fri\, Sep 16\, 2016 at 11:14:38PM +0100\, Dave Mitchell wrote:

I intend to revisit it soon. I've now revisited it\, with the branch smoke-me/davem/aassign currently being smoked. It turned out that the slowdown in split between 5.22.0 and 5.24.0 was (mostly) not in fact due to the OPpASSIGN_COMMON_AGG flag being incorrectly set on the AASSIGN op.

In fact\, it's due to a bug fix in 5.24.0. Formerly\, the AASSIGN op would steal the string buffer of RHS temporaries when copying the values. This failed when the same temporary appeared twice on the RHS\, as in e.g.
@&#8203;a = $split\($\)\[0\,0\]
where split (and other such functions return temporaries). 5.24.0 started using the SV_NOSTEAL flag for the copy\, which give the correct behaviour\, but slowed down many list assignments where the RHS contained strings.

In the branch above I've completely reworked the slurpy part of pp_aaasign()\, with the result that many lists assigns are now much\, much faster (not just than 5.24.0\, but much faster than 5.22.0 too).

Benchmarking the two examples in this ticket\, using 'perf stat -r 10' to count CPU cycles:
for my $i $1\.\.300\_000$ \{
    my @&#8203;a = $\(split //\, "1"x100$\, "\-"\);
\}

    5\.22\.0    11\,075\,364\,115 CPU cycles
    5\.24\.0    15\,821\,609\,317
    5\.25\.5    15\,648\,800\,025
    my branch  7\,641\,656\,944

@&#8203;array\_of\_strings  = qw$stnstntsn stnstnstnstns stnstnstnstn snstnstnstn
    ccvdvdvdvzddvd$;
for my $i $1\.\.300\_000$ \{
    @&#8203;array = map \{ split$//\, $\_$\, 'separator' \} @&#8203;array\_of\_strings;
\}

    5\.22\.0    12\,935\,451\,207
    5\.24\.0    11\,706\,821\,273
    5\.25\.5    11\,233\,362\,837
    my branch  6\,091\,583\,255
And here are some further performance comments taken from the commit message:
Here are the average expr&#8203;::aassign&#8203;:: benchmarks for selected perls
$raw numbers \- lower is better$

          5\.6\.1    5\.22\.0    5\.24\.0    5\.25\.5      this
         \-\-\-\-\-\-    \-\-\-\-\-\-    \-\-\-\-\-\-    \-\-\-\-\-\-    \-\-\-\-\-\-
    Ir   1355\.9    1497\.8    1387\.0    1382\.0    1146\.6
    Dr    417\.2     454\.2     410\.1     411\.1     335\.2
    Dw    260\.6     270\.8     249\.0     246\.8     194\.5
  COND    193\.5     223\.2     212\.0     207\.7     174\.4
   IND     25\.3      17\.6      10\.8      10\.8      10\.0

COND\_m      4\.1       3\.1       3\.1       3\.7       2\.8
 IND\_m      8\.9       6\.1       5\.5       5\.5       5\.5

And this code&#8203;:

    my @&#8203;a;
    for my $i $1\.\.10\_000\_000$ \{
        @&#8203;a = $1\,2\,3$;
        \#@&#8203;a = ;
    \}

with the empty assign is 33% faster than blead\, and without is 12% faster
than blead\.

p5pRT commented 8 years ago

@iabyn - Status changed from 'open' to 'pending release'

p5pRT commented 8 years ago

From @iabyn

On Fri\, Oct 21\, 2016 at 04:23:45PM +0100\, Dave Mitchell wrote:

On Fri\, Sep 16\, 2016 at 11:14:38PM +0100\, Dave Mitchell wrote:

I intend to revisit it soon.

I've now revisited it\, with the branch smoke-me/davem/aassign currently being smoked.

It turns out that George Greer's smoker hardware is down\, so it hasn't been smoked. But I've just pulled it into blead now with v5.25.6-78-g8b0c337.

-- More than any other time in history\, mankind faces a crossroads. One path leads to despair and utter hopelessness. The other\, to total extinction. Let us pray we have the wisdom to choose correctly. -- Woody Allen

p5pRT commented 7 years ago

From @khwilliamson

Thank 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 resolved.

Perl 5.26.0 may be downloaded via: https://metacpan.org/release/XSAWYERX/perl-5.26.0

If you find that the problem persists\, feel free to reopen this ticket.

p5pRT commented 7 years ago

@khwilliamson - Status changed from 'pending release' to 'resolved'

Perl / perl5