Closed p5pRT closed 7 years ago
p5pRT
commented
8 years ago 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
p5pRT
commented
8 years ago p5pRT
commented
8 years ago On Thu Sep 08 14:02:41 2016\, atoomic@cpan.org wrote:
This is a bug report for perl from atoomic@cpan.org\, generated with the help of perlbug 1.40 running under perl 5.25.5.
----------------------------------------------------------------- [Please describe your issue here]
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
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: smoke-me/jkeenan/atoomic/129235-selectsaver
Thank you very much.
-- James E Keenan (jkeenan@cpan.org)
p5pRT
commented
8 years ago The RT System itself - Status changed from 'new' to 'open'
p5pRT
commented
8 years ago Indeed 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 nicolas
On Sun Sep 11 08:34:48 2016\, jkeenan wrote:
On Thu Sep 08 14:02:41 2016\, atoomic@cpan.org wrote:
This is a bug report for perl from atoomic@cpan.org\, generated with the help of perlbug 1.40 running under perl 5.25.5.
----------------------------------------------------------------- [Please describe your issue here]
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
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: smoke-me/jkeenan/atoomic/129235-selectsaver
Thank you very much.
p5pRT
commented
8 years ago On Sun Sep 11 09:00:14 2016\, atoomic wrote:
Indeed 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 nicolas
Well\, in the meantime I discovered that this works on both Linux and FreeBSD:
##### $ ./perl -Ilib -e 'require q{lib/SelectSaver.pm}; print qq|$$\n|; print qx{ps -o rss $$ | tail -1}' 17494 4984 #####
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.
-- James E Keenan (jkeenan@cpan.org)
p5pRT
commented
8 years ago No objections really\, but it seems a weird thing to optimise. What non-toy code is going to run without Carp?
Yves
On 11 September 2016 at 18:46\, James E Keenan via RT \perlbug\-followup@​perl\.org wrote:
On Sun Sep 11 09:00:14 2016\, atoomic wrote:
Indeed 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 nicolas
Well\, in the meantime I discovered that this works on both Linux and FreeBSD:
##### $ ./perl -Ilib -e 'require q{lib/SelectSaver.pm}; print qq|$$\n|; print qx{ps -o rss $$ | tail -1}' 17494 4984 #####
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.
-- James E Keenan (jkeenan@cpan.org)
--- via perlbug: queue: perl5 status: open https://rt-archive.perl.org/perl5/Ticket/Display.html?id=129235
-- perl -Mre=debug -e "/just|another|perl|hacker/"
p5pRT
commented
8 years ago indeed this seems pointless as [m]any scripts will quickly load Carp and import subs from it but at cPanel we use several scripts where we try to optimize / reduce their memory footprint\, and for now Carp is in our blacklist\, or at least we only load it on demand when required
I agree\, not everyone could take benefits from this patch\, but this is probably not a bad pattern\, (outside this might fail differently if you cannot open the file when you need to lazy load it\, but if it happens\, you are already in a /very/ bad state\, no ?)
On Tue Sep 13 09:49:39 2016\, demerphq wrote:
No objections really\, but it seems a weird thing to optimise. What non-toy code is going to run without Carp?
Yves
On 11 September 2016 at 18:46\, James E Keenan via RT \perlbug\-followup@​perl\.org wrote:
On Sun Sep 11 09:00:14 2016\, atoomic wrote:
Indeed 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 nicolas
Well\, in the meantime I discovered that this works on both Linux and FreeBSD:
##### $ ./perl -Ilib -e 'require q{lib/SelectSaver.pm}; print qq|$$\n|; print qx{ps -o rss $$ | tail -1}' 17494 4984 #####
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.
-- James E Keenan (jkeenan@cpan.org)
--- via perlbug: queue: perl5 status: open https://rt-archive.perl.org/perl5/Ticket/Display.html?id=129235
p5pRT
commented
8 years ago On 09/13/2016 10:56 AM\, Atoomic via RT wrote:
indeed this seems pointless as [m]any scripts will quickly load Carp and import subs from it but at cPanel we use several scripts where we try to optimize / reduce their memory footprint\, and for now Carp is in our blacklist\, or at least we only load it on demand when required
I agree\, not everyone could take benefits from this patch\, but this is probably not a bad pattern\, (outside this might fail differently if you cannot open the file when you need to lazy load it\, but if it happens\, you are already in a /very/ bad state\, no ?)
I have seen this pattern elsewhere in core\, but don't remember where. It seems reasonable to me.
On Tue Sep 13 09:49:39 2016\, demerphq wrote:
No objections really\, but it seems a weird thing to optimise. What non-toy code is going to run without Carp?
Yves
On 11 September 2016 at 18:46\, James E Keenan via RT \perlbug\-followup@​perl\.org wrote:
On Sun Sep 11 09:00:14 2016\, atoomic wrote:
Indeed 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 nicolas
Well\, in the meantime I discovered that this works on both Linux and FreeBSD:
##### $ ./perl -Ilib -e 'require q{lib/SelectSaver.pm}; print qq|$$\n|; print qx{ps -o rss $$ | tail -1}' 17494 4984 #####
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.
-- James E Keenan (jkeenan@cpan.org)
--- via perlbug: queue: perl5 status: open https://rt-archive.perl.org/perl5/Ticket/Display.html?id=129235
--- via perlbug: queue: perl5 status: open https://rt-archive.perl.org/perl5/Ticket/Display.html?id=129235
p5pRT
commented
8 years ago On 09/13/2016 06:56 PM\, Atoomic via RT wrote:
indeed this seems pointless as [m]any scripts will quickly load Carp and import subs from it but at cPanel we use several scripts where we try to optimize / reduce their memory footprint\, and for now Carp is in our blacklist\, or at least we only load it on demand when required
I agree\, not everyone could take benefits from this patch\, but this is probably not a bad pattern\, (outside this might fail differently if you cannot open the file when you need to lazy load it\, but if it happens\, you are already in a /very/ bad state\, no ?)
I understand the need for this\, and it make sense. My first impulse is "that can't be the only place where Carp is loaded". You would need to track where else this happens (which I assume you are). This can lead to multiple similar patches that do this thing. Why not generalize it?
package LazyCarp { use strict; use warnings;
sub carp { require Carp; goto &Carp::carp; } }
Your code:
use LazyCarp;
LazyCarp::carp(...);
But then\, I would want to generalize this too:
package Lazy { use strict; use warnings;
sub run { my ( $module\, $sub_name ) = splice @_\, 0\, 2; require Module::Runtime; Module::Runtime::use_module($module); my $sub = $module . '::' . $sub_name; goto &{$sub}; } }
Your code is then:
use Lazy; Lazy::run( "Carp"\, "carp"\, ... );
It's a bit uglier because this is naive simple code that I wrote in 2 minutes while starting my day\, but it's beginning to look interesting.
Some benchmarks:
$ perl -d:SizeMe -e1 Total size 510.6KB spread over 14977 nodes [lines=41910 bytes=522899 write=7760.00s]
$ perl -d:SizeMe -MLazyCarp -e1 Total size 512.6KB spread over 15117 nodes [lines=42249 bytes=524929 write=8579.00s]
$ perl -d:SizeMe -MLazy -e1 Total size 515.7KB spread over 15232 nodes [lines=42605 bytes=528067 write=9499.00s]
$ perl -d:SizeMe -MCarp -e1 Total size 656.8KB spread over 18366 nodes [lines=52618 bytes=672601 write=7916.00s]
(This is perl 5\, version 18\, subversion 2 (v5.18.2) built for x86_64-linux-gnu-thread-multi)
There's are some stuff on CPAN that does something similar\, such as Class::LazyLoad\, but I haven't checked them.
So now I'm wondering if we should be discussing a general solution to lazily load a class and run a method on it\, which we could apply to the various core pieces that load heavy things. On the other hand\, of course\, there's delaying the heavy loading in the modules themselves (such as Carp) instead of patching every usage of them.
p5pRT
commented
8 years ago On 14 September 2016 at 21:43\, Sawyer X \xsawyerx@​gmail\.com wrote:
So now I'm wondering if we should be discussing a general solution to lazily load a class and run a method on it\, which we could apply to the various core pieces that load heavy things. On the other hand\, of course\, there's delaying the heavy loading in the modules themselves (such as Carp) instead of patching every usage of them.
A long time ago in a galaxy less far away ( Perl 5.10 )\, Carp itself was a lazy loader.
https://metacpan.org/source/RGARCIA/perl-5.10.0/lib/Carp.pm
-- Kent
KENTNL - https://metacpan.org/author/KENTNL
p5pRT
commented
8 years ago On 14 Sep 2016 06:05\, "Kent Fredric" \kentfredric@​gmail\.com wrote:
On 14 September 2016 at 21:43\, Sawyer X \xsawyerx@​gmail\.com wrote:
So now I'm wondering if we should be discussing a general solution to lazily load a class and run a method on it\, which we could apply to the various core pieces that load heavy things. On the other hand\, of course\, there's delaying the heavy loading in the modules themselves (such as Carp) instead of patching every usage of them.
A long time ago in a galaxy less far away ( Perl 5.10 )\, Carp itself was a lazy loader.
Why did that change? Why not restore that functionality instead of all these tricks in caller modules. Imo that is a codesmell itself.
Yves
p5pRT
commented
8 years ago On Wed\, Sep 14\, 2016 at 11:43 AM\, Sawyer X \xsawyerx@​gmail\.com wrote:
So now I'm wondering if we should be discussing a general solution to lazily load a class and run a method on it\, which we could apply to the various core pieces that load heavy things. On the other hand\, of course\, there's delaying the heavy loading in the modules themselves (such as Carp) instead of patching every usage of them.
I think autouse is what you want.
Leon
p5pRT
commented
8 years ago On Wed\, Sep 14\, 2016 at 1:40 PM\, Leon Timmermans \fawaka@​gmail\.com wrote:
On Wed\, Sep 14\, 2016 at 11:43 AM\, Sawyer X \xsawyerx@​gmail\.com wrote:
So now I'm wondering if we should be discussing a general solution to lazily load a class and run a method on it\, which we could apply to the various core pieces that load heavy things. On the other hand\, of course\, there's delaying the heavy loading in the modules themselves (such as Carp) instead of patching every usage of them.
I think autouse is what you want.
D'oh.
Yes. :)
p5pRT
commented
8 years ago On Wed Sep 14 04:00:37 2016\, demerphq wrote:
On 14 Sep 2016 06:05\, "Kent Fredric" \kentfredric@​gmail\.com wrote:
On 14 September 2016 at 21:43\, Sawyer X \xsawyerx@​gmail\.com wrote:
So now I'm wondering if we should be discussing a general solution to lazily load a class and run a method on it\, which we could apply to the various core pieces that load heavy things. On the other hand\, of course\, there's delaying the heavy loading in the modules themselves (such as Carp) instead of patching every usage of them.
A long time ago in a galaxy less far away ( Perl 5.10 )\, Carp itself was a lazy loader.
Why did that change?
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
p5pRT
commented
8 years ago On 14 September 2016 at 12:59\, demerphq \demerphq@​gmail\.com wrote:
On 14 Sep 2016 06:05\, "Kent Fredric" \kentfredric@​gmail\.com wrote:
On 14 September 2016 at 21:43\, Sawyer X \xsawyerx@​gmail\.com wrote:
So now I'm wondering if we should be discussing a general solution to lazily load a class and run a method on it\, which we could apply to the various core pieces that load heavy things. On the other hand\, of course\, there's delaying the heavy loading in the modules themselves (such as Carp) instead of patching every usage of them.
A long time ago in a galaxy less far away ( Perl 5.10 )\, Carp itself was a lazy loader.
Why did that change? Why not restore that functionality instead of all these tricks in caller modules. Imo that is a codesmell itself.
Details there : https://rt.perl.org/Public/Bug/Display.html?id=42329 TL;DR ; if you're running out of file descriptors you won't be able to load any more module\, and error-throwing modules should be loaded anyway.
p5pRT
commented
8 years ago There 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 26d58bfed57736ec1e1f1dfd579484f8b6fcccd7.
Marking ticket Resolved. Discussion of larger issues (e.g.\, lazy loading in general) should be taken to p5p list.
Thank you very much. -- James E Keenan (jkeenan@cpan.org)
p5pRT
commented
8 years ago @jkeenan - Status changed from 'open' to 'resolved'
p5pRT
commented
8 years ago On Fri Oct 07 14:02:46 2016\, jkeenan wrote:
There 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 26d58bfed57736ec1e1f1dfd579484f8b6fcccd7.
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! :)
p5pRT
commented
8 years ago Reverted in commit 11a12be6c2df2fcd9601f9fde8c7df8c7538c956 -- James E Keenan (jkeenan@cpan.org)
p5pRT
commented
8 years ago @jkeenan - Status changed from 'resolved' to 'open'
p5pRT
commented
7 years ago @iabyn - Status changed from 'open' to 'rejected'
Migrated from rt.perl.org#129235 (status was 'rejected')
Searchable as RT129235$