Blead Breaks CPAN: Role::Tiny, Variable::Magic, Moo #16191
Comments
From @simcop2387Somewhere between e04fc1a and e92359c, Role::Tiny and Variable::Magic Reported by HAARG also, Moo has failures of it's own with this change, one --- BEGIN perl -V --- Linker and Libraries: Characteristics of this binary (from libperl): PERLBREW_MANPATH="/home/ryan/perl5/perlbrew/perls/perlbot-blead-2017-10-09_12776/man" PERLBREW_PATH="/home/ryan/perl5/perlbrew/bin:/home/ryan/perl5/perlbrew/perls/perlbot-blead-2017-10-09_12776/bin" /home/ryan/perl5/perlbrew/perls/perlbot-blead-2017-10-09_12776/lib/site_perl/5.27.5/x86_64-linux /home/ryan/perl5/perlbrew/perls/perlbot-blead-2017-10-09_12776/lib/site_perl/5.27.5 /home/ryan/perl5/perlbrew/perls/perlbot-blead-2017-10-09_12776/lib/5.27.5/x86_64-linux /home/ryan/perl5/perlbrew/perls/perlbot-blead-2017-10-09_12776/lib/5.27.5 |
From @cpansproutOn Mon, 09 Oct 2017 08:06:46 -0700, simcop2387@simcop2387.info wrote:
Very likely. I was expecting some fallout.
-- Father Chrysostomos |
|
The RT System itself - Status changed from 'new' to 'open' |
From @haargSome additional failures due to the same change. |
From @haarg |
From @haarg |
From @haarg |
From @haarg |
From @simcop2387Also breaks autovivification On Tue, Oct 10, 2017 at 2:16 AM, Graham Knop via RT <
|
From @simcop2387 |
From @andk
> Very likely. I was expecting some fallout. What are the plans how to proceed? Shall this go into 5.27.5? I mean, is -- |
From @cpansproutOn Sat, 14 Oct 2017 23:48:08 -0700, andreas.koenig.7os6VVqR@franz.ak.mind.de wrote:
I am hoping, when I get time on the weekends (like today), to start patching the CPAN modules one by one. We still have many months to go before the stable release. But if the pumpking wants to back out the change for now, I have no objection (except that it may make it harder to get the patches accepted--I really do want this change in before 5.28). -- Father Chrysostomos |
From @jkeenanOn Sun, 15 Oct 2017 15:17:03 GMT, sprout wrote:
Father C, Can you describe the purpose of the branch you merged on Oct 08? Given the fact that the libraries which we already know have been broken are rather high up on the CPAN river, there is a greater than normal likelihood that libraries farther down the river and darkpan code will also break. If so, then we need to assess the costs and benefits of the merge before proceeding. Thank you very much. -- |
From @xsawyerxOn 10/15/2017 10:07 PM, James E Keenan via RT wrote:
An additional temporary problem is that this makes it difficult for CPAN I think it's better to temporarily revert this just until we can resolve it. Father C., if you can share your patch, others could help with the |
From @cpansproutOn Sun, 15 Oct 2017 13:07:56 -0700, jkeenan wrote:
It saves a large amount of memory. Before this change, subroutines in packages (the most common use of package symbols) resided in typeglobs. So ‘package Foo; sub foo{}’ would be stored as *foo{CODE} in the *foo typeglob in the Foo package. I.e., $Foo::{foo} was a typeglob with its CODE slot pointing to something. The new behaviour is that, instead of a typeglob, we have a scalar with a reference to the subroutine. So now $Foo::{foo} is a coderef. Since an RV (a reference) takes 24 bytes of memory whereas a GV (typeglob) takes 24 (sv head) + 48 (xpvgv body) + 80 (gp) = 152 bytes, plus a little malloc overhead for the gp struct, which I think is 4 to 6 bytes on most platforms, so the total is about 157 bytes. That’s 133 bytes extra per subroutine. If you load five modules each with 100 subroutines, you’ve just used 65 K more ram than you needed to. (These are all 64-bit numbers. The 32-bit numbers differ, but the GV will always be bigger than the RV.) Granted, method calls will undo the optimization (caching the method in the typeglob, trading memory for speed), but this only happens if a particular method actually gets called. Hence, small scripts that load large libraries will see the biggest reduction in memory usage. Large object-oriented systems that call methods for everything see the smallest reduction in memory usage.
If we are to revert the behaviour, it is only this hunk from 6881372 that needs to be reverted: @@ -8583,7 +8590,7 @@ Perl_newATTRSUB_x(pTHX_ I32 floor, OP *o, OP *proto, OP *attrs, and whatever tests end up failing need to be marked TODO. -- Father Chrysostomos |
From @jkeenanOn Mon, 16 Oct 2017 16:16:18 GMT, xsawyerx@gmail.com wrote:
I also think that would be the best course of action. Father C could fork HEAD to a branch just before reverting. Ongoing work could then take place in that branch and, at a suitable point, we could install it somewhere for smoking the failing libraries and the upper parts of the CPAN river.
Since this code had no FAILs within the CPAN test suite -- only downstream on CPAN. I think we need to look at the code on CPAN which broke to write tests which we can then include in the Perl 5 test suite to head off such failures in the future. Thank you very much. -- |
From @shlomifOn Tue, 17 Oct 2017 06:55:11 -0700
Sounds like a good course of action.
I also think this will be a good idea.
-- Shlomi Fish http://www.shlomifish.org/ Every successful open source project will eventually spawn a sub‐project. Please reply to list if it's a mailing list post - http://shlom.in/reply . |
From @haargOn Sun, Oct 15, 2017 at 5:17 PM, Father Chrysostomos via RT
I have no particular opinion about if this patch should be backed out,
|
From @cpansproutOn Tue, 17 Oct 2017 15:16:46 -0700, haarg wrote:
I have just submitted a patch for Variable::Magic: https://rt.cpan.org/Ticket/Display.html?id=123314 I am very short on time till the weekend. Would someone else have a chance to revert the hunk I cited? -- Father Chrysostomos |
From @xsawyerxOn 10/17/2017 03:55 PM, James E Keenan via RT wrote:
I like this idea. |
From @jkeenanOn 10/18/2017 05:43 AM, Sawyer X wrote:
Sawyer, I think it will have to be your call to either (a) do the above, which https://www.nntp.perl.org/group/perl.perl5.porters/2017/10/msg246738.html (a) has the disadvantage that it puts us into the murkiness described in Thank you very much. |
From @jkeenanOn Wed, 18 Oct 2017 03:04:52 GMT, sprout wrote:
Father C made these commits: ##### Deparse.t tweak commit 834e1dc perldelta for 1406232 commit 6eed25e Temporarily revert CV-in-stash optimisation I installed blead for testing, then installed cpanm. I then ran the following: ##### All these modules installed with no test failures. But I think the ticket should stay open for a while to see if any other distros had the BBC. Thank you very much. -- |
From @cpansproutOn Mon, 16 Oct 2017 09:16:18 -0700, xsawyerx@gmail.com wrote:
Thank you. For starters, I have just submitted a patch to fix Log::Trace’s bug #49409: https://rt.cpan.org/Ticket/Display.html?id=49409 Log::Trace fails to handle the new optimization for the same reason that it has always failed with constants (see the ticket). The bug wrt constants was reported eight years ago, and Log::Trace last saw a new release twelve years ago. This is an unfortunate situation for a module depended on by so many things. -- Father Chrysostomos |
From @xsawyerxOn 10/18/2017 02:13 PM, James E Keenan wrote:
I had asked to temporarily revert it. |
From @cpansproutOn Thu, 26 Oct 2017 08:06:54 -0700, xsawyerx@gmail.com wrote:
All the modules listed in this ticket now have patches: Haarg says in https://rt-archive.perl.org/perl5/Ticket/Display.html?id=132252#txn-1500347 that he has prepared patches for Moo and Role::Tiny. Log::Trace: https://rt.cpan.org/Ticket/Display.html?id=49409 Variable::Magic: https://rt.cpan.org/Ticket/Display.html?id=123314 Test::API: dagolden/Test-API#6 Type::Tiny: https://rt.cpan.org/Ticket/Display.html?id=123408 indirect: https://rt.cpan.org/Ticket/Display.html?id=123374 autovivification: https://rt.cpan.org/Ticket/Display.html?id=123411
I this case it doesn’t really apply. In fact, most of the modules are already buggy, because they don’t take into account that, since 5.6 or so, stash elements have contained things other than typeglobs. In some instances I wrote new tests for the modules that got them to fail with existing perl versions. The fix to work with the optimisation in these cases was the same as the fix to work with constants in earlier perl versions.
I reverted to the old behaviour in commit 6eed25e and have now pushed a new sprout/cv-in-stash branch, based on current blead (6e8135a), with the revert reverted. At this point, how do we proceed? -- Father Chrysostomos |
From @cpansproutOn Sun, 29 Oct 2017 11:40:14 -0700, sprout wrote:
Except for Cpanel::JSON::XS, which was failing with my patch because the core function get_cvn_flags was not coping with subrefs when called the way that module calls it. This I have fixed and tested in a385812. -- Father Chrysostomos |
From @dur-randir2017-11-01 4:00 GMT+03:00 Father Chrysostomos via RT <perlbug-followup@perl.org>:
This commit introduced a new build warning, as follows: perl.c:2708:9: warning: incompatible pointer types returning 'SV *' (aka 'struct sv *') from a function with result type 'CV *' (aka 'struct cv *') Best regards, |
From @haargThis seems like lot of breakage for such a tiny benefit. Can this change be reverted? Additionally, since the stash entries get de-optimized to globs when called as methods, this change will actually lead to increased memory usage on servers that fork. Many methods won't get called until post-fork, so the globs will need to be created for every fork instead of being able to benefit from copy-on-write. |
From @dur-randirOn Mon, 29 Jan 2018 05:51:38 -0800, haarg wrote:
All those modules are already broken on the current perl version (and some versions back in time), they just don't test for it - as globs are already not created for subs in the 'main' package.
You can't rely on copy-on-write for almost all non-optree data (and sometimes even on optree too), as any run-time SV upgrade/free will touch the whole page. Furthermore, there're constant subs (sub foo () {42}), which are almost never called as methods - and by reverting this we will loose memory savings on them. |
From @haargOn Mon, Jan 29, 2018 at 6:01 PM, Sergey Aleynikov via RT
They are "broken" in a way that nobody has noticed for a long time.
The actual memory savings is minuscule. I'm not sure what you are |
From @eserteAnother one with "Not a GLOB reference at": DAGOLDEN/Object-LocalVars-0.21.tar.gz |
From @eserteHere's a list of distributions which have an old ExtUtils::Installer bundled. "perl Makefile.PL" fails for these with a "Not a GLOB reference" error. * AUTRIJUS/LWP-Authen-Wsse-0.05.tar.gz |
From @iabynOn Tue, Jan 30, 2018 at 10:39:30PM -0800, slaven@rezic.de via RT wrote:
I feel that we have too much breakage too late in the release cycle, -- |
From @eserteFor the sake of completeness, here more distributions where "Not a GLOB reference at ..." appears in the failing test logs: * HKOBA/YATT-Lite-0.101.tar.gz The following distributions cannot be built anymore because of an old bundled inc/Module/Install.pm: * CFRANKS/HTML-Menu-Select-1.01.tar.gz |
From @cpansproutOn Mon, 29 Jan 2018 21:31:31 -0800, haarg wrote:
I will point out that most of the module I patched already fail with constants for the same reason. I.e., we have uncovered existing bugs in many modules. I do agree, though, that this amount of breakage warrants a revert. Would you be adverse to reënabling the optimisation in blead after 5.28?
(I hope you will respond to Mr. Aleynikov’s point about copy-on-write. Such programs also suffer from modules that do autoloading.) -- Father Chrysostomos |
From @eserteDana Sat, 09 Dec 2017 02:00:58 -0800, andreas.koenig.7os6VVqR@franz.ak.mind.de reče:
Fix for Test-Sims: schwern/Test-Sims#3 |
From @jkeenanOn 01/31/2018 01:39 AM, slaven@rezic.de via RT wrote:
Spot-checking these, I find 'use inc::Module::Install;' -- but I don't
|
From @jkeenanOn Thu, 01 Feb 2018 20:39:43 GMT, slaven@rezic.de wrote:
Spot-checking these, I find that none has had a new release since 2006. Am I correct in thinking that, to keep these distros alive, the best advice to the authors would be, "Move off Module::Install"? Thank you very much. -- |
From @demerphqOn 29 Jan 2018 18:01, "Sergey Aleynikov via RT" <perlbug-followup@perl.org> On Mon, 29 Jan 2018 05:51:38 -0800, haarg wrote:
All those modules are already broken on the current perl version (and some
You can't rely on copy-on-write for almost all non-optree data (and It will touch a whole page of Sv *heads*, which are slab allocated anyway. So I don't follow your point. Furthermore , there're constant subs (sub foo () {42}), which are almost never called That is a piffle compared to the methods that will get forked. If we keep Yves |
From @eserteDana Fri, 02 Feb 2018 09:26:57 -0800, jkeenan@pobox.com reče:
Sorry, I really meant Module::Install, not ExtUtils::Installer.
|
From @xsawyerxI'm late here (had a serious move over the week) but I've already The problem is not the conversation about which way is more optimized, It should be reverted and we can reapproach it next release. On 31 January 2018 at 20:29, Dave Mitchell <davem@iabyn.com> wrote:
|
From @cpansproutOn Sun, 04 Feb 2018 01:51:53 -0800, xsawyerx@gmail.com wrote:
I have disabled it in 1e2cfe1. -- Father Chrysostomos |
From @xsawyerxOn 02/04/2018 10:22 PM, Father Chrysostomos via RT wrote:
Thank you. |
From @iabynOn Sun, Feb 04, 2018 at 12:22:31PM -0800, Father Chrysostomos via RT wrote:
I'll remove this ticket from the 5.28 blockers list; -- |
p5pRT
added
BBC
|
Given this was reverted, I assume the case can now be closed. If we want to put the optimization back, maybe we're lacking an issue to track that? |
toddr
added
Closable?
This "optimization" caused a considerable amount of CPAN breakage and a lot of discussion which has to be reviewed whenever anyone looks at this ticket. So if there is anyone interested in pursuing a revival of that work, they should do so in a fork or a branch. Closing. Thank you very much. |
jkeenan
removed
the
Closable?
|
This is a good time if anyone believes this optimization is valuable enough to open the discussion. (This is what it's really about.) |
|
On Mon, Feb 17, 2020 at 11:29:47PM -0800, Sawyer X wrote:
This is a good time if anyone believes this optimization is valuable enough to open the discussion. (This is what it's really about.)
Well it saves ~150 bytes of memory per defined (rather than just declared)
sub. But since subs tend to be large users of memory anyway (optree,
padtmps etc) I think the relative saving per sub is small.
…--
The optimist believes that he lives in the best of all possible worlds.
As does the pessimist.
|
Do you mean that broken modules should not be fixed just because of there's a lot of them? |
|
I appreciate the enormous amount of effort @cpansprout went to in fixing broken modules. I feel we'd benefit from having this patch if it isn't actively pessimal, since it would expose those existing bugs and help avoid the same mistake being made by future CPAN and darkPAN authors - but if we wanted to roll it out again, it seems likely there would still be a lot more such fixing to do, and that's unlikely to happen unless it has a champion willing to do much of the work. But I agree with Jim that any such work would be best started in a branch, at least to the point that enough of the upstream dependencies have fixes to allow us to get a reasonably complete picture of the size of the remaining downstream problem. Hugo |
|
While I'm a bit dubious of this change in general, the high impact CPAN dists have been fixed, so it hopefully wouldn't impede general smoke testing of CPAN. |
Migrated from rt.perl.org#132252 (status was 'open')
Searchable as RT132252$
The text was updated successfully, but these errors were encountered: