Reduce SelectSaver memory footprint #15595
Comments
From @atoomicCreated by @atoomicThis is saving about 500k when using SelectSaver before #perl -I. -e 'require q{lib/SelectSaver.pm}; print qx{grep VmRSS after #perl -I. -e 'require q{lib/SelectSaver.pm}; print qx{grep VmRSS Perl Info |
From @atoomic0001-Reduce-SelectSaver-memory-footprint.patchFrom 98e22f59f3ad7a333774b06ef5929d382343e509 Mon Sep 17 00:00:00 2001
From: Nicolas R <atoomic@cpan.org>
Date: Thu, 8 Sep 2016 14:29:46 -0600
Subject: [PATCH] Reduce SelectSaver memory footprint
This is saving about 500k when using SelectSaver
as most of the time Carp is not required.
before> perl -I. -e 'require q{lib/SelectSaver.pm}; print qx{grep VmRSS /proc/$$/status}'
VmRSS: 2920 kB
after> perl -I. -e 'require q{lib/SelectSaver.pm}; print qx{grep VmRSS /proc/$$/status}'
VmRSS: 2352 kB
diff --git a/lib/SelectSaver.pm b/lib/SelectSaver.pm
index b67adff..19cd764 100644
--- a/lib/SelectSaver.pm
+++ b/lib/SelectSaver.pm
@@ -35,11 +35,10 @@ that was selected when it was created.
=cut
require 5.000;
-use Carp;
-use Symbol;
+use Symbol q{qualify};
sub new {
- @_ >= 1 && @_ <= 2 or croak 'usage: SelectSaver->new( [FILEHANDLE] )';
+ @_ >= 1 && @_ <= 2 or do { require Carp; Carp::croak('usage: SelectSaver->new( [FILEHANDLE] )') };
my $fh = select;
my $self = bless \$fh, $_[0];
select qualify($_[1], caller) if @_ > 1;
--
2.10.0
|
From @jkeenanOn Thu Sep 08 14:02:41 2016, atoomic@cpan.org wrote:
According to https://en.wikipedia.org/wiki/Procfs, "Many Unix-like operating systems support the proc filesystem ...." That implies that there are OSes -- Unix-like and not -- which do not support /proc. How would you measure the benefit of your patch on such OSes? Smoke-testing in branch: Thank you very much. -- |
|
The RT System itself - Status changed from 'new' to 'open' |
From @atoomicIndeed the one liner I provided just gives a metric on some systems, perl can also run on different OS (windows included) where this metric cannot be gathered using this method. I do not think knowing the exact value on all system matters. I would think it's sane to assume that when reducing dependencies the memory is reduced on all/most of the systems. Let me know if you really want to know this metric on a specific system, or have some concerns that this might increase memory on others. thanks for considering this patch sincerely On Sun Sep 11 08:34:48 2016, jkeenan wrote:
|
From @jkeenanOn Sun Sep 11 09:00:14 2016, atoomic wrote:
Well, in the meantime I discovered that this works on both Linux and FreeBSD: ##### I am taking this patch for the purpose of applying it to blead within 7 days unless someone lodges an objection. Thank you very much. -- |
From @demerphqNo objections really, but it seems a weird thing to optimise. What Yves On 11 September 2016 at 18:46, James E Keenan via RT
-- |
From @atoomicindeed this seems pointless as [m]any scripts will quickly load Carp and import subs from it I agree, not everyone could take benefits from this patch, but this is probably not a bad pattern, On Tue Sep 13 09:49:39 2016, demerphq wrote:
|
From @khwilliamsonOn 09/13/2016 10:56 AM, Atoomic via RT wrote:
I have seen this pattern elsewhere in core, but don't remember where.
|
From @xsawyerxOn 09/13/2016 06:56 PM, Atoomic via RT wrote:
I understand the need for this, and it make sense. My first impulse is package LazyCarp { sub carp { Your code: use LazyCarp; LazyCarp::carp(...); But then, I would want to generalize this too: package Lazy { sub run { Your code is then: use Lazy; It's a bit uglier because this is naive simple code that I wrote in 2 Some benchmarks: $ perl -d:SizeMe -e1 $ perl -d:SizeMe -MLazyCarp -e1 $ perl -d:SizeMe -MLazy -e1 $ perl -d:SizeMe -MCarp -e1 (This is perl 5, version 18, subversion 2 (v5.18.2) built for There's are some stuff on CPAN that does something similar, such as So now I'm wondering if we should be discussing a general solution to |
From @kentfredricOn 14 September 2016 at 21:43, Sawyer X <xsawyerx@gmail.com> wrote:
A long time ago in a galaxy less far away ( Perl 5.10 ), Carp itself https://metacpan.org/source/RGARCIA/perl-5.10.0/lib/Carp.pm -- KENTNL - https://metacpan.org/author/KENTNL |
From @demerphqOn 14 Sep 2016 06:05, "Kent Fredric" <kentfredric@gmail.com> wrote:
Why did that change? Why not restore that functionality instead of all Yves |
From @LeontOn Wed, Sep 14, 2016 at 11:43 AM, Sawyer X <xsawyerx@gmail.com> wrote:
I think autouse is what you want. Leon |
From @xsawyerxOn Wed, Sep 14, 2016 at 1:40 PM, Leon Timmermans <fawaka@gmail.com> wrote:
D'oh. Yes. :) |
From @cpansproutOn Wed Sep 14 04:00:37 2016, demerphq wrote:
Because when you are in the middle of processing an error, you may not be able to load anything. I seem to remember the same bug coming up multiple times, which may even have involved crashing, and which was difficult to debug. Sorry, I don’t remember more details than that. -- Father Chrysostomos |
From @rgarciaOn 14 September 2016 at 12:59, demerphq <demerphq@gmail.com> wrote:
Details there : https://rt.perl.org/Public/Bug/Display.html?id=42329 |
From @jkeenanThere has been no further discussion in this RT since Sep 15. While there was a lot of discussion of larger issues, no one picked up the ball and ran with it. So, so as not to leave the OP hanging, I applied the patch with committer's corrections in commit 26d58bf. Marking ticket Resolved. Discussion of larger issues (e.g., lazy loading in general) should be taken to p5p list. Thank you very much. |
|
@jkeenan - Status changed from 'open' to 'resolved' |
From @xsawyerxOn Fri Oct 07 14:02:46 2016, jkeenan wrote:
I think this should be reverted. Carp.pm is required to report errors. If you cannot load Carp, you will not be able to lazily load Carp, which means you cannot load the error module to report the error of loading a module. It's a circular problem. To repeat rgs' comments above: If you, for example, run out of file descriptors, you will not be able to load additional modules. This means that if SelectSaver.pm has an error in new(), it will not be able to load Carp and report it. Modules for reporting errors should be available when you need to load errors, unless you can be assured that you will be able to load them. I would be happy if there could be more discussion on that point, but it fell through the cracks. Therefore, I would like this ticket reopened, the patch reverted, and - if the patch submitter is still interested - an explanation addressing this point of problem still standing. Thanks! :) |
|
@jkeenan - Status changed from 'resolved' to 'open' |
|
@iabyn - Status changed from 'open' to 'rejected' |
Migrated from rt.perl.org#129235 (status was 'rejected')
Searchable as RT129235$
The text was updated successfully, but these errors were encountered: