Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

fix NODEFAULT_SHAREKEYS #201

Closed
rurban opened this issue Sep 14, 2016 · 5 comments
Closed

fix NODEFAULT_SHAREKEYS #201

rurban opened this issue Sep 14, 2016 · 5 comments

Comments

@rurban
Copy link
Member

rurban commented Sep 14, 2016

When NODEFAULT_SHAREKEYS is defined, sv_clear of a IsCOW/LEN=0 PV fails to get the he before the hek, which is shared with the pv.

With default SHAREKEYS every single key is stored twice. Once in strtab, and once in the hash. This seems to be a massive overhead. But I cannot test the overhead with this failure. (=> 20-30% faster)

This is an upstream bug, typical bitrot. perl5 simply assumes that every single non-strtab hash uses shared keys, ignoring the HVhek_UNSHARED bit somewhere. Or most likely forgetting to copy this bit.
NODEFAULT_SHAREKEYS was previously in newHV() in hv.c, added with fde52b5 in perl 5.003_01

repro: Configure with -DDEBUGGING -Accflags=-DNODEFAULT_SHAREKEYS;
make miniperl; ./miniperl -Ilib configpm && make uni.data and see the unshare_hek assert.
This even happens with 5.10.0, before new IsCOW times, but works with 5.8.9. So it got broken during the 5.9 cycle, probably with the old COW.

@rurban
Copy link
Member Author

rurban commented Sep 15, 2016

upstream bug bisected with git bisect start perl-5.10.0 perl-5.9.0;
git bisect run ../bisect-miniperl.sh

../bisect-miniperl.sh

#!/bin/sh
# exit 125 marks it as untestable, == bisect skip
# exit >128 stops the bisect run immediately.

reset=0
make -s -j4 miniperl 2>/dev/null || \
    (git reset --hard;
     git clean -dxf;
     export reset=1;
     perl -MDevel::PatchPerl -e'Devel::PatchPerl::patch_source'; \
     ./Configure -des -Dusedevel -DEBUGGING \
                 -Accflags=-DNODEFAULT_SHAREKEYS >/dev/null 2>/dev/null && \
     make -s -j4 miniperl 2>/dev/null) || exit 1

./miniperl -Ilib configpm && make -s uni.data
err=$?
git reset --hard
if [ $reset -ne 0 ]; then
    git clean -dxf
    unset reset
fi
if [ $err -ne 0 ]; then exit 1; fi

to

commit a51caccf222faf8588ed061240391957b9a0e70d
Author: Nicholas Clark <nick@ccl4.org>
Date:   Tue Apr 11 16:17:13 2006 +0000

    Stop Perl_newSVpvn_share() potentially leaking the return result from
    bytes_from_utf8().

    p4raw-id: //depot/perl@27766

@rurban rurban added the bug label Sep 15, 2016
@rurban
Copy link
Member Author

rurban commented Sep 15, 2016

benchmarks: simple bench.pl without many hashes: on linux 27% faster with NODEFAULT_SHAREKEYS.
on darwin the fluctuation and loadavg is too high to be meaningful. barely any measurable differences.

@rurban
Copy link
Member Author

rurban commented Sep 29, 2016

It turned out the hash loop itself was stripping HEK_UNSHARED from unshared heks.
Broken by b2c6404 (Nicholas Clark) 2003-11-22 16:43:09
and essentially introduced with 19692e8 (Nicholas Clark) 2002-04-06 01:21:17

rurban pushed a commit that referenced this issue Sep 29, 2016
Dont store all hash keys in strtab also.

WIP: sv_clear of a IsCOW/LEN=0 PV fails to get the he before the hek, which
is shared with the pv. Fixed with subsequent commits.

See GH #201
rurban pushed a commit that referenced this issue Sep 29, 2016
The WASUTF8 logic turned off UNSHARED and STATIC also,
whilst it should only reset UTF8 and WASUTF8.

Closes GH #201
@rurban
Copy link
Member Author

rurban commented Sep 29, 2016

Merged with c2ef6c7

@rurban rurban closed this as completed Sep 29, 2016
@rurban
Copy link
Member Author

rurban commented Oct 2, 2016

Simple scripts without much hash copying or looping are 20-30% faster, but other scripts are a few percent slower. So we keep NODEFAULT_SHAREKEYS undefined.

In detail: ../bench.pl is faster, perlbench-run is slower with -DNODEFAULT_SHAREKEYS

rurban pushed a commit that referenced this issue Oct 12, 2016
Dont store all hash keys in strtab also.

WIP: sv_clear of a IsCOW/LEN=0 PV fails to get the he before the hek, which
is shared with the pv. Fixed with subsequent commits.

See GH #201
rurban pushed a commit that referenced this issue Oct 12, 2016
The WASUTF8 logic turned off UNSHARED and STATIC also,
whilst it should only reset UTF8 and WASUTF8.

Closes GH #201
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

1 participant