Minor speedup improvement for pp_require #16175
Comments
From @atoomicThis is a bug report for perl from atoomic@cpan.org, notice that doing an extra INC check before calling require can improve the The idea of this commit is to check INC earlier if possible, by avoiding Would add some benchmark in a future message to show the improvements.
real 0m5.345s
real 0m5.847s Flags: Site configuration information for perl 5.27.5: Configured by root at Tue Sep 26 16:44:50 CDT 2017. Summary of my perl5 (revision 5 version 27 subversion 5) configuration: Locally applied patches: @INC for perl 5.27.5: Environment for perl 5.27.5: PATH=/usr/local/cpanel/3rdparty/perl/526/bin:/usr/local/cpanel/3rdparty/perl/524/bin:/usr/local/cpanel/3rdparty/perl/522/bin:/usr/local/cpanel/3rdparty/perl/514/bin:/usr/local/cpanel/3rdparty/bin:/root/bin/:/opt/local/bin:/opt/local/sbin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/opt/cpanel/composer/bin:/root/.dotfiles/bin:/root/perl5/bin:/root/.rvm/bin:/root/bin |
From @atoomic0001-pp_require-return-earlier-when-module-is-already-loa.patchFrom 27059c3d9938ec4993efee4ec069a9b79b361aac Mon Sep 17 00:00:00 2001
From: Nicolas R <atoomic@cpan.org>
Date: Tue, 26 Sep 2017 18:07:47 -0500
Subject: [PATCH] pp_require: return earlier when module is already loaded
---
pp_ctl.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/pp_ctl.c b/pp_ctl.c
index 5f3cfdf23f..3bea5d4246 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -3752,6 +3752,7 @@ S_require_file(pTHX_ SV *sv)
I32 old_savestack_ix;
const bool op_is_require = PL_op->op_type == OP_REQUIRE;
const char *const op_name = op_is_require ? "require" : "do";
+ SV ** svp_cached = NULL;
assert(op_is_require || PL_op->op_type == OP_DOFILE);
@@ -3761,6 +3762,15 @@ S_require_file(pTHX_ SV *sv)
if (!(name && len > 0 && *name))
DIE(aTHX_ "Missing or undefined argument to %s", op_name);
+#ifndef VMS
+ /* try to return earlier (save the SAFE_PATHNAME check) if INC already got the name */
+ if (op_is_require) {
+ /* can optimize to only perform one single lookup */
+ SV ** svp_cached = hv_fetch(GvHVn(PL_incgv), (char*) name, len, 0);
+ if ( svp_cached && *svp_cached != &PL_sv_undef ) RETPUSHYES;
+ }
+#endif
+
if (!IS_SAFE_PATHNAME(name, len, op_name)) {
if (!op_is_require) {
CLEAR_ERRSV();
@@ -3799,8 +3809,8 @@ S_require_file(pTHX_ SV *sv)
unixlen = len;
}
if (op_is_require) {
- SV * const * const svp = hv_fetch(GvHVn(PL_incgv),
- unixname, unixlen, 0);
+ /* reuse the previous hv_fetch result if possible */
+ SV * const * const svp = svp_cached ? svp_cached : hv_fetch(GvHVn(PL_incgv), unixname, unixlen, 0);
if ( svp ) {
if (*svp != &PL_sv_undef)
RETPUSHYES;
--
2.14.2
|
From @atoomicJust ran some basic benchmark with/without the patch on top of blead=dc41635313 using Porting/bench.pl tool and the following command: ./perl -Ilib Porting/bench.pl --benchfile=../benchmark-tests ../perl2/perl=blead ./perl=blead+patch Of course, the result highly depends on the test used, I try to provide some basic scenarii with short and long module names existing or not. Feel free to comment on it On Tue, 26 Sep 2017 16:16:26 -0700, atoomic@cpan.org wrote:
|
From @atoomic0001-pp_require-return-earlier-when-module-is-already-loa.patchFrom 27059c3d9938ec4993efee4ec069a9b79b361aac Mon Sep 17 00:00:00 2001
From: Nicolas R <atoomic@cpan.org>
Date: Tue, 26 Sep 2017 18:07:47 -0500
Subject: [PATCH] pp_require: return earlier when module is already loaded
---
pp_ctl.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/pp_ctl.c b/pp_ctl.c
index 5f3cfdf23f..3bea5d4246 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -3752,6 +3752,7 @@ S_require_file(pTHX_ SV *sv)
I32 old_savestack_ix;
const bool op_is_require = PL_op->op_type == OP_REQUIRE;
const char *const op_name = op_is_require ? "require" : "do";
+ SV ** svp_cached = NULL;
assert(op_is_require || PL_op->op_type == OP_DOFILE);
@@ -3761,6 +3762,15 @@ S_require_file(pTHX_ SV *sv)
if (!(name && len > 0 && *name))
DIE(aTHX_ "Missing or undefined argument to %s", op_name);
+#ifndef VMS
+ /* try to return earlier (save the SAFE_PATHNAME check) if INC already got the name */
+ if (op_is_require) {
+ /* can optimize to only perform one single lookup */
+ SV ** svp_cached = hv_fetch(GvHVn(PL_incgv), (char*) name, len, 0);
+ if ( svp_cached && *svp_cached != &PL_sv_undef ) RETPUSHYES;
+ }
+#endif
+
if (!IS_SAFE_PATHNAME(name, len, op_name)) {
if (!op_is_require) {
CLEAR_ERRSV();
@@ -3799,8 +3809,8 @@ S_require_file(pTHX_ SV *sv)
unixlen = len;
}
if (op_is_require) {
- SV * const * const svp = hv_fetch(GvHVn(PL_incgv),
- unixname, unixlen, 0);
+ /* reuse the previous hv_fetch result if possible */
+ SV * const * const svp = svp_cached ? svp_cached : hv_fetch(GvHVn(PL_incgv), unixname, unixlen, 0);
if ( svp ) {
if (*svp != &PL_sv_undef)
RETPUSHYES;
--
2.14.2
|
From @atoomic
The numbers represent relative counts per loop iteration, compared to test_1 blead blead+patch COND_m 100.00 100.10 Ir_m1 100.00 101.03 Ir_mm 100.00 100.00 test_2 blead blead+patch COND_m 100.00 100.28 Ir_m1 100.00 100.23 Ir_mm 100.00 100.00 test_3 blead blead+patch COND_m 100.00 434.57 Ir_m1 100.00 101.16 Ir_mm 100.00 100.00 test_4 blead blead+patch COND_m 100.00 99.33 Ir_m1 100.00 100.08 Ir_mm 100.00 100.00 test_5 blead blead+patch COND_m 100.00 97.45 Ir_m1 100.00 99.62 Ir_mm 100.00 100.00 AVERAGE blead blead+patch COND_m 100.00 117.39 Ir_m1 100.00 100.42 Ir_mm 100.00 100.00 |
From @atoomic |
|
The RT System itself - Status changed from 'new' to 'open' |
From @tonycozOn Tue, 26 Sep 2017 16:21:01 -0700, atoomic wrote:
--- a/pp_ctl.c assert(op_is_require || PL_op->op_type == OP_DOFILE); @@ -3761,6 +3762,15 @@ S_require_file(pTHX_ SV *sv) +#ifndef VMS This declares a new svp_cached, so the initial svp_cached isn't modified. + if ( svp_cached && *svp_cached != &PL_sv_undef ) RETPUSHYES; @@ -3799,8 +3809,8 @@ S_require_file(pTHX_ SV *sv) so svp_cached is always NULL here. Tony |
From @atoomicHi, Tony, indeed you are right, sorry came from a last-minute refactoring, I should have noticed it. That 'svp_cached' variable is not doing the cache it claims to do... Here is the updated patch. Just confirm that the test suite passes on my box. I've also updated the benchmark results, using the same tests from benchmark-tests. nicolas On Tue, 26 Sep 2017 18:18:35 -0700, tonyc wrote:
|
From @atoomic0001-pp_require-return-earlier-when-module-is-already-loa.patchFrom 54eb6e76131a19539daa27abe5af4dd36f7df080 Mon Sep 17 00:00:00 2001
From: Nicolas R <atoomic@cpan.org>
Date: Tue, 26 Sep 2017 18:07:47 -0500
Subject: [PATCH] pp_require: return earlier when module is already loaded
---
pp_ctl.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/pp_ctl.c b/pp_ctl.c
index 5f3cfdf23f..1ef7fb463d 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -3752,6 +3752,7 @@ S_require_file(pTHX_ SV *sv)
I32 old_savestack_ix;
const bool op_is_require = PL_op->op_type == OP_REQUIRE;
const char *const op_name = op_is_require ? "require" : "do";
+ SV ** svp_cached = NULL;
assert(op_is_require || PL_op->op_type == OP_DOFILE);
@@ -3761,6 +3762,15 @@ S_require_file(pTHX_ SV *sv)
if (!(name && len > 0 && *name))
DIE(aTHX_ "Missing or undefined argument to %s", op_name);
+#ifndef VMS
+ /* try to return earlier (save the SAFE_PATHNAME check) if INC already got the name */
+ if (op_is_require) {
+ /* can optimize to only perform one single lookup */
+ svp_cached = hv_fetch(GvHVn(PL_incgv), (char*) name, len, 0);
+ if ( svp_cached && *svp_cached != &PL_sv_undef ) RETPUSHYES;
+ }
+#endif
+
if (!IS_SAFE_PATHNAME(name, len, op_name)) {
if (!op_is_require) {
CLEAR_ERRSV();
@@ -3799,8 +3809,8 @@ S_require_file(pTHX_ SV *sv)
unixlen = len;
}
if (op_is_require) {
- SV * const * const svp = hv_fetch(GvHVn(PL_incgv),
- unixname, unixlen, 0);
+ /* reuse the previous hv_fetch result if possible */
+ SV * const * const svp = svp_cached ? svp_cached : hv_fetch(GvHVn(PL_incgv), unixname, unixlen, 0);
if ( svp ) {
if (*svp != &PL_sv_undef)
RETPUSHYES;
--
2.14.2
|
From @atoomic
The numbers represent relative counts per loop iteration, compared to test_1 blead blead+patch COND_m 100.00 99.26 Ir_m1 100.00 101.00 Ir_mm 100.00 100.00 test_2 blead blead+patch COND_m 100.00 100.16 Ir_m1 100.00 100.23 Ir_mm 100.00 100.00 test_3 blead blead+patch COND_m 100.00 431.69 Ir_m1 100.00 101.16 Ir_mm 100.00 100.00 test_4 blead blead+patch COND_m 100.00 98.86 Ir_m1 100.00 100.12 Ir_mm 100.00 100.00 test_5 blead blead+patch COND_m 100.00 97.75 Ir_m1 100.00 99.76 Ir_mm 100.00 100.00 AVERAGE blead blead+patch COND_m 100.00 117.04 Ir_m1 100.00 100.45 Ir_mm 100.00 100.00 |
From @tonycozOn Wed, 27 Sep 2017 09:49:51 -0700, atoomic wrote:
Thanks, applied as 0cbfaef. Tony |
|
@tonycoz - Status changed from 'open' to 'pending release' |
From @khwilliamsonThank you for filing this report. You have helped make Perl better. With the release yesterday of Perl 5.28.0, this and 185 other issues have been Perl 5.28.0 may be downloaded via: If you find that the problem persists, feel free to reopen this ticket. |
|
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#132171 (status was 'resolved')
Searchable as RT132171$
The text was updated successfully, but these errors were encountered: