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
Bleadperl v5.27.3-34-gf6107ca24b breaks MLEHMANN/AnyEvent-HTTP-2.23.tar.gz #16162
Comments
From @andkAs usual, cudos to discoverer Slaven! bisect
diagnostics: Perl Info
|
From @ilmari(Andreas J. Koenig) (via RT) <perlbug-followup@perl.org> writes:
AnyEvent is relying on in-place sorting of its array of pending timers Weakening all the references in the array after sorting it fixes the With the old behaviour, getting the strengthening effect was trivial: This makes me think that despite all the work that's gone into making I still think sv_rvunweaken() is a useful API function, so the commit - ilmari |
The RT System itself - Status changed from 'new' to 'open' |
From zefram@fysh.orgDagfinn Ilmari Mannsaker wrote:
With the new behaviour, one can fairly trivially get the
-1. The optimisation should be a pure optimisation. If the useful -zefram |
From @andkAlso affected: MGRIMES/AnyEvent-Filesys-Notify-1.21.tar.gz Result type: intermittent failures. Up to v5.27.3-33-gae2cf9f629 I ran -- |
From @tonycozOn Fri, Sep 22, 2017 at 03:48:18AM +0100, Zefram wrote:
I agree. Tony |
From @ilmariZefram <zefram@fysh.org> writes:
Attached is a patch for AnyEvent that does this for its pending timer |
From @ilmari0001-Use-strong-references-to-weak-references-in-the-time.patchFrom eb30dd3aab9c33204418c77f7f81d3acb6e5b3eb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Tue, 26 Sep 2017 22:03:48 +0100
Subject: [PATCH] Use strong references to weak references in the @timer
Perl 5.27.4 fixes the in-place sort optimisation (@a = sort @a) to
strengthen references, like assignment should.
However, we need to keep the references to timers weak, so callers can
cancel them by dropping their reference. To achieve this, make the
actual references in @timer strong references to weak references to the
value returned to the user, and filter out ones that have gone undef
when necessary.
---
lib/AnyEvent/Loop.pm | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/lib/AnyEvent/Loop.pm b/lib/AnyEvent/Loop.pm
index fb9351a..f57eab0 100644
--- a/lib/AnyEvent/Loop.pm
+++ b/lib/AnyEvent/Loop.pm
@@ -202,22 +202,26 @@ sub one_event {
# first sort timers if required (slow)
if ($MNOW >= $need_sort) {
$need_sort = 1e300;
- @timer = sort { $a->[0] <=> $b->[0] } @timer;
+ @timer = sort { $$a->[0] <=> $$b->[0] } grep { defined $$_ } @timer;
+ }
+ else {
+ # shift off any cancelled timers at the start of the queue
+ shift @timer while @timer and not defined ${$timer[0]};
}
# handle all pending timers
- if (@timer && $timer[0][0] <= $MNOW) {
+ if (@timer && ${$timer[0]}->[0] <= $MNOW) {
do {
- my $timer = shift @timer;
- $timer->[1] && $timer->[1]($timer);
- } while @timer && $timer[0][0] <= $MNOW;
+ my $timer = ${shift @timer};
+ $timer && $timer->[1] && $timer->[1]($timer);
+ } while @timer && ${$timer[0]}->[0] <= $MNOW;
} else {
# poll for I/O events, we do not do this when there
# were any pending timers to ensure that one_event returns
# quickly when some timers have been handled
my ($wait, @vec, $fds)
- = (@timer && $timer[0][0] < $need_sort ? $timer[0][0] : $need_sort) - $MNOW;
+ = (@timer && ${$timer[0]}->[0] < $need_sort ? ${$timer[0]}->[0] : $need_sort) - $MNOW;
$wait = $wait < MAXWAIT ? $wait + ROUNDUP : MAXWAIT;
$wait = 0 if @idle;
@@ -252,7 +256,7 @@ sub one_event {
} elsif (AnyEvent::WIN32 && $fds && $! == AnyEvent::Util::WSAEINVAL) {
# buggy microshit windoze asks us to route around it
CORE::select undef, undef, undef, $wait if $wait;
- } elsif (!@timer || $timer[0][0] > $MNOW && !$fds) {
+ } elsif (!@timer || ${$timer[0]}->[0] > $MNOW && !$fds) {
$$$_ && $$$_->() for @idle = grep $$$_, @idle;
}
}
@@ -319,8 +323,8 @@ sub timer($$$) {
if ($interval) {
$self = [$MNOW + $after , sub {
$_[0][0] = List::Util::max $_[0][0] + $interval, $MNOW;
- push @timer, $_[0];
- weaken $timer[-1];
+ weaken(my $weak = $_[0]);
+ push @timer, \$weak;
$need_sort = $_[0][0] if $_[0][0] < $need_sort;
&$cb;
}];
@@ -328,8 +332,8 @@ sub timer($$$) {
$self = [$MNOW + $after, $cb];
}
- push @timer, $self;
- weaken $timer[-1];
+ weaken(my $weakself = $self);
+ push @timer, \$weakself;
$need_sort = $self->[0] if $self->[0] < $need_sort;
$self
--
2.7.4
|
From @ilmari
Normally I would agree, but the previous behaviour has been there as - ilmari |
From zefram@fysh.orgDagfinn Ilmari Mannsaker wrote:
You could just stick it in the RT queue. (And face even more wrath.)
It's been a bug all that time.
Yes.
That's not detectable, is it? Preserving deletedness seems like it -zefram |
From @ilmariZefram <zefram@fysh.org> writes:
$ perl -E '@a=(1..3); delete $a[1]; @a = reverse @a; say 0+exists $a[1]' -- |
From @andkWhat is the state of this ticket? From looking at it it looks stalled. Has anybody summarised? Reported to Marc? Provided a patch? Considered Thanks! |
From @ilmariAndreas Koenig <andreas.koenig.7os6VVqR@franz.ak.mind.de> writes:
I provided a patch in As mentioned in that previous message, I'm not in the mood to confront Sawyer, do you feel like sending these two patches to Marc and suggest - ilmari |
From @ilmari0001-Re-weaken-timer-references-after-sorting.patchFrom 8e12a666543e50043419630d5e4ff00c7dd277e2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Fri, 17 Nov 2017 23:59:30 +0000
Subject: [PATCH] Re-weaken timer references after sorting
Perl 5.27.4 fixes the in-place sort optimisation (@a = sort @a) to
strengthen references, like assignment should.
However, we need to keep the references to timers weak, so callers can
cancel them by dropping their reference.
---
lib/AnyEvent/Loop.pm | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/lib/AnyEvent/Loop.pm b/lib/AnyEvent/Loop.pm
index fb9351a..f7145fd 100644
--- a/lib/AnyEvent/Loop.pm
+++ b/lib/AnyEvent/Loop.pm
@@ -173,6 +173,9 @@ BEGIN {
$round = 0.001 if $round < 0.001; # 1ms is enough for us
$round -= $round * 1e-2; # 0.1 => 0.099
eval "sub ROUNDUP() { $round }";
+
+ my $unweakens = $] >= 5.027004 ? 1 : 0;
+ eval "sub INPLACE_SORT_UNWEAKENS() { $unweakens }";
}
_update_clock;
@@ -203,6 +206,9 @@ sub one_event {
if ($MNOW >= $need_sort) {
$need_sort = 1e300;
@timer = sort { $a->[0] <=> $b->[0] } @timer;
+ if (INPLACE_SORT_UNWEAKENS) {
+ weaken($_) for @timer;
+ }
}
# handle all pending timers
--
2.7.4
|
From @xsawyerxOn Fri, 17 Nov 2017 16:09:12 -0800, ilmari wrote:
I didn't receive an email for this correspondence so I only saw the ping from Andreas. I have just sent an email to Marc with the latest patch, asking to apply the patch and release a new version. P5P was CC'ed on the email as well. Thank you for producing the patches, Ilmari. |
From @iabynOn Sun, Nov 19, 2017 at 06:10:28AM -0800, Sawyer X via RT wrote:
My understanding of the current state of this 5.28 blocker ticket is: 1) A bug fix to blead removed an inadvertent user-visible difference 2) AnyEvent relied on this now-fixed buggy behaviour; 3) A patch has been privately submitted to Marc; 4) There has not been a new release of AnyEvent yet; 5) There was a brief discussion about the desirability of an explicit I propose a) this ticket is removed from 5.28 blockers. -- |
From @andk
> On Sun, Nov 19, 2017 at 06:10:28AM -0800, Sawyer X via RT wrote:
> My understanding of the current state of this 5.28 blocker ticket is: > 1) A bug fix to blead removed an inadvertent user-visible difference It has been pointed out that the "internally optimized behaviour" of '@a > 2) AnyEvent relied on this now-fixed buggy behaviour; The problematic terms continue. > 3) A patch has been privately submitted to Marc; But the patch has issues. Discussion about it has been avoided and > 4) There has not been a new release of AnyEvent yet; What could that probably mean? > 5) There was a brief discussion about the desirability of an explicit Why was it so brief? Originally the in-place sort was deemed as a > I propose > a) this ticket is removed from 5.28 blockers. I would suggest a revert with a chance to retry for 5.30, but with the > Hofstadter's Law: It always takes longer than you expect, even when you Long live Hofstadter, |
From tom@binary.comOn 20 April 2018 at 12:50, Andreas Koenig <
I believe the original release note from perl5100delta was:
Since there was no corresponding documentation update to sort itself, and
The sort optimisation was added in 5.10. AnyEvent has test passes (and code When I mentioned the in-place sort issue to ilmari originally, I was under It's something I've run into a few times in the last few years, is more of cheers, Tom |
From @iabynOn Fri, Apr 20, 2018 at 06:50:54AM +0200, Andreas Koenig wrote:
Again, as the author of that optimisation,
Any bug fix must by definition change established behaviour.
We'll have to disagree about the "announced" bit.
My personal feeling is that avoiding unexpected differences in behaviour
Unusually for CPAN authors, Marc doesn't use any publicly-facing bug
Again, due to the lack of a public bug tracker, that's hard to guess.
Presumably because no-one could come up with any good suggestions.
Again, I disagree.
Many things are possible. In practice, perl gets 1000 new RT tickets per
Does anyone else here any feelings either way? -- |
From @iabynOn Fri, Apr 20, 2018 at 08:29:08AM +0100, Dave Mitchell wrote:
From the silence, I'm assuming no-one else wants this change reverted -- |
From @khwilliamsonOn 04/25/2018 04:18 AM, Dave Mitchell wrote:
I could support a temporary revert. |
From @jkeenanOn Wed, 25 Apr 2018 10:20:08 GMT, davem wrote:
We're so close to release that I think reverting would open up more problems than it would solve. MLEHMANN and his defenders are going to complain regardless of what we do. We've lived with the code in its current state, so we have a fair idea of what its impact will be. Unless someone wants to revert in a branch and test most of CPAN against that branch, and unless such a process has demonstrably better results than the current state of blead, I think we should proceed on course. Let's face the music and dance. Jim Keenan -- |
From @andk
> MLEHMANN and his defenders are going to complain regardless of what While you try to make this an argument about my gang vs your gang, Thank you very much, |
From @iabynOn Wed, Apr 25, 2018 at 01:45:46PM -0600, Karl Williamson wrote:
I have pushed a branch, smoke-me/davem/weak_sort, which contains (I hope) -- |
From @xsawyerxAlright, everyone. It's time. I'm calling it. We revert this fix for 5.28 and leave it for 5.30. I have a lot to say about this and I'm incredibly frustrated and unhappy I have privately emailed Dave about this and he now provided the branch. On 04/26/2018 07:21 AM, Andreas Koenig wrote:
|
From @andkAfter some digging I finally found the original tickets that started 2004-01-25: https://rt.perl.org/Public/Bug/Display.html?id=25263 RT issue 2004-01-26: https://rt.perl.org/Public/Bug/Display.html?id=25263#txn-72227 Within same RT ticket 2004-02-20: https://rt.perl.org/Public/Bug/Display.html?id=25263#txn-79013 Within same RT ticket 2017-09-21: https://rt.perl.org/Public/Bug/Display.html?id=132142 BBC ticket 2017-11-19: https://www.nntp.perl.org/group/perl.perl5.porters/2017/12/msg248259.html From: Sawyer X 2018-04-26: https://rt.perl.org/Public/Bug/Display.html?id=132142#txn-1550198 From: Sawyer X -- |
From @iabynOn Sun, Jun 10, 2018 at 08:52:25PM +0200, Andreas Koenig wrote:
I am genuinely puzzled by your email. In what way do you feel that this In particular, reading through the 2004 ticket in its entirety only -- |
From @andk
> On Sun, Jun 10, 2018 at 08:52:25PM +0200, Andreas Koenig wrote:
> I am genuinely puzzled by your email. In what way do you feel that this Thanks for asking. I would suggest re-reading the one sentence: : It would be cool if I could say: sort @array; or @array = sort_in_place Ask yourself what in-place-sort means and answer the following - does an in-place-sort change any element within the array? My tentative answers: - no > In particular, reading through the 2004 ticket in its entirety only There is no doubt that your intentions back then and today are I sincerely believe it cannot be too hard for an average bystander of -- |
From @iabynOn Sun, Jun 24, 2018 at 11:34:30AM +0200, Andreas Koenig wrote:
The basic question question is whether a perl programmer could have a @array1 = sort @array1 behaves differently than, e.g. @array2 = sort @array1; @array1 = @array1 as regards things like strengthening weak references. Since there is absolutely no mention of this in the documentation for There is an entry in perldelta talking about an 'in-place' optimisation, Finally there is the RT ticket, originally concerning a 'sort -u' feature I really cannot see how any of this justifies not strengthening weak refs -- |
From @andk
> On Sun, Jun 24, 2018 at 11:34:30AM +0200, Andreas Koenig wrote: In your previous posting you asked me "in what way do you feel that the Thank you very much. |
From @iabynOn Wed, Jun 27, 2018 at 08:21:43AM +0200, Andreas Koenig wrote:
There is ambiguity in the term 'in-place' - it can either refer to If the phrase 'in-place' was being used to describe a user-level feature, no If the phrase 'in-place' was being used to describe the implementation yes NB I am of the firm opinion that the 2004-02-20 commit was entirely Here's a question for you. What in the documentation for sort, might lead -- |
From @andk
> On Wed, Jun 27, 2018 at 08:21:43AM +0200, Andreas Koenig wrote:
> There is ambiguity in the term 'in-place' - it can either refer to An ambiguity there is, what a great step forward, thank you! > If the phrase 'in-place' was being used to describe a user-level feature, > no Could you extend on that? Especially, how big is the difference between I mean, look how perfectly well allocated the elements of the array are % perl -le 'use Devel::Peek; @a = qw(2 1);Dump @a;@a = sort @a;Dump @a;' 2>&1 | grep at Both head and body of every PV involved stay the same. This you can't > no > If the phrase 'in-place' was being used to describe the implementation > yes > NB I am of the firm opinion that the 2004-02-20 commit was entirely But let's recall, that there is a finally acknowledged ambiguity for the > Here's a question for you. What in the documentation for sort, might lead It has already been answered above that it was at least documented in : =head2 In-place sorting But there is more to it. Documentation is always more than what projects To sum up: a reasonable perl programmer who was following the -- |
From @iabynOn Thu, Jul 05, 2018 at 09:57:22PM +0200, Andreas Koenig wrote:
This is just beyond absurd. If we apply that criteria to perl generally, So hooray, perl is complete. We can all go away and work on other things A *reasonable* perl programmer would have looked at the documentation for PS - I don't think for one minute the use of the phrase "in-place" was -- |
From @ilmariDave Mitchell <davem@iabyn.com> writes:
FWIW: My original change came abuot exactly because someone on IRC was - ilmari |
From @andk
> A *reasonable* perl programmer would have looked at the documentation for I'm wary whenever somebody argues that something would have needed to be Especially so, when this somebody is a user of the language that the We have a case that what the language maintainer demands from the I would argue that - by having the documentation in the delta manpage, there is a case of a customary right on the side of all users of the Alternatives have not been suggested yet and might exist that might lead > PS - I don't think for one minute the use of the phrase "in-place" was I'm not sure I can parse this sentence correctly. I think I have -- |
From @iabynOn Sat, Jul 14, 2018 at 10:15:11AM +0200, Andreas Koenig wrote:
There was no such documentation in perldelta. There was mention of a new There is a strong ethos within core development that new optimisations Note also that the change was back-ported to 5.8.4 - an odd thing to do
There was nothing to take over.
There was nothing missing.
So do you agree that the behaviour which has been present since 5.000 and
I'm open to suggestions.
You asked me some questions about the meaning of 'in-place'. I pointed -- |
From @andk
> On Sat, Jul 14, 2018 at 10:15:11AM +0200, Andreas Koenig wrote:
> There was no such documentation in perldelta. There was mention of a new I have cited and llinked to the original places which sentences I speak > There is a strong ethos within core development that new optimisations It was not noticed for 13 years. > Note also that the change was back-ported to 5.8.4 - an odd thing to do I'll cite my timeline again from my previous posting: 2004-02-20: https://rt.perl.org/Public/Bug/Display.html?id=25263#txn-79013 Within same RT ticket From: Dave Mitchell Citation: "I've now applied change #22349 to bleed, which makes @a = sort @a act inplace." And one line from man perlhist: 5.8.4 2004-Apr-21 For my reading, this is not a backport, it was properly injected into
> There was nothing to take over.
> There was nothing missing.
> So do you agree that the behaviour which has been present since 5.000 and The changes in RT ticket 25263 got applause for 13 years. I have no
> I'm open to suggestions. First of all I would suggest to document existing behaviour for v5.8.4 -
> You asked me some questions about the meaning of 'in-place'. I pointed Acknowledged. -- |
From @andk
> There was nothing to take over.
> There was nothing missing. In my previous posting I overlooked this very interesting point in perl 5.8.4 introduced (thanks to your effort!) a remarkable user-tunable Let me present figures how remarkable and how user-tunable it was and | | max VmPeak | The point is that it is not only user-visible but actually user-tunable. -- PS: full testrun: % for t in `seq 1 3`; do for p in perl-5.8.3/6fdb perl-5.8.4/6fdb v5.28.0/da1c; do |
From @iabynOn Sun, Jul 22, 2018 at 04:28:36PM +0200, Andreas Koenig wrote:
Most optimisations I do are "user tunable" by that definition. Did you know that if you do $a + $b, and ensure that $a and $b have only I think the position you have taken is extreme, and if we used that Really I am utterly confounded by the stance you are taking. Perl has a As I have said before, I am willing to debate the pragmatic pros and cons -- |
From @andk
> On Sun, Jul 22, 2018 at 04:28:36PM +0200, Andreas Koenig wrote:
> Most optimisations I do are "user tunable" by that definition. > Did you know that if you do $a + $b, and ensure that $a and $b have only We have talked about it already. Perl is open source and as such the > I think the position you have taken is extreme, and if we used that This is without doubt an interesting question. But watch out, there is a > Really I am utterly confounded by the stance you are taking. Perl has a Yes, but breaking stuff on CPAN without careful consideration is also > But we have never committed to maintaining 100% backwards Indeed. That's what I'm asking for: the appropriate amount of > As I have said before, I am willing to debate the pragmatic pros and cons I believe I have made a lot of my points. At least I have digged out the > What I am not prepared to accept, is that digging out a 14 year old Your argument is very hard to parse for me, please explain what your - "digging": is it the fact that I was digging in the history what you - "a 14 year old": is it that the discussion is 14 years old? would it - "at no point proposes that weak references": the ticket said - "perpetuity": what are you arguing against perpetuity? Has a language I repeat my quest: it is time to document existing behaviour for v5.8.4 -- |
Given this was a BBC ticket and the issue was resolved with a revert of the breaking commit, I don't see any further action required here. I have opened #17569 to decide if we want to put the reverted commit back in the future. |
Migrated from rt.perl.org#132142 (status was 'open')
Searchable as RT132142$
The text was updated successfully, but these errors were encountered: