module constant.pm uses +1000 kB in 522 compare to 514 #15154
Comments
From @atoomicThe memory increase is not explained by one single commit, it's probably an addition of few more things. # use perl5.22.1 version of constant
After applying the suggested patch to constant this can be reduced by ~800 kB
Doing a git bisect I could identify this commit as one of the biggest memory increase: Before commit HEAD=bcb8752^
With HEAD=bcb8752
More detailed analysis of memory usage over versions # 5.14.4
# 5.18.1
# 5.18.4
# 5.19.0
# 5.19.6
# 5.19.7
# 5.19.8
# 5.19.9
# 5.19.11
# 5.20.0
# 5.20.1
# 5.22.1
|
From @atoomic0001-Reduce-memory-footprint-of-constant.pm.patchFrom d212aef5d7f91a80e2370fe7149b8c8084cc6285 Mon Sep 17 00:00:00 2001
From: Nicolas Rochelemagne <rochelemagne@cpanel.net>
Date: Wed, 27 Jan 2016 13:23:25 -0600
Subject: [PATCH] Reduce memory footprint of constant.pm
Compare to v5.14 constant.pm increased its memory
footprint by about ~1000 kB.
The increase cannot be explained by one single commit but
we can bisect that the main increase of memory for constant.pm
appears to come from bcb875216f24899d543 (between v5.19.7 and v5.19.8).
The memory increase comes from updates to regcomp.c. We can workaround
this in constant.pm by simplifying the normal_constant_name regexp and
checking the optional leading '_' differently.
Note that we could also use '$altered_name =~ s{^_}{};' instead of using
substr.
This commit is reducing the footprint of constant.pm by ~800 kB.
---
dist/constant/lib/constant.pm | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/dist/constant/lib/constant.pm b/dist/constant/lib/constant.pm
index e4b8fd2..5a28aa7 100644
--- a/dist/constant/lib/constant.pm
+++ b/dist/constant/lib/constant.pm
@@ -17,7 +17,9 @@ my %forced_into_main = map +($_, 1),
my %forbidden = (%keywords, %forced_into_main);
-my $normal_constant_name = qr/^_?[^\W_0-9]\w*\z/;
+# this can also start by an _ but do not add it as part of the regexp
+# use and extra 800 kB by adding ^_? to the regexp
+my $normal_constant_name = qr/^[^\W_0-9]\w*\z/;
my $tolerable = qr/^[A-Za-z_]\w*\z/;
my $boolean = qr/^[01]?\z/;
@@ -95,8 +97,13 @@ sub import {
$pkg = $caller;
}
- # Normal constant name
- if ($name =~ $normal_constant_name and !$forbidden{$name}) {
+ my $altered_name = $name;
+ # optional _ at the start: use substr to save some memory
+ if ( my $first = substr( $name, 0, 1 ) ) {
+ substr( $altered_name, 0, 1, '' ) if $first eq '_';
+ }
+
+ if ($altered_name =~ $normal_constant_name and !$forbidden{$name}) {
# Everything is okay
# Name forced into main, but we're not in main. Fatal.
@@ -119,7 +126,7 @@ sub import {
warnings::warn("Constant name '$name' is " .
"forced into package main::");
}
- }
+ }
# Looks like a boolean
# use constant FRED == fred;
--
2.7.0
|
From @jkeenanOn Wed Jan 27 11:37:43 2016, atoomic wrote:
These numbers have jumped around a bit between perl versions. See attachment; all probes run on the same machine. Thank you very much. -- |
From @jkeenan$ perlbrew use perl-5.10.1 $ perlbrew use perl-5.14.4 $ perlbrew use perl-5.16.3 $ perlbrew use perl-5.18.2 $ perlbrew use perl-5.20.1 $ perlbrew use perl-5.22.0 |
|
The RT System itself - Status changed from 'new' to 'open' |
From @tonycozOn Wed Jan 27 11:37:43 2016, atoomic wrote:
Most of the extra memory use introduced in bcb8752 seems to be in creating inversion lists, test code: PERL_DESTRUCT_LEVEL=2 valgrind --tool=massif --time-unit=B ./perl -Ilib -e '$x =qr/^_?[^\W_0-9]\w*\z/;' From massif at around peak memory, n time(B) total(B) useful-heap(B) extra-heap(B) stacks(B) 27 922,464 824,256 812,102 12,154 0 without the ^_: n time(B) total(B) useful-heap(B) extra-heap(B) stacks(B) 21 168,744 167,192 149,720 17,472 0 in bcb8752^ with the ^_: n time(B) total(B) useful-heap(B) extra-heap(B) stacks(B) 30 121,728 121,024 112,742 8,282 0 Tony |
From @khwilliamsonThanks for reporting this problem. This is not a specific issue for constant.pm, but is due to a change in the perl interpreter itself, and affects other modules besides constant.pm. The better long term solution is to change the interpreter, and leave constant.pm as-is. This will be done for 5.24. If it is necessary to fix constant.pm to not use so much memory when loaded with earlier perl versions, there is a better patch in my opinion, which I can furnish if you desire, for your consideration. -- |
From @atoomicThanks Karl for your answer. I definitely agree fixing the problem as its root sounds better, and glad to hear the 5.24 will address this kind of concerns. Thanks in advance On Wed Feb 03 10:07:40 2016, khw wrote:
|
From @khwilliamsonOn 02/03/2016 11:58 AM, Atoomic via RT wrote:
The problem stems from our handling of Unicode characters, which before In the case of constant.pm, most times the constant will just be ASCII, My patch needs an eval to enable the module to work on perls before when Here are the results on my 64-bit system with DEBUGGING, and no Before attached patch: ./perl -e 'use utf8; use constant π => 4 * atan2(1, 1); print qx{grep After patch: perl -e 'use utf8; use constant PI => 4 * atan2(1, 1); print qx{grep ./perl -e 'use utf8; use constant π => 4 * atan2(1, 1); print qx{grep Running it multiple times gives somewhat different numbers each time,
|
From @khwilliamson0007-constant.pm-lower-memory-use.patchFrom e14f3a24ed48118d85f3c6b0236b74dc0f199a43 Mon Sep 17 00:00:00 2001
From: Karl Williamson <khw@cpan.org>
Date: Wed, 3 Feb 2016 13:41:11 -0700
Subject: [PATCH 7/7] constant.pm lower memory use
---
dist/constant/lib/constant.pm | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/dist/constant/lib/constant.pm b/dist/constant/lib/constant.pm
index e4b8fd2..6a691cc 100644
--- a/dist/constant/lib/constant.pm
+++ b/dist/constant/lib/constant.pm
@@ -17,7 +17,7 @@ my %forced_into_main = map +($_, 1),
my %forbidden = (%keywords, %forced_into_main);
-my $normal_constant_name = qr/^_?[^\W_0-9]\w*\z/;
+my $uncompiled_normal_constant_name = '^_?[^\W_0-9]\w*\z';
my $tolerable = qr/^[A-Za-z_]\w*\z/;
my $boolean = qr/^[01]?\z/;
@@ -95,8 +95,15 @@ sub import {
$pkg = $caller;
}
- # Normal constant name
- if ($name =~ $normal_constant_name and !$forbidden{$name}) {
+ # Normal constant name. Perl 5.20 and 5.22 consumed too much memory
+ # when compiling '$uncompiled_normal_constant_name'. But this can
+ # mostly be avoided by using /a, which we can do if the constant name
+ # is ASCII-only. (/a is available starting in 5.14.)
+ if (($] lt 5.014 || $name =~ /[[:^ascii:]]/)
+ ? $name =~ /$uncompiled_normal_constant_name/
+ : eval "$name =~ /$uncompiled_normal_constant_name/a"
+ and !$forbidden{$name})
+ {
# Everything is okay
# Name forced into main, but we're not in main. Fatal.
--
2.1.4
|
From @ap* Karl Williamson <public@khwilliamson.com> [2016-02-03 21:55]:
That cannot possibly be correct. The eval code string interpolates the N.B.: as long as $name is a valid identifier that’s not defined in the |
From @khwilliamsonOn 02/07/2016 09:09 PM, Aristotle Pagaltzis wrote:
Well, I tested it before I publicized it, and it worked. But you're |
From @atoomicThanks for the suggestion that also works, here is the metrics I have no patch> perl522 -Ilib -e 'use utf8; use constant PI => 4 * atan2(1, 1); print qx{grep VmRSS /proc/$$/status}' |
From @toddrCreated by @toddrWe have discovered a discrepancy with perl 5.22 when non-ascii regexes The programmy @re; map { "foo" =~ $_ } @re;
|
From @tonycozOn Thu Feb 18 15:24:26 2016, TODDR wrote:
This looks like https://rt-archive.perl.org/perl5/Ticket/Display.html?id=127392 |
|
The RT System itself - Status changed from 'new' to 'open' |
From @khwilliamsonThe two tickets for excess memory use have now been consolidated. blead now contains a fix to lower peak memory use. I would appreciate it if the requestors would try blead out on their respective problems and report back. As the commit says (accb436) on my box, For [perl #127392], peak was cut from around 8600kB to 7800kB. |
From @atoomicExcellent ! I did some basic tests for both tickets ** with patch accb436 ** without patch (head=accb436^) On Wed Feb 24 09:32:13 2016, khw wrote:
|
From @khwilliamsonblead now contains some further reductions in memory use. Could you try it now, please -- |
From @toddrOn Sat Feb 27 17:18:56 2016, khw wrote:
Karl, Atoomic and I are backporting and testing these to 5.22.1 actually. Can you provide the commits so we can test them please? If doing so is in any way dangerous, please let us know. |
From @jkeenanOn Sat Feb 27 17:18:56 2016, khw wrote:
See data from various perl versions, including blead, attached. -- |
From @jkeenan$ perlbrew use perl-5.10.1;perl -e 'use utf8; use constant PI => 4 * atan2(1, 1); print qx{grep VmRSS /proc/$$/status}' $ perlbrew use perl-5.14.4;perl -e 'use utf8; use constant PI => 4 * atan2(1, 1); print qx{grep VmRSS /proc/$$/status}' $ perlbrew use perl-5.16.3;perl -e 'use utf8; use constant PI => 4 * atan2(1, 1); print qx{grep VmRSS /proc/$$/status}' $ perlbrew use perl-5.18.4;perl -e 'use utf8; use constant PI => 4 * atan2(1, 1); print qx{grep VmRSS /proc/$$/status}' $ perlbrew use perl-5.20.1;perl -e 'use utf8; use constant PI => 4 * atan2(1, 1); print qx{grep VmRSS /proc/$$/status}' $ perlbrew use perl-5.22.0;perl -e 'use utf8; use constant PI => 4 * atan2(1, 1); print qx{grep VmRSS /proc/$$/status}' $ ./perl -v | head -2 | tail -1
|
From @khwilliamsonOn 02/27/2016 07:15 PM, Todd Rinaldo via RT wrote:
I don't guarantee that these will cleanly apply, but take all patches in and ending with You need not take (but it's trivial): |
From @atoomichere is the list to consider, some minor changes to proto.h are required * 6c0a9fb regcomp.c: Use less peak memory (2 days ago, Karl Williamson) On Sat Feb 27 21:57:05 2016, public@khwilliamson.com wrote:
|
From @atoomicFrom the tests I made, only accb436 seems to reduce the memory for the two tickets here are the one liner used: * with HEAD=e1ad4fb Improve wording of perldelta entry for ae146f5 #127568: \w * with HEAD=accb436 #127568: \w * with HEAD=accb436^=f0c0c5a #127568: \w On Sat Feb 27 21:57:05 2016, public@khwilliamson.com wrote:
|
From @atoomicThanks Karl for your patches, I've backported most of them to 5.22 (only require some minor adjustment in proto.h) Backport on top of 5.22.1 are here: if we are interested to add them to a next 5.22.* release It took me a few minutes to understand that commit af72888 was fixing a segfault when running: af72888 regcomp.c: Improve free-ing up unused inversion list spac Here is a nice sample to prove that we are now saving the memory and avoid the leak after> perl -e 'my On Mon Feb 29 08:49:35 2016, atoomic wrote:
|
From @khwilliamsonOn 03/02/2016 09:48 AM, Atoomic via RT wrote:
These newer numbers look a lot better than the 5% savings earlier given.
I did not know that. I just saw that the code didn't look right. I'll
|
From @atoomicno need to add some tests the segfault test I provided is already part of the testsuite and indeed this was only triggered using some configure options we can close this ticket On Wed Mar 02 09:55:08 2016, public@khwilliamson.com wrote:
|
From @khwilliamsonOn 03/02/2016 01:56 PM, Atoomic via RT wrote:
It occurred to me afterwards that there was desire to patch constant.pm So what should be done, and by whom? |
From @toddrOn Wed Mar 02 15:05:08 2016, public@khwilliamson.com wrote:
I'm inclined to say the bug was in Perl not core so patching constant is inappropriate. It's been broken since 5.16 and nobody has noticed till now right? As for whether it should be officially backported to 11.52, I'm I suspect backport criteria would exclude these patches unless it really was a memory leak? At this point I'm unclear if it was just unnecessary use of memory that was later freed or if it actually is a memory leak. |
From @khwilliamsonOn 03/02/2016 04:08 PM, Todd Rinaldo via RT wrote:
It was not a memory leak. |
From @toddrOn Wed Mar 02 15:08:54 2016, TODDR wrote:
Apologies I meant. Perl not constant |
From @khwilliamsonClosed with the approval of the OP |
|
@khwilliamson - Status changed from 'open' to 'pending release' |
From @khwilliamsonThank you for submitting this report. You have helped make Perl better. Perl 5.24.0 may be downloaded via https://metacpan.org/release/RJBS/perl-5.24.0 |
|
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#127392 (status was 'resolved')
Searchable as RT127392$
The text was updated successfully, but these errors were encountered: