COW related performance regression in 5.19 #13800
Comments
From @demerphqThe attached benchmark shows a marked performance degradation between The performance drop has been bisected back to: commit 13b0f67 re-enable Copy-on-Write by default. COW was first introduced (and enabled by default) in 5.17.7. By re-enabling it now, early in the 5.19.x release cycle, hopefully it This commit mainly reverts 9f351b4 and e1fd413 (with -- |
From @demerphq |
From @rjbs* yves orton <perlbug-followup@perl.org> [2014-05-04T06:08:46]
Some discussion on IRC suggested that the fundamental problem is that COW is Is a reasonable next step to find a decent setting for this value to have a -- |
|
The RT System itself - Status changed from 'new' to 'open' |
From @demerphqOn 6 May 2014 16:46, Ricardo Signes <perl.p5p@rjbs.manxome.org> wrote:
I thought I tried setting this and it had no affect whatsoever. Confusingly there are actually two vars for this, and it isnt clear which I will try to play further. Yves -- |
From @demerphqI have further details on this issue. $ cat ../read_bench.pl open my $fh, "<", "/usr/share/dict/american-english" my @got; Performance is 5.14.2: $ perl -E'say $]'; dumbbench -- perl ../read_bench.pl Performance in stock blead from this morning: $ ./perl -Ilib -E'say $]'; dumbbench -- ./perl -Ilib ../read_bench.pl Setting SV_COW_THRESHOLD 1250 which is the same value $ ./perl -Ilib -E'say $]'; dumbbench -- ./perl -Ilib ../read_bench.pl However I do not see simply setting SV_COW_THRESHOLD as the solution. I cheers, |
From @demerphqFurther to this, i pushed the following commit as a branch. It moves the commit ff6e449 Move PERL_NEW_COPY_ON_WRITE thresholds to sv.c so its faster to rebuild On 8 May 2014 11:36, demerphq <demerphq@gmail.com> wrote:
-- |
From [email protected]On 05/04/2014 12:08 PM, yves orton (via RT) wrote:
Shorter test case. The two perls are the commit before the sha1 that $ time install-COW/bin/perl5.19.1 -e 'my $s = "x\n" x 1e5; open my $fh, real 0m0.069s $ time install-COW/bin/perl5.19.1 -e 'my $s = ("x\n") x 1e5; open my real 0m1.326s If I do actual file IO, the same happens, so don't be put off by the We're still reading code (and profiler output) to find the root cause. --Steffen |
From @demerphqOn 8 May 2014 13:33, Steffen Mueller <mail@steffen-mueller.net> wrote:
The only place the threshold is checked is via the GE_COW_THRESHOLD(cur) Yves -- |
From [email protected]RT 121815 was reduced down to a script very similar to the benchmark here (I put it in the body of the email instead of as an attachment at http://nntp.perl.org/group/perl.perl5.porters/215113); can/should that be made a child ticket of this one (or some such? I don't really know RT). |
From [Unknown Contact. See original ticket]RT 121815 was reduced down to a script very similar to the benchmark here (I put it in the body of the email instead of as an attachment at http://nntp.perl.org/group/perl.perl5.porters/215113); can/should that be made a child ticket of this one (or some such? I don't really know RT). |
From @iabynOn Thu, May 08, 2014 at 01:33:16PM +0200, Steffen Mueller wrote:
The fault here appears to lie with sv_gets() rather than COW per se. SV = PV(0xa98c18) at 0xac4638 The sv_setsv() called from pp_push() then does a COW copy of $_, so the At the next call to sv_gets(), the COW is dropped from $_ and another 200K So the net effect is that every SV pushed onto the array has a 200K How much better this is when reading from a file, I don't yet know; I suspect I had a quick look at sv_gets() 5.18.2 and it was creating 80 byte If I'm right, then the real fix probably needs to be in sv_gets(); Anyway I thought I'd report this early, since other people are working on -- |
From @iabynOn Thu, May 08, 2014 at 04:28:41PM +0100, Dave Mitchell wrote:
And looking further: with this code: my $s = "x\n" x 1e5; gives: $ perl5190 ~/tmp/p $ perl5191 ~/tmp/p Note the massive LEN. And changing the code to read from a regular file gives: $ perl5190 ~/tmp/p $ perl5191 ~/tmp/p -- |
From @demerphqOn 8 May 2014 17:48, Dave Mitchell <davem@iabyn.com> wrote:
This probably also explains the out of memory errors that people are seeing Yves -- |
From @LeontOn Thu, May 8, 2014 at 5:28 PM, Dave Mitchell <davem@iabyn.com> wrote:
sv_gets is begging for a rewrite, for more reasons than just this one. It's
That is a good point too. Leon |
From @demerphqOn 8 May 2014 17:48, Dave Mitchell <davem@iabyn.com> wrote:
Thanks, this gave me the clue. See branch yves/cow_threshold_in_sv_c for a BTW, setting PERL_PREALLOC_EMPTY_IN_SV_GETS to a higher number produces a The code probably needs a comment too. Patch: commit b53d1dc in sv_gets() if the sv is empty then preallocate a small buffer When the COW logic is triggered there is an unfortunate slowdown. https://rt-archive.perl.org/perl5/Ticket/Display.html?id=121796 What happens is that the buffer gets "stolen" by COW, By initializing the string to at least something this does not happen. sv_gets() seriously needs love. :-( Inline Patchdiff --git a/sv.c b/sv.c
index 439b973..3c52e0d 100644
--- a/sv.c
+++ b/sv.c
@@ -46,6 +46,8 @@
char *gconvert(double, int, int, char *);
#endif
+#define PERL_PREALLOC_EMPTY_IN_SV_GETS 20
+
#ifdef PERL_NEW_COPY_ON_WRITE
# ifndef SV_COW_THRESHOLD
# define SV_COW_THRESHOLD 0 /* min string length for cow */
@@ -8154,6 +8156,10 @@ Perl_sv_gets(pTHX_ SV *const sv, PerlIO *const fp,
+ if (!SvLEN(sv)) { cnt = PerlIO_get_cnt(fp); /* get count into register */ -- |
From @tseeOn 05/09/2014 03:00 AM, demerphq wrote:
Oh, wow. Not to take away the achievement of having figured this nasty I've only scratched the surface, but I think this is really multiple The downside of preallocating a buffer of some arbitrary (small) size is --Steffen |
From @demerphqOn 9 May 2014 08:35, Steffen Mueller <smueller@cpan.org> wrote:
Yeah, I guess so. I think the alternative is a complete rewrite of the Which I am actually game for, and started at some point (to support regex
I was a bit tired when I wrote that. It actually allocates the same amount
Well when the alternative is what it is now, I think that is an acceptable For what its worth I think the code in sv_gets() was written from the point So I definitely agree there is a mess of weird interactions here. However from what i can tell the way it works with my patch is pretty close So I think we are good from now. Sorry if this is a bit stream of consciousness, I didnt get a lot of sleep Yves -- |
From @tseeOn 05/09/2014 10:08 AM, demerphq wrote:
Don't get me wrong. I'll take your patch over the current broken --Steffen |
From @LeontOn Fri, May 9, 2014 at 10:08 AM, demerphq <demerphq@gmail.com> wrote:
I had similar plans for slightly different reasons. Part of the logic
There is also the assumption that an extra read through the buffer is more Leon |
From @iabynOn Fri, May 09, 2014 at 10:21:35AM +0200, Steffen Mueller wrote:
Unfortunately I don't think the patch is sufficient. If the line being For a file full of 76 char char lines, and with your cow_threshold_in_sv_c open my $fh, "<", "/tmp/s" or die; for my $i (0..20) { $ head /tmp/s $ wc -l /tmp/s $ ./perl -Ilib ~/tmp/p 2>&1 | grep LEN I still suspect that it may be better to (also) hack the COW/swipe SvLEN(sv) > 16 && SvLEN(sv) > 2*SvCUR(sv) This would just be making perl revert to pre-COW behaviour for certain I intend to spend today looking at this issue further (i.e. daytime, -- |
From @demerphqOn 9 May 2014 12:46, Dave Mitchell <davem@iabyn.com> wrote:
I agree its not great. I think maybe the threshold should be raised.
I did this last night but apparently forgot to push it: $ git log -1 origin/yves/check_cow_threshold check_cow_threshold However I think it is also the wrong fix, in that I dont see why strings The problem seems to really be related to *uncowing* a string. A strategy where the "original owner" of the string got to keep its string, I was also thinking that we really could use a "DONT COW THIS SV" flag so Also I think the behavior of sv_gets() is not perfect. Maybe it made sense We need to bisect when this behavior of setting the size of $_ to the size Yves -- |
From @demerphqThoughts on sv_gets() and COW. sv_gets() implements readline, and sets from <>. It handles the various The partof sv_gets() that implements "find a delimiter" logic starts with /* Here is some breathtakingly efficient cheating */ The uninitiated then finds a dense and nearly impenetrable set of logic, Being uninitiated I thought I would try to rewrite it to be friendly to This was a useful if perhaps unfruitful exercise as it taught me that the First off, resist the tendency to think about the problem in terms of line The strategy that sv_gets() uses is to preallocate the target SV with as This cleverness of this strategy is that Perl performs relatively few The problem however is that it *really* wants its target strings to have This last part plays really badly with how COW is currently implemented. Consider the following code: while (<>) { What this code does is first load $_ with sv_gets(), and lets assume that Then the push @got, $_ results in $got[0] and $_ containing a COW'ed pv. Pre COW this problem does not come up because $got[0] ends up with a copy The most obvious short term fix for this is what Dave M proposed, impose an I wonder however if this is really just masking the deeper issue, and that Yves On 4 May 2014 12:08, yves orton <perlbug-followup@perl.org> wrote:
-- |
From [email protected]On 05/11/2014 11:44 AM, demerphq wrote:
I'm really not a fan of yet another state bit on an SV. It's already all |
From @tseeSomehow I think I just accidentally sent my reply long before I was done On 05/11/2014 11:44 AM, demerphq wrote:
I don't think another bit of state that needs to be taken into account How about changing the scanning logic to NOT copy as it goes but really --Steffen |
From @LeontOn Sun, May 11, 2014 at 12:34 PM, Steffen Mueller <smueller@cpan.org> wrote:
Maybe. I don't think we have flag bits available to the point is moot right How about changing the scanning logic to NOT copy as it goes but really
That's what I suggested earlier. It would make more sense to me too, though Leon |
From @tseeOn 05/11/2014 02:25 PM, Leon Timmermans wrote:
I think that's a tad optimistic. We're talking about string processing
Agreed. --Steffen |
From @bulk88On Sun May 11 02:45:34 2014, demerphq wrote:
Here is an idea, realloc to SvCUR. -- |
From @rjbsYves noted that the oversizing of strings may be the cause of OOM in https://rt.perl.org/Ticket/Display.html?id=121826 and (I believe) https://rt-archive.perl.org/perl5/Ticket/Display.html?id=121815 -- |
From [Unknown Contact. See original ticket]Yves noted that the oversizing of strings may be the cause of OOM in https://rt.perl.org/Ticket/Display.html?id=121826 and (I believe) https://rt-archive.perl.org/perl5/Ticket/Display.html?id=121815 -- |
From @iabyn
Note however that COW has different use cases which would imply For example, sometimes the original string is just a temporary value, $x = (string-expression); Here, the expression creates a temporary SV, which we want to pass to $x Conversely, in something like $s =~ /(.)/; we likely want to keep $s, and temporarily make use of $1. -- |
From @demerphqOn 12 May 2014 17:25, Ricardo SIGNES via RT <perlbug-comment@perl.org>wrote:
We should know soon. The patch I pushed this morning should be run against Yves -- |
From @demerphqOn 11 May 2014 15:02, Steffen Mueller <smueller@cpan.org> wrote:
Steffen and I accidentally continued discussing this offlist (the And I have to admit I was wrong. After a bunch of benchmarking, analysis, and some late night hacking I do not have an implementation yet, but we definitely can do better than It is on my todo list. More in the future. Thanks to Steffen for not Yves -- |
From @LeontOn Sun, May 11, 2014 at 3:02 PM, Steffen Mueller <smueller@cpan.org> wrote:
I meant after the strchr(), right after the buffer is accessed it I'd hope Leon |
From @rjbsI've removed this from blocking 5.20.0. Yves, do you want to keep this ticket open for the further changes, or make a new one? -- |
From [Unknown Contact. See original ticket]I've removed this from blocking 5.20.0. Yves, do you want to keep this ticket open for the further changes, or make a new one? -- |
From @demerphqOn 19 May 2014 16:42, Ricardo SIGNES via RT <perlbug-comment@perl.org>wrote:
I think we should start a new ticket. So please feel free to close this one. Yves |
From @rjbsClosed per discussion with Yves. -- |
|
@rjbs - Status changed from 'open' to 'resolved' |
From @cpansproutOn Mon May 12 09:22:19 2014, demerphq wrote:
Finding this whole discussion late, I would like to thank Yves Orton and Dave Mitchell for diagnosing and fixing this. That’s quite a bit of detective work! -- Father Chrysostomos |
From [Unknown Contact. See original ticket]On Mon May 12 09:22:19 2014, demerphq wrote:
Finding this whole discussion late, I would like to thank Yves Orton and Dave Mitchell for diagnosing and fixing this. That’s quite a bit of detective work! -- Father Chrysostomos |
From @demerphqOn 23 August 2014 07:20, Father Chrysostomos via RT <
Now you are back I think there are issues raised in this thread that bear Specifically: Should we have a "do not COW" flag? Also, I had a question about the implementation of the refcount data for I was wondering if you had thought about doing something like the OOK The reason I ask is twofold, first there is a lack of locality of reference Just curious for your thoughts. Yves -- |
From @cpansproutOn Sun Aug 24 04:17:31 2014, demerphq wrote:
It doesn’t really matter to me. I think that in each case where it seems that COW shouldn’t happen, it’s because the COW algorithm needs adjustment. Fixing the algorithm for one particular inefficient case should also fix it for other similar cases. That said, I would not be opposed to such a flag. (BTW, SVf_READONLY already means that, but I assume you want a flag for mutable strings.) We have plenty of room in sv_flags. You just have to look closely to see it. :-) Specifically, SVf_FAKE is unused on strings now and SVf_OOK|SVf_IsCOW is an unused combination. SVf_AMAGIC is free for non-HVs.
The whole point of this COW mechanism was to upgrade existing strings to COW strings. That precludes using a byte at the beginning of the string, because that byte is already in use. If you are thinking of allocating most strings with an extra initial byte to begin with, that could work, but would effectively disable COW* until we comb through the entire codebase changing every instance of SvPVX_set to account. Is it worth that much trouble? * Bear in mind that I have an innate tendency to exaggerate.
That is why I sent patches to many CPAN modules to stop them from diddling with SvPVX without calling sv_force_normal first. :-) I hope that COW strings are common enough that such dodgy behaviour will result in test failures. -- Father Chrysostomos |
From @cpansproutOn Sun Aug 24 07:06:26 2014, sprout wrote:
How about SvLEN(sv)-- when turning on COWness? (And SvLEN(sv)++ when turning it off.) -- Father Chrysostomos |
From @rurbanSince you now realized the problem with COW, maybe I can add my old The cow refcount needs to be in it's own xpv field. It should not be The two usecases for COW are threads and the compiler. proper threads |
From @demerphqOn 26 August 2014 15:49, Reini Urban <rurban@x-ray.at> wrote:
Which problem? There were at least two I mentioned.
Which devolves down to some specific overhead for all strings. I think you are right that there is a debate to be had about whether adding
Huh? What do you mean by "*the* two uses cases"? The uses cases I can think of for COW have nothing to do with threads or
The current implementation is not about making COW strings read only. I Yves -- |
From @iabynOn Tue, Aug 26, 2014 at 08:49:25AM -0500, Reini Urban wrote:
How would that work? Each SV has its own private XPV struct, but a shared Or are you suggesting instead that a single XPV should be shared between -- |
Migrated from rt.perl.org#121796 (status was 'resolved')
Searchable as RT121796$
The text was updated successfully, but these errors were encountered: