[PATCH] remove extra stat() call from .pm opening+remove extra safepath check #14991
Comments
From @bulk88See attached patch. syscalls before (not the same processes, ignore the carp below) 1:42:26.8681992 PM perl.exe 5404 CreateFile C:\p521\src\lib\strict.pmc NAME NOT FOUND Desired Access: Read Attributes, Synchronize, Disposition: Open, Options: Synchronous IO Non-Alert, Non-Directory File, Attributes: n/a, ShareMode: None, AllocationSize: n/a after 3:53:21.5922283 PM perl.exe 1076 CreateFile C:\p521\src\lib\strict.pmc NAME NOT FOUND Desired Access: Generic Read, Disposition: Open, Options: Synchronous IO Non-Alert, Non-Directory File, Attributes: N, ShareMode: Read, Write, AllocationSize: n/a
|
From @bulk880001-remove-extra-stat-call-from-.pm-opening-remove-extra.patchFrom 20a88012e06e1e416b4a8de4f10a0c79ad603383 Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Fri, 16 Oct 2015 17:40:38 -0400
Subject: [PATCH] remove extra stat() call from .pm opening+remove extra
safepath check
Originally S_doopen_pm had 2 stat calls, one on the .pm path, and another
on the .pmc, to get mtimes of both. Commit a91233bf4c "Load .pmc always,
even if they are older than a matching .pm file." (see
http://www.nntp.perl.org/group/perl.perl5.porters/2006/03/msg110639.html )
got rid of one of the stat calls but the other was left in place, possibly
as an oversight. S_check_type_and_open itself does another stat call on
unix to check for bad kinds of FS entries (reading a dir as a file), so
assuming someone used .pmc files, a good .pmc would be stat, stat,
open instead of the ideal, stat, open. Remove the extra stat from
S_doopen_pm for efficiency. Since the timestamp compare was removed, the
role of S_doopen_pm has been to verify an attempted path is acceptable to
pass to the FS (no IO done), and generate a .pmc path (no IO done), the
IO side of thing is in S_check_type_and_open, it shouldn't be in
S_doopen_pm.
On Win32, on a no .pmc build, an open is directly done on the attempted
.pm path for efficiency, no stat is done normally (see commit d345f48775
"Win32: stat() only after a failed open() on a module"). Before this patch
the .pmc attempted path got a stat which on Win32 is more than 1 IO call,
compared to Win32 open which is 1 IO call. With this patch, the Win32
specific IO logic in S_check_type_and_open executes instead of a generic
Win32 stat so there is just 1 failing IO call for file not found (typical
case for .pmc) instead of multiple file not found IO calls. See ticket for
details.
When .pmc files are enabled (enabled is default), 2 checks for bad null
char paths were done, once in S_doopen_pm, then again in lower level
S_check_type_and_open. Do the check only once in the higher level call
(S_doopen_pm) for efficiency, there is no way for string "c" which is
catted on to contain a null. There was an existing comment refering to
the problem of a low level check for null returning a message about a
".pmc" instead of a ".pm", so that is another reason to do it at a higher
level. Note on no PMC builds, S_check_type_and_open replaces S_doopen_pm
and still must do the check.
---
pp_ctl.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/pp_ctl.c b/pp_ctl.c
index 1333be8..cdbdbd0 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -3481,11 +3481,14 @@ S_check_type_and_open(pTHX_ SV *name)
/* checking here captures a reasonable error message when
* PERL_DISABLE_PMC is true, but when PMC checks are enabled, the
* user gets a confusing message about looking for the .pmc file
- * rather than for the .pm file.
+ * rather than for the .pm file so do the check in S_doopen_pm when
+ * PMC is on instead of here. S_doopen_pm calls this func.
* This check prevents a \0 in @INC causing problems.
*/
+#ifdef PERL_DISABLE_PMC
if (!IS_SAFE_PATHNAME(p, len, "require"))
return NULL;
+#endif
/* on Win32 stat is expensive (it does an open() and close() twice and
a couple other IO calls), the open will fail with a dir on its own with
@@ -3541,13 +3544,14 @@ S_doopen_pm(pTHX_ SV *name)
if (namelen > 3 && memEQs(p + namelen - 3, 3, ".pm")) {
SV *const pmcsv = sv_newmortal();
- Stat_t pmcstat;
+ PerlIO * pmcio;
SvSetSV_nosteal(pmcsv,name);
sv_catpvs(pmcsv, "c");
- if (PerlLIO_stat(SvPV_nolen_const(pmcsv), &pmcstat) >= 0)
- return check_type_and_open(pmcsv);
+ pmcio = check_type_and_open(pmcsv);
+ if (pmcio)
+ return pmcio;
}
return check_type_and_open(name);
}
--
1.9.5.msysgit.1
|
From @jkeenanOn Fri Oct 16 14:51:29 2015, bulk88 wrote:
1. Does no harm on Linux. Smoke-testing in smoke-me/jkeenan/bulk88/126377-remove-extra-stat-call. 2. bulk88, the performance data you present below is interesting but difficult to absorb quickly in an inline format which changes tabs to whitespace. Would it be possible for you to save the data to .tsv or .csv files and attach to the RT? Thank you very much.
[snip] -- |
|
The RT System itself - Status changed from 'new' to 'open' |
From @bulk88On Sat Oct 17 05:09:42 2015, jkeenan wrote:
The original data is gone (closed the window). The CSV looked more unreadable because the columns were not lined up than the format I pasted it in. It looked fine in my webmail editor (my normal email prog is down ATM). I can post CSV logs as separate files but I just think they are more unreadable. Any consensus on whether to post CSV or space/tab padded logs? If you click on "Download (untitled)" next to "text/plain 9.5k" on the OP, whose link is https://rt-archive.perl.org/perl5/Ticket/Attachment/1371067/735368/ it is readable and sort of aligned, you might have to zoom out with your browser to decrease the line wrapping. -- |
From @TuxOn Sat, 17 Oct 2015 14:02:07 -0700, "bulk88 via RT"
<shameless plug> $ cpan Text::CSV_XS Spreadsheet::Read </shameless plug> if you have X and perl/Tk $ csv2tk test.csv
-- |
|
@tonycoz - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#126377 (status was 'resolved')
Searchable as RT126377$
The text was updated successfully, but these errors were encountered: