develooper Front page | perl.perl5.porters | Postings from October 2014

Re: 5.20.1 / state bug

Thread Previous | Thread Next
From:
Aaron Crane
Date:
October 21, 2014 16:35
Subject:
Re: 5.20.1 / state bug
Message ID:
CACmk_tu6Nh=2xqRn_7aVsGK6vJm3rhq4hM88t5wjMNFvyqyENg@mail.gmail.com
Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> wrote:
> This is fixed in blead. Bisecting a simplified case:
>
> ./Porting/bisect.pl -Accflags=-DPERL_NO_COW --target=miniperl --start=v5.20.0 --expect-fail -e 'use strict; use feature "state"; sub state_test { state $s; $s = "foo"; my $c = $s; die "gone" unless defined $s } state_test()'
>
> shows that this commit fixed it:
>
> commit c0683843e9299db25f354e2c8c90faa7614950d1
> Author: Father Chrysostomos <sprout@cpan.org>
> Date:   Mon Sep 22 20:25:29 2014 -0700
>
>     Stop setting PADMY; renumber PADSTALE

Thanks for that bisection, Ilmari. The bug was apparently introduced
in this commit:

commit 9ffd39ab75dd662df22fcdafbf7f740838acc898
Author: Father Chrysostomos <sprout@cpan.org>
Date:   Wed Nov 13 18:10:49 2013 -0800

    Allow PADTMPs’ strings to be swiped

    While copy-on-write does speed things up, it is not perfect.  Take
    this snippet for example:

    $a = "$b$c";
    $a .= $d;

    The concatenation operator on the rhs of the first line has its own
    scalar that it reuses every time that operator is called (its target).
    When the assignment happens, $a and that target share the same string
    buffer, which is good, because we didn’t have to copy it.  But because
    it is shared between two scalars, the concatenation on the second line
    forces it to be copied.

    While copy-on-write may be fast, string swiping surpasses it, because
    it has no later bookkeeping overhead.  If we allow stealing targets’
    strings, then $a = "$b$c" no longer causes $a to share the same string
    buffer as the target; rather, $a steals that buffer and leaves the tar-
    get undefined.  The result is that neither ‘$a =’ nor ‘$a .= $d’ needs
    to copy any strings.  Only the "$b$c" will copy strings (unavoidably).

    This commit only applies that to long strings, however.  This is why:

    Simply swiping the string from any swipable TARG (which I tried at
    first) resulted in a significant slowdown.  By swiping the string from
    a TARG that is going to be reused (as opposed to a TEMP about to be
    freed, which is where swipe was already happening), we force it to
    allocate another string next time, greatly increasing the number
    of malloc calls.  malloc overhead exceeds the overhead of copying
    short strings.

    I tried swiping TARGs for short strings only when the buffer on the
    lhs was not big enough for a copy (or there wasn’t one), but simple
    benchmarks with mktables show that even checking SvLEN(dstr) is enough
    to slow things down, since the speed-up this provides is minimal where
    short strings are involved.

    Then I tried checking just the string length, and saw a consistent
    speed increase.  So that’s what this patch uses.  Programs using short
    strings will not benefit.  Programs using long strings may see a 1.5%
    increase in speed, due to fewer string copies.

This was a regression in 5.20 compared to 5.18, so I think we should
consider cherry-picking c0683843e to any future 5.20.2 maint release.
But regardless, I'll add a test to blead for this bug, so it doesn't
regress again.

-- 
Aaron Crane ** http://aaroncrane.co.uk/

Thread Previous | Thread Next


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About