Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

5.20.x regression in "state" under PERL_NO_COW #14175

Closed
p5pRT opened this issue Oct 21, 2014 · 15 comments
Closed

5.20.x regression in "state" under PERL_NO_COW #14175

p5pRT opened this issue Oct 21, 2014 · 15 comments
Labels

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Oct 21, 2014

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

Searchable as RT123029$

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 21, 2014

From @arc

Niels Larson reports that Perl 5.20 compiled with -DPERL_NO_COW has a regression with state variables​: they get reset to undef when accessed​:

sub no_PERL_NO_COW_regression {
  state $s;
  $s = 'foo';
  my $c = $s;
  return defined $s;
}
ok(no_PERL_NO_COW_regression(),
  "state variables don't surprisingly disappear when accessed");

Bisection reveals that the bug was introduced in 9ffd39a (between 5.19.6 and 5.19.7), and fixed in c068384 (between 5.21.4 and 5.21.5).

I'm creating this ticket so that (a) a regression test can mention it, and (b) the meta-ticket for 5.20.2 can depend on it.

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 21, 2014

From @arc

Commit c4a33ec contains a regression test for this bug.

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 21, 2014

From [Unknown Contact. See original ticket]

Commit c4a33ec contains a regression test for this bug.

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 21, 2014

@arc - Status changed from 'new' to 'open'

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 21, 2014

From @cpansprout

On Tue Oct 21 10​:36​:51 2014, arc wrote​:

Niels Larson reports that Perl 5.20 compiled with -DPERL_NO_COW has a
regression with state variables​: they get reset to undef when
accessed​:

sub no_PERL_NO_COW_regression {
state $s;
$s = 'foo';
my $c = $s;
return defined $s;
}
ok(no_PERL_NO_COW_regression(),
"state variables don't surprisingly disappear when accessed");

Bisection reveals that the bug was introduced in
9ffd39a (between 5.19.6 and 5.19.7),
and fixed in c068384 (between 5.21.4
and 5.21.5).

I'm creating this ticket so that (a) a regression test can mention it,
and (b) the meta-ticket for 5.20.2 can depend on it.

With a COW build​:

$ perl5.20.1 -l -E 'sub { state $s; $s = "foo"x500; my $c = $s; print $s//"undef"}->()'
undef

SvPADTMP and SvPADSTALE used to share a bit, so

  (sflags & (SVs_PADTMP|SVf_READONLY|SVf_PROTECT|SVf_IsCOW))
  == SVs_PADTMP

gave the wrong answer. Putting SVs_PADMY in the list of flags to check will fix the bug for 5.20. However, there is also a problem with the way CHECK_COWBUF_THRESHOLD is defined. Here we use it outside #ifdef PERL_NEW_COPY_ON_WRITE, and when COW is not enabled it is not defined properly. So we need to change the COW constants to be defined regardless of whether COW is enabled, since they are also used for targ swiping. 5.20 without COW is slower than it needs to be, because targ swiping happens far too much, resulting in many malloc calls.

I’ll write a separate maint patch on a branch.

--

Father Chrysostomos

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 22, 2014

From @cpansprout

On Tue Oct 21 16​:19​:36 2014, sprout wrote​:

I’ll write a separate maint patch on a branch.

Please review (and maybe even vote on backporting) the sprout/maint-5.20-123029 branch, which contains two fixes​:

1) Don’t swipe short PADTMP buffers when COW is disabled. That was
  never intended to happen, and it is a regression, in that it
  causes a significant slowdown.
2) Check the flags properly and don’t steal buffers from state vars.

--

Father Chrysostomos

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 22, 2014

From @arc

This looks well-formed to me for a maint change, though I don't claim to entirely understand that part of the core. With that caveat aside, given that it fixes this regression in 5.20, I vote +1 to backporting it to maint.

Thanks, Father Chrysostomos.

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 22, 2014

From [Unknown Contact. See original ticket]

This looks well-formed to me for a maint change, though I don't claim to entirely understand that part of the core. With that caveat aside, given that it fixes this regression in 5.20, I vote +1 to backporting it to maint.

Thanks, Father Chrysostomos.

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 22, 2014

From @jkeenan

On 10/21/2014 11​:28 PM, Father Chrysostomos via RT wrote​:

On Tue Oct 21 16​:19​:36 2014, sprout wrote​:

I’ll write a separate maint patch on a branch.

Please review (and maybe even vote on backporting) the sprout/maint-5.20-123029 branch, which contains two fixes​:

Is it possible to smoke-test a backport?

1) Don’t swipe short PADTMP buffers when COW is disabled. That was
never intended to happen, and it is a regression, in that it
causes a significant slowdown.
2) Check the flags properly and don’t steal buffers from state vars.

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 22, 2014

From @arc

James E Keenan via RT <perlbug-followup@​perl.org> wrote​:

Is it possible to smoke-test a backport?

I can't think of a reason why this wouldn't work, to create a
smoke-me/jkeenan/maint-123029 branch with the same code as FC's
branch​:

git fetch && git push camel
origin/sprout/maint-5.20-123029​:smoke-me/jkeenan/maint-123029

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 22, 2014

From @cpansprout

On Wed Oct 22 05​:30​:41 2014, arc wrote​:

James E Keenan via RT <perlbug-followup@​perl.org> wrote​:

Is it possible to smoke-test a backport?

I can't think of a reason why this wouldn't work, to create a
smoke-me/jkeenan/maint-123029 branch with the same code as FC's
branch​:

git fetch && git push camel
origin/sprout/maint-5.20-123029​:smoke-me/jkeenan/maint-123029

I’ve already pushed a smoke-me branch. I’m just waiting for results.

--

Father Chrysostomos

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 27, 2014

From @cpansprout

On Wed Oct 22 06​:03​:56 2014, sprout wrote​:

On Wed Oct 22 05​:30​:41 2014, arc wrote​:

James E Keenan via RT <perlbug-followup@​perl.org> wrote​:

Is it possible to smoke-test a backport?

I can't think of a reason why this wouldn't work, to create a
smoke-me/jkeenan/maint-123029 branch with the same code as FC's
branch​:

git fetch && git push camel
origin/sprout/maint-5.20-123029​:smoke-me/jkeenan/maint-123029

I’ve already pushed a smoke-me branch. I’m just waiting for results.

I’m seeing these failures on Windows on the smoke-me/maint-5.20-123029 branch, but I don’t think they are related​:

Failures​: (common-args) none
[default]
../cpan/Module-Build/t/basic.t..............................FAILED
  Non-zero exit status​: 1
  Bad plan. You planned 58 tests but ran 47.

[default] -DDEBUGGING -Duseithreads
../t/op/threads.t...........................................FAILED
  Non-zero exit status​: 9
  Bad plan. You planned 27 tests but ran 9.
../t/win32/popen.t..........................................FAILED
  Non-zero exit status​: 9
  No plan found in TAP output

--

Father Chrysostomos

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 1, 2014

@steve-m-hay - Status changed from 'open' to 'pending release'

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 2, 2015

From @khwilliamson

Thanks for submitting this ticket

The issue should be resolved with the release today of Perl v5.22. If you find that the problem persists, feel free to reopen this ticket

--
Karl Williamson for the Perl 5 porters team

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 2, 2015

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

@p5pRT p5pRT closed this Jun 2, 2015
@p5pRT p5pRT added the Severity Low label Oct 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.