[PATCH] PathTools, dont require() modules in subs likely to be in loops #14859
Comments
From @bulk88Created by @bulk88See attached patch. write_buildcustomize went from ~70 calls to Perl Info |
From @bulk880001-PathTools-dont-require-modules-in-subs-likely-to-be-.patchFrom 0ba767e9c1e8eb3911d5ff603b38ab8aa5d4ce4b Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Mon, 17 Aug 2015 06:46:08 -0400
Subject: [PATCH] PathTools, dont require() modules in subs likely to be in
loops
-since Cwd is always loaded in File:Spec:Unix at the top, require'ing it in
a user's loop in _cwd is duplicative and wasteful
-since sub _cwd now has no real body anymore just alias it for
speed(no callframe)/memory(no CV+ops)
-Cwd has PP parts if XS isn't available, on miniperl load full Cwd at the
start instead below inside a sub
-File::Spec::Win32 doesn't need to load Cwd in a sub, probably in a user's
loop, Cwd is loaded from File::Spec::Unix
-File::Spec loads an OS specific .pm, all the OS specific .pm'es load
Unix.pm, hence all File::Spec::Functions needs to do is load File::Spec to
get Unix.pm, not directly call for Unix.pm
---
dist/PathTools/lib/File/Spec/Functions.pm | 1 -
dist/PathTools/lib/File/Spec/Unix.pm | 13 +++++--------
dist/PathTools/lib/File/Spec/Win32.pm | 1 -
3 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/dist/PathTools/lib/File/Spec/Functions.pm b/dist/PathTools/lib/File/Spec/Functions.pm
index 5c2cec0..e5927e4 100644
--- a/dist/PathTools/lib/File/Spec/Functions.pm
+++ b/dist/PathTools/lib/File/Spec/Functions.pm
@@ -37,7 +37,6 @@ require Exporter;
%EXPORT_TAGS = ( ALL => [ @EXPORT_OK, @EXPORT ] );
-require File::Spec::Unix;
my %udeps = (
canonpath => [],
catdir => [qw(canonpath)],
diff --git a/dist/PathTools/lib/File/Spec/Unix.pm b/dist/PathTools/lib/File/Spec/Unix.pm
index 48e2b60..523fad0 100644
--- a/dist/PathTools/lib/File/Spec/Unix.pm
+++ b/dist/PathTools/lib/File/Spec/Unix.pm
@@ -7,12 +7,13 @@ $VERSION = '3.57';
my $xs_version = $VERSION;
$VERSION =~ tr/_//;
-#dont try to load XSLoader and DynaLoader only to ultimately fail on miniperl
-if(!defined &canonpath && defined &DynaLoader::boot_DynaLoader) {
+
+if(!defined &canonpath) {
eval {#eval is questionable since we are handling potential errors like
#"Cwd object version 3.48 does not match bootstrap parameter 3.50
#at lib/DynaLoader.pm line 216." by having this eval
- if ( $] >= 5.006 ) {
+#dont try to load XSLoader and DynaLoader only to ultimately fail on miniperl
+ if ( $] >= 5.006 && defined &DynaLoader::boot_DynaLoader) {
require XSLoader;
XSLoader::load("Cwd", $xs_version);
} else {
@@ -555,11 +556,7 @@ L<File::Spec>
# Internal routine to File::Spec, no point in making this public since
# it is the standard Cwd interface. Most of the platform-specific
# File::Spec subclasses use this.
-sub _cwd {
- require Cwd;
- Cwd::getcwd();
-}
-
+*_cwd = *Cwd::getcwd;
# Internal method to reduce xx\..\yy -> yy
sub _collapse {
diff --git a/dist/PathTools/lib/File/Spec/Win32.pm b/dist/PathTools/lib/File/Spec/Win32.pm
index 77e0fed..222a428 100644
--- a/dist/PathTools/lib/File/Spec/Win32.pm
+++ b/dist/PathTools/lib/File/Spec/Win32.pm
@@ -331,7 +331,6 @@ sub rel2abs {
}
if ( !defined( $base ) || $base eq '' ) {
- require Cwd ;
$base = Cwd::getdcwd( ($self->splitpath( $path ))[0] ) if defined &Cwd::getdcwd ;
$base = $self->_cwd() unless defined $base ;
}
--
1.7.9.msysgit.0
|
From @ap* bulk88 <perlbug-followup@perl.org> [2015-08-17 13:30]:
I don’t like the reliance of modules on being loaded from specific other You’ll get almost all of the same savings by turning every `require Cwd` Otherwise ++ (It’s not the ’90s any more.) Regards, |
|
The RT System itself - Status changed from 'new' to 'open' |
From @tonycozOn Mon Aug 17 08:24:48 2015, aristotle wrote:
I agree. Tony |
From @bulk88On Mon Aug 17 08:24:48 2015, aristotle wrote:
File::Spec is an abstract base class with almost no code in it, it is implemented with an OS specific class. All the OS specific classes happen to inherit from Unix class but that is upto the OS specific class to decide. File::Spec::Functions is a wrapper around File::Spec class. The use/requires should match the class diagram. If an OS specific class wants to implement sub _cwd itself without Cwd.pm or Cwd.xs, it is free to do so. The _cwd sub isn't public API of File::Spec.
The requires were moved to the top so they run once. File::Spec's loading time is important since the short ExtUtils::Command one liners use File::Copy which loads File::Spec. Each Perl_pp_require function takes 0.165 ms or .091 ms (removed timer overhead derived from Perl_pp_padsv which is very light) to execute for me. If you count 3500 perl process starts which load File::Spec during make test (someone said it was ~7000 process starts to do a make test, I'll assume 1/3 are EU::* processes and test.pl), .091*3500=318 ms saved. A make test is 25 to 30 minutes for me. Although 318 ms is 1/6000th of a make test, 300 ms is countable time. A couple hundred smoke tests later over some weeks, 300 ms turns into dozens of seconds. Adding "use Cwd" lines would take half a minute of scrolling and typing in the future. The CPU time saved exceeded human time. Next question, who are the future maintainers you speak of, and why would they be removing Cwd.pm from PathTools distro and perl core which would requiring redesigning the class dependencies of PathTools/File::Spec? What is flawed with File::Spec's current package names? Is Cwd.pm going to be removed from PathTools in favor of Cwd::Tiny.pm? How would CPAN code handle Cwd.pm being spun off into its own tarball? If someone will redesign the class dependency of File::Spec/Cwd for some unknown reason, changing use/require lines comes with the cost of touching the code to rename PathTool's .pm files and all of its classes. I feel your concerns about future changes are as valid as saying "This patch should only split function arguments between lines and not at logic operators since it will cause extra costs in the future when the code is ported to Python." -- |
From @ap* bulk88 via RT <perlbug-followup@perl.org> [2015-08-19 09:25]:
I wasn’t talking about either File::Spec or File::Spec::Functions so (You didn’t even patch File::Spec, so I cannot even have an objection to
Except in File::Spec::Win32 where instead of moving it to the top, you
Dozens of seconds over the course of weeks. Well that convinced me. :-)
It has had a few dozen patchers in the past: Take your pick. (I’m not sure how I’m supposed to interpret this question anyway: après
I… have no idea what any of that has to do with anything I said.
I have to qualify that, based on how much the content of your mail seems And I will agree with you that that dude’s concerns are indeed about as Regards, |
From zefram@fysh.orgThe removal of "require File::Spec::Unix" from File::Spec::Functions is The removal of "require Cwd" from F:S:Win32 is similarly faulty, since The proposed new version of _cwd in F:S:U is also breaking an API. The loading of Cwd from F:S:U can be simplified massively by relying on Rewritten version of the patch applied as commit -zefram |
From @xsawyerxOn 11/11/2017 11:08 AM, Zefram wrote:
I wish this email was part of the commit message as well. Does anyone want to pick up cutting a new release of PathTools? |
|
@xsawyerx - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#125827 (status was 'resolved')
Searchable as RT125827$
The text was updated successfully, but these errors were encountered: