New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PATCH] remove PL_patchlevel from and optimize S_minus_v #12687
Comments
From @bulk88See attached patch. |
From @bulk880001-remove-PL_patchlevel-from-and-optimize-S_minus_v.patchFrom 7e84c731682a55cf3f8f88caa599c844c8151b8f Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Fri, 4 Jan 2013 12:34:33 -0500
Subject: [PATCH] remove PL_patchlevel from and optimize S_minus_v
|SvPVX(vstringify(PL_patchlevel))| is the same string as
|"v" PERL_VERSION_STRING|. No need to go through the overhead of using a
version object. Instead of creating a SV, then further manipulating it.
Create and manipulate it at the same time with Perl_newSVpvf_nocontext or
newSVpvn. "num_len >= level_len " will be folded away by the compiler,
previously, since both lengths came from SVs, they were not const
technically. A very smart compiler might see strncmp/strnEQ takes all
const arguments, and will optimize that away also. VC 2003 didnt do that.
Change SvREFCNT_dec to SvREFCNT_dec_NN for obvious reasons.
With the copyright lines, use more CPP cating. This reduced the number of
instructions, and removes a couple alignment null in .rdata image section.
UNDER_CE presents a challange because of the wce_hitreturn(); call.
BINARY_BUILD_NOTICE is arbitrary code. ActivePerl uses it and it contains
a perlio printf call. So that can not be CPP catted unfortunatly. If a
build is not UNDER_CE and BINARY_BUILD_NOTICE is undef, then the copyright
line is catted with the license line.
Prior to this patch, S_minus_v was 0x129 bytes long of x86-32 VC 2003 -O1
-GL machine code. After it is 0x70 bytes long. Although S_minus_v is not
hot and is rarely used, making the interp image smaller, and therefore
faster is a good idea. There should not be any user visible changes to -v
with this patch. switches.t contains a regexp for -v already so no further
tests were added.
---
perl.c | 102 ++++++++++++++++++++++++++++++++--------------------------------
1 files changed, 51 insertions(+), 51 deletions(-)
diff --git a/perl.c b/perl.c
index 0cfb73c..7589e1f 100644
--- a/perl.c
+++ b/perl.c
@@ -3414,32 +3414,38 @@ Perl_moreswitches(pTHX_ const char *s)
STATIC void
S_minus_v(pTHX)
{
+
PerlIO * PIO_stdout;
- if (!sv_derived_from(PL_patchlevel, "version"))
- upg_version(PL_patchlevel, TRUE);
#if !defined(DGUX)
{
- SV* level= vstringify(PL_patchlevel);
+ const char * const level_str = "v" PERL_VERSION_STRING;
+ const STRLEN level_len = sizeof("v" PERL_VERSION_STRING)-1;
#ifdef PERL_PATCHNUM
+ SV* level;
# ifdef PERL_GIT_UNCOMMITTED_CHANGES
- SV *num = newSVpvs(PERL_PATCHNUM "*");
+ static const char num [] = PERL_PATCHNUM "*";
# else
- SV *num = newSVpvs(PERL_PATCHNUM);
+ static const char num [] = PERL_PATCHNUM;
# endif
{
- STRLEN level_len, num_len;
- char * level_str, * num_str;
- num_str = SvPV(num, num_len);
- level_str = SvPV(level, level_len);
- if (num_len>=level_len && strnEQ(num_str,level_str,level_len)) {
- SvREFCNT_dec(level);
- level= num;
+ const STRLEN num_len = sizeof(num)-1;
+ /* A very advanced compiler would fold away the strnEQ
+ and this whole conditional, but most (all?) won't do it.
+ SV level could also be replaced by with preprocessor
+ catenation.
+ */
+ if (num_len >= level_len && strnEQ(num,level_str,level_len)) {
+ /* per 46807d8e80, PERL_PATCHNUM is outside of the control
+ of the interp so it might contain format characters
+ */
+ level = Perl_newSVpvf_nocontext("%s", num);
} else {
- Perl_sv_catpvf(aTHX_ level, " (%"SVf")", num);
- SvREFCNT_dec(num);
+ level = Perl_newSVpvf_nocontext("%s (%s)", level_str, num);
}
}
- #endif
+#else
+ SV* level = newSVpvn(level_str, level_len);
+#endif /* #ifdef PERL_PATCHNUM */
PIO_stdout = PerlIO_stdout();
PerlIO_printf(PIO_stdout,
"\nThis is perl " STRINGIFY(PERL_REVISION)
@@ -3447,14 +3453,13 @@ S_minus_v(pTHX)
", subversion " STRINGIFY(PERL_SUBVERSION)
" (%"SVf") built for " ARCHNAME, level
);
- SvREFCNT_dec(level);
+ SvREFCNT_dec_NN(level);
}
#else /* DGUX */
PIO_stdout = PerlIO_stdout();
/* Adjust verbose output as in the perl that ships with the DG/UX OS from EMC */
PerlIO_printf(PIO_stdout,
- Perl_form(aTHX_ "\nThis is perl, %"SVf"\n",
- SVfARG(vstringify(PL_patchlevel))));
+ Perl_form(aTHX_ "\nThis is perl, " "v" PERL_VERSION_STRING "\n");
PerlIO_printf(PIO_stdout,
Perl_form(aTHX_ " built under %s at %s %s\n",
OSNAME, __DATE__, __TIME__));
@@ -3472,47 +3477,42 @@ S_minus_v(pTHX)
#endif
PerlIO_printf(PIO_stdout,
- "\n\nCopyright 1987-2012, Larry Wall\n");
-#ifdef MSDOS
- PerlIO_printf(PIO_stdout,
- "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n");
-#endif
-#ifdef DJGPP
- PerlIO_printf(PIO_stdout,
+ "\n\nCopyright 1987-2012, Larry Wall\n"
+#if defined(MSDOS)
+ "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n"
+#elif defined(DJGPP)
"djgpp v2 port (jpl5003c) by Hirofumi Watanabe, 1996\n"
- "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n");
-#endif
-#ifdef OS2
- PerlIO_printf(PIO_stdout,
+ "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n"
+#elif defined(OS2)
"\n\nOS/2 port Copyright (c) 1990, 1991, Raymond Chen, Kai Uwe Rommel\n"
- "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n");
-#endif
-#ifdef OEMVS
- PerlIO_printf(PIO_stdout,
- "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n");
-#endif
-#ifdef __VOS__
- PerlIO_printf(PIO_stdout,
- "Stratus VOS port by Paul.Green@stratus.com, 1997-2002\n");
-#endif
-#ifdef POSIX_BC
- PerlIO_printf(PIO_stdout,
- "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n");
-#endif
-#ifdef UNDER_CE
- PerlIO_printf(PIO_stdout,
+ "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n"
+#elif defined(__BEOS__)
+ "BeOS port Copyright Tom Spindler, 1997-1999\n"
+#elif defined(OEMVS)
+ "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"
+#elif defined(__VOS__)
+ "Stratus VOS port by Paul.Green@stratus.com, 1997-2002\n"
+#elif defined(POSIX_BC)
+ "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"
+/* UNDER_CE closes the printf call*/
+#elif defined(UNDER_CE)
"WINCE port by Rainer Keuchel, 2001-2002\n"
"Built on " __DATE__ " " __TIME__ "\n\n");
wce_hitreturn();
-#endif
-#ifdef __SYMBIAN32__
- PerlIO_printf(PIO_stdout,
- "Symbian port by Nokia, 2004-2005\n");
-#endif
+#elif defined(__SYMBIAN32__)
+ "Symbian port by Nokia, 2004-2005\n"
+#endif /* end of additional copyright lines */
+
#ifdef BINARY_BUILD_NOTICE
+# ifndef UNDER_CE
+ ); /* close Larry Wall copyright printf */
+# endif
BINARY_BUILD_NOTICE;
-#endif
PerlIO_printf(PIO_stdout,
+#elif defined(UNDER_CE) /* because of wce_hitreturn() open new printf */
+ PerlIO_printf(PIO_stdout,
+/* else CPP catenation continues from Larry Wall copyright printf */
+#endif
"\n\
Perl may be copied only under the terms of either the Artistic License or the\n\
GNU General Public License, which may be found in the Perl 5 source kit.\n\n\
--
1.7.9.msysgit.0
|
From @tonycozOn Fri Jan 04 09:37:10 2013, bulk88 wrote:
This patch no longer applies. I can see a couple of places it could be improved: + level = Perl_newSVpvf_nocontext("%s", num); Couldn't this just be: level = newSVpvn(num, num_len); PerlIO_printf(PIO_stdout, Why is Perl_form() being called here? #elif defined(UNDER_CE) The contortions to handle UNDER_CE ending the PerlIO_printf() are Tony |
The RT System itself - Status changed from 'new' to 'open' |
From @bulk88On Fri Jul 05 00:01:09 2013, tonyc wrote:
I'll have to work on it then.
No for code size reasons. There is a 2nd Perl_newSVpvf_nocontext below
Good catch. It is redundant.
So confusing I need more time to think up an answer. I have to try to
-- |
From @bulk88Patch rewritten for blead. Some changes from the last old one is the |
From @bulk880001-remove-PL_patchlevel-from-and-optimize-S_minus_v.patchFrom 4acb8e4ecbbd1691ad8e83c1a8a8e57a94fd97d3 Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Fri, 5 Jul 2013 19:12:29 -0400
Subject: [PATCH] remove PL_patchlevel from and optimize S_minus_v
|SvPVX(vstringify(PL_patchlevel))| is the same string as
|"v" PERL_VERSION_STRING|. No need to go through the overhead of using a
version object. Instead of creating a SV, then further manipulating it.
Create and manipulate it at the same time with Perl_newSVpvf_nocontext or
newSVpvn. "num_len >= level_len " will be folded away by the compiler,
previously, since both lengths came from SVs, they were not const
technically. A very smart compiler might see strncmp/strnEQ takes all
const arguments, and will optimize that away also. VC 2003 didnt do that.
Change SvREFCNT_dec to SvREFCNT_dec_NN for obvious reasons.
With the copyright lines, use more CPP cating. This reduced the number of
instructions, and removes a couple alignment null in .rdata image section.
UNDER_CE presents a challange because of the wce_hitreturn(); call.
BINARY_BUILD_NOTICE is arbitrary code. ActivePerl uses it and it contains
a perlio printf call. So that can not be CPP catted unfortunatly. If a
build is not UNDER_CE and BINARY_BUILD_NOTICE is undef, then the copyright
line is catted with the license line.
Prior to this patch, S_minus_v was 0x13b bytes long of x86-32 VC 2003 -O1
-GL machine code. After it is 0x77 bytes long. Although S_minus_v is not
hot and is rarely used, making the interp image smaller, and therefore
faster is a good idea. There should not be any user visible changes to -v
with this patch. switches.t contains a regexp for -v already so no further
tests were added.
---
perl.c | 95 ++++++++++++++++++++++++++++++++++-----------------------------
1 files changed, 51 insertions(+), 44 deletions(-)
diff --git a/perl.c b/perl.c
index bad66f5..2198205 100644
--- a/perl.c
+++ b/perl.c
@@ -3502,30 +3502,35 @@ STATIC void
S_minus_v(pTHX)
{
PerlIO * PIO_stdout;
- if (!sv_derived_from(PL_patchlevel, "version"))
- upg_version(PL_patchlevel, TRUE);
{
- SV* level= vstringify(PL_patchlevel);
+ const char * const level_str = "v" PERL_VERSION_STRING;
+ const STRLEN level_len = sizeof("v" PERL_VERSION_STRING)-1;
#ifdef PERL_PATCHNUM
+ SV* level;
# ifdef PERL_GIT_UNCOMMITTED_CHANGES
- SV *num = newSVpvs(PERL_PATCHNUM "*");
+ static const char num [] = PERL_PATCHNUM "*";
# else
- SV *num = newSVpvs(PERL_PATCHNUM);
+ static const char num [] = PERL_PATCHNUM;
# endif
{
- STRLEN level_len, num_len;
- char * level_str, * num_str;
- num_str = SvPV(num, num_len);
- level_str = SvPV(level, level_len);
- if (num_len>=level_len && strnEQ(num_str,level_str,level_len)) {
- SvREFCNT_dec(level);
- level= num;
+ const STRLEN num_len = sizeof(num)-1;
+ /* A very advanced compiler would fold away the strnEQ
+ and this whole conditional, but most (all?) won't do it.
+ SV level could also be replaced by with preprocessor
+ catenation.
+ */
+ if (num_len >= level_len && strnEQ(num,level_str,level_len)) {
+ /* per 46807d8e80, PERL_PATCHNUM is outside of the control
+ of the interp so it might contain format characters
+ */
+ level = Perl_newSVpvf_nocontext("%s", num);
} else {
- Perl_sv_catpvf(aTHX_ level, " (%"SVf")", num);
- SvREFCNT_dec(num);
+ level = Perl_newSVpvf_nocontext("%s (%s)", level_str, num);
}
}
- #endif
+#else
+ SV* level = newSVpvn(level_str, level_len);
+#endif /* #ifdef PERL_PATCHNUM */
PIO_stdout = PerlIO_stdout();
PerlIO_printf(PIO_stdout,
"\nThis is perl " STRINGIFY(PERL_REVISION)
@@ -3533,59 +3538,61 @@ S_minus_v(pTHX)
", subversion " STRINGIFY(PERL_SUBVERSION)
" (%"SVf") built for " ARCHNAME, level
);
- SvREFCNT_dec(level);
+ SvREFCNT_dec_NN(level);
}
-#if defined(LOCAL_PATCH_COUNT)
- if (LOCAL_PATCH_COUNT > 0)
- PerlIO_printf(PIO_stdout,
- "\n(with %d registered patch%s, "
- "see perl -V for more detail)",
- LOCAL_PATCH_COUNT,
- (LOCAL_PATCH_COUNT!=1) ? "es" : "");
-#endif
-
PerlIO_printf(PIO_stdout,
- "\n\nCopyright 1987-2013, Larry Wall\n");
+#if defined(LOCAL_PATCH_COUNT)
+ /* LOCAL_PATCH_COUNT is a C constant, not a preprocessor constant */
+ LOCAL_PATCH_COUNT == 1 ?
+ "\n(with %d registered patch, see perl -V for more detail)%s"
+ : LOCAL_PATCH_COUNT > 0 ?
+ "\n(with %d registered patches, see perl -V for more detail)%s"
+ : "%.0s%s",
+ LOCAL_PATCH_COUNT <= 0 ? NULL : LOCAL_PATCH_COUNT,
+#endif
+ "\n\nCopyright 1987-2013, Larry Wall\n"
#ifdef MSDOS
- PerlIO_printf(PIO_stdout,
- "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n");
+ "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n"
#endif
#ifdef DJGPP
- PerlIO_printf(PIO_stdout,
"djgpp v2 port (jpl5003c) by Hirofumi Watanabe, 1996\n"
- "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n");
+ "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n"
#endif
#ifdef OS2
- PerlIO_printf(PIO_stdout,
"\n\nOS/2 port Copyright (c) 1990, 1991, Raymond Chen, Kai Uwe Rommel\n"
- "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n");
+ "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n"
#endif
#ifdef OEMVS
- PerlIO_printf(PIO_stdout,
- "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n");
+ "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"
#endif
#ifdef __VOS__
- PerlIO_printf(PIO_stdout,
- "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n");
+ "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n"
#endif
#ifdef POSIX_BC
- PerlIO_printf(PIO_stdout,
- "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n");
+ "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"
+#endif
+#ifdef __SYMBIAN32__
+ "Symbian port by Nokia, 2004-2005\n"
#endif
+/* UNDER_CE closes the printf call*/
#ifdef UNDER_CE
- PerlIO_printf(PIO_stdout,
"WINCE port by Rainer Keuchel, 2001-2002\n"
"Built on " __DATE__ " " __TIME__ "\n\n");
wce_hitreturn();
-#endif
-#ifdef __SYMBIAN32__
- PerlIO_printf(PIO_stdout,
- "Symbian port by Nokia, 2004-2005\n");
-#endif
+#endif /* end of additional copyright lines */
+
#ifdef BINARY_BUILD_NOTICE
+# ifndef UNDER_CE
+ ); /* close Larry Wall copyright printf */
+# endif
+/* ActivePerl uses BINARY_BUILD_NOTICE, it is a statement */
BINARY_BUILD_NOTICE;
#endif
+/* because of wce_hitreturn() open new printf for UNDER_CE */
+#if defined(BINARY_BUILD_NOTICE) || defined(UNDER_CE)
PerlIO_printf(PIO_stdout,
+/* else CPP catenation continues from Larry Wall copyright printf */
+#endif
"\n\
Perl may be copied only under the terms of either the Artistic License or the\n\
GNU General Public License, which may be found in the Perl 5 source kit.\n\n\
--
1.7.9.msysgit.0
|
From @iabynOn Fri, Jul 05, 2013 at 08:34:01AM -0700, bulk88 via RT wrote:
I really don't like this part. Its just an excessive level of On x86_64 platforms with the "first 6 args are in registers" calling The downside is that the src code becomes more bloated and confusing. -- |
From @bulk88On Sat Jul 06 07:45:05 2013, davem wrote:
On Win64 that is wrong. my_perl has to be copied every time to the 1st Keeping func params const on assembly level, to avoid copying in the http://lists.cs.uiuc.edu/pipermail/llvmdev/2009-September/025927.html http://nondot.org/sabre/LLVMNotes/GlobalRegisterVariables.txt Here is the boot func from re.dll
And this is why people give up, switch languages, then tell all their -- |
From @khwilliamsonOn 07/06/2013 10:10 AM, bulk88 via RT wrote:
I agree with Dave, and I believe it is counterproductive to write code But suppose we are wrong; suppose it has to be implemented the way you |
From @tonycozOn Sat, Jul 06, 2013 at 09:10:12AM -0700, bulk88 via RT wrote:
While Dave was wrong about the registers, his general argument about With gcc/clang the change *costs* 5 bytes[1] - while making the code a
I think it's great that you're enthusiastic about optimizing perl's We optimize for several things, generated code size is important, but And code size doesn't map one-to-one to performance - several Performance doesn't matter much for S_minus_v(), but developer time Tony [1] gcc, default Configure set optimization, -Duseithreads, 00000000000050a0 <S_minus_v>: Then newSVpvn(): 00000000000050a0 <S_minus_v>: |
From @tonycozOn Fri, Jul 05, 2013 at 04:29:29PM -0700, bulk88 via RT wrote:
Except (as discussed) the use of newSVpvn(), I think it's good up to
That's fine too.
This code is incorrect. For some compilers NULL is ((void *)0) - a pointer, not an integer. Consider - what's the type of the expression: LOCAL_PATCH_COUNT <= 0 ? NULL : LOCAL_PATCH_COUNT, It's also unnecessarily duplicative, in that if the registered patches gcc and clang complain about the type too, gcc: perl.c: In function ‘S_minus_v’: clang produces a more understandable message: perl.c:3551:9: warning: format specifies type 'int' but the argument has type (I think it warns twice because you provide three different format The original code is clearer I think.
Rather than trying to build it all into one printf, I think it would PerlIO_printf(PIO_stdout, Tony |
From @bulk88On Sat Jul 06 22:00:36 2013, public@khwilliamson.com wrote:
Does that apply to Perl? http://perl5.git.perl.org/perl.git/commit/99ee1dcd0469086e91a96e31a9b9ea27bb7f0c7e That commit message makes me think you (khw) do believe in writing code Some commits (all authors) talk about cache alignment and struct
There is a comment that explains what is going on "+/* else CPP On Sun Jul 07 23:20:13 2013, tonyc wrote:
Those are good compilers if they do that then but gcc/clang aren't the
The same opinions are coming out again, self-righteous "if I wouldn't
Who is "the poor sob who comes along in 6 months or 5 years and needs to a recent one http://perl5.git.perl.org/perl.git/commit/28e464fba839d02f376952199261fc8b7d58a0d8 http://perl5.git.perl.org/perl.git/commit/88675427278c9e6329fba9382072cae9dd00c163 http://perl5.git.perl.org/perl.git/commit/175bc858cac2126947f4b8fa838835417c1430d1 https://rt-archive.perl.org/perl5/Ticket/Display.html?id=116925 when will
S_minus_v never gets changed except for copyright dates. Ideally it On Sun Jul 07 23:53:42 2013, tonyc wrote:
Will switch to newSVpvn as said above.
I knew that would be a portability problem, so the NULL went there.
The alternative was more complicated (putting each litteral into a
I could break out the LOCAL_PATCH_COUNT stuff into a separate
The GPL line wont be concatted with the larry CR and platform CR in that -- |
From @doughera88On Mon, Jul 08, 2013 at 03:36:35PM -0700, "bulk88 via RT" wrote:
I think that is an unfair characterization of those discussions, -- |
From @rjbs* bulk88 via RT <perlbug-followup@perl.org> [2013-07-08T18:36:35]
I think that this is not only hyperbole (which is one thing), but also unfair In every case that I've seen, the issue has not been "I don't care about your
I encourage you to politely point out such shortcomings as they occur, to I think it would be in some ways nice, and would have some positive effects, to We need a better culture of code review, I agree. I also agree that it would That's why I hope you will, in the future, politely point out when commits have -- |
From @bulk88On Sun Jul 07 23:53:42 2013, tonyc wrote:
Patch remade, newSVpvn and %d replaced with %zu to hopefully (not using -- |
From @bulk880001-remove-PL_patchlevel-from-and-optimize-S_minus_v.patchFrom 272f571b0f05ee9f86e59b3560daf73a5a976e0a Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Thu, 18 Jul 2013 14:55:37 -0400
Subject: [PATCH] remove PL_patchlevel from and optimize S_minus_v
|SvPVX(vstringify(PL_patchlevel))| is the same string as
|"v" PERL_VERSION_STRING|. No need to go through the overhead of using a
version object. Instead of creating a SV, then further manipulating it.
Create and manipulate it at the same time with Perl_newSVpvf_nocontext or
newSVpvn. "num_len >= level_len " will be folded away by the compiler,
previously, since both lengths came from SVs, they were not const
technically. A very smart compiler might see strncmp/strnEQ takes all
const arguments, and will optimize that away also. VC 2003 didnt do that.
Change SvREFCNT_dec to SvREFCNT_dec_NN for obvious reasons.
With the copyright lines, use more CPP cating. This reduced the number of
instructions, and removes a couple alignment null in .rdata image section.
UNDER_CE presents a challange because of the wce_hitreturn(); call.
BINARY_BUILD_NOTICE is arbitrary code. ActivePerl uses it and it contains
a perlio printf call. So that can not be CPP catted unfortunatly. If a
build is not UNDER_CE and BINARY_BUILD_NOTICE is undef, then the copyright
line is catted with the license line.
Although S_minus_v is not hot and is rarely used, making the interp
image smaller, and therefore faster is a good idea. There should not be
any user visible changes to -v with this patch. switches.t contains a
regexp for -v already so no further tests were added.
---
perl.c | 95 ++++++++++++++++++++++++++++++++++-----------------------------
1 files changed, 51 insertions(+), 44 deletions(-)
diff --git a/perl.c b/perl.c
index bad66f5..2c52a6e 100644
--- a/perl.c
+++ b/perl.c
@@ -3502,30 +3502,35 @@ STATIC void
S_minus_v(pTHX)
{
PerlIO * PIO_stdout;
- if (!sv_derived_from(PL_patchlevel, "version"))
- upg_version(PL_patchlevel, TRUE);
{
- SV* level= vstringify(PL_patchlevel);
+ const char * const level_str = "v" PERL_VERSION_STRING;
+ const STRLEN level_len = sizeof("v" PERL_VERSION_STRING)-1;
#ifdef PERL_PATCHNUM
+ SV* level;
# ifdef PERL_GIT_UNCOMMITTED_CHANGES
- SV *num = newSVpvs(PERL_PATCHNUM "*");
+ static const char num [] = PERL_PATCHNUM "*";
# else
- SV *num = newSVpvs(PERL_PATCHNUM);
+ static const char num [] = PERL_PATCHNUM;
# endif
{
- STRLEN level_len, num_len;
- char * level_str, * num_str;
- num_str = SvPV(num, num_len);
- level_str = SvPV(level, level_len);
- if (num_len>=level_len && strnEQ(num_str,level_str,level_len)) {
- SvREFCNT_dec(level);
- level= num;
+ const STRLEN num_len = sizeof(num)-1;
+ /* A very advanced compiler would fold away the strnEQ
+ and this whole conditional, but most (all?) won't do it.
+ SV level could also be replaced by with preprocessor
+ catenation.
+ */
+ if (num_len >= level_len && strnEQ(num,level_str,level_len)) {
+ /* per 46807d8e80, PERL_PATCHNUM is outside of the control
+ of the interp so it might contain format characters
+ */
+ level = newSVpvn(num, num_len);
} else {
- Perl_sv_catpvf(aTHX_ level, " (%"SVf")", num);
- SvREFCNT_dec(num);
+ level = Perl_newSVpvf_nocontext("%s (%s)", level_str, num);
}
}
- #endif
+#else
+ SV* level = newSVpvn(level_str, level_len);
+#endif /* #ifdef PERL_PATCHNUM */
PIO_stdout = PerlIO_stdout();
PerlIO_printf(PIO_stdout,
"\nThis is perl " STRINGIFY(PERL_REVISION)
@@ -3533,59 +3538,61 @@ S_minus_v(pTHX)
", subversion " STRINGIFY(PERL_SUBVERSION)
" (%"SVf") built for " ARCHNAME, level
);
- SvREFCNT_dec(level);
+ SvREFCNT_dec_NN(level);
}
-#if defined(LOCAL_PATCH_COUNT)
- if (LOCAL_PATCH_COUNT > 0)
- PerlIO_printf(PIO_stdout,
- "\n(with %d registered patch%s, "
- "see perl -V for more detail)",
- LOCAL_PATCH_COUNT,
- (LOCAL_PATCH_COUNT!=1) ? "es" : "");
-#endif
-
PerlIO_printf(PIO_stdout,
- "\n\nCopyright 1987-2013, Larry Wall\n");
+#if defined(LOCAL_PATCH_COUNT)
+ /* LOCAL_PATCH_COUNT is a C constant, not a preprocessor constant */
+ LOCAL_PATCH_COUNT == 1 ?
+ "\n(with %zu registered patch, see perl -V for more detail)%s"
+ : LOCAL_PATCH_COUNT > 0 ?
+ "\n(with %zu registered patches, see perl -V for more detail)%s"
+ : "%.0s%s",
+ INT2PTR(size_t, LOCAL_PATCH_COUNT),
+#endif
+ "\n\nCopyright 1987-2013, Larry Wall\n"
#ifdef MSDOS
- PerlIO_printf(PIO_stdout,
- "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n");
+ "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n"
#endif
#ifdef DJGPP
- PerlIO_printf(PIO_stdout,
"djgpp v2 port (jpl5003c) by Hirofumi Watanabe, 1996\n"
- "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n");
+ "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n"
#endif
#ifdef OS2
- PerlIO_printf(PIO_stdout,
"\n\nOS/2 port Copyright (c) 1990, 1991, Raymond Chen, Kai Uwe Rommel\n"
- "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n");
+ "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n"
#endif
#ifdef OEMVS
- PerlIO_printf(PIO_stdout,
- "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n");
+ "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"
#endif
#ifdef __VOS__
- PerlIO_printf(PIO_stdout,
- "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n");
+ "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n"
#endif
#ifdef POSIX_BC
- PerlIO_printf(PIO_stdout,
- "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n");
+ "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"
+#endif
+#ifdef __SYMBIAN32__
+ "Symbian port by Nokia, 2004-2005\n"
#endif
+/* UNDER_CE closes the printf call*/
#ifdef UNDER_CE
- PerlIO_printf(PIO_stdout,
"WINCE port by Rainer Keuchel, 2001-2002\n"
"Built on " __DATE__ " " __TIME__ "\n\n");
wce_hitreturn();
-#endif
-#ifdef __SYMBIAN32__
- PerlIO_printf(PIO_stdout,
- "Symbian port by Nokia, 2004-2005\n");
-#endif
+#endif /* end of additional copyright lines */
+
#ifdef BINARY_BUILD_NOTICE
+# ifndef UNDER_CE
+ ); /* close Larry Wall copyright printf */
+# endif
+/* ActivePerl uses BINARY_BUILD_NOTICE, it is a statement */
BINARY_BUILD_NOTICE;
#endif
+/* because of wce_hitreturn() open new printf for UNDER_CE */
+#if defined(BINARY_BUILD_NOTICE) || defined(UNDER_CE)
PerlIO_printf(PIO_stdout,
+/* else CPP catenation continues from Larry Wall copyright printf */
+#endif
"\n\
Perl may be copied only under the terms of either the Artistic License or the\n\
GNU General Public License, which may be found in the Perl 5 source kit.\n\n\
--
1.7.9.msysgit.0
|
From @nwc10On Thu, Jul 18, 2013 at 11:57:37AM -0700, bulk88 via RT wrote:
I'm not sure if everything likes %zu. Particularly older SysV vendor compilers.
I'm pretty sure that mixing #ifdef and pre-processor string concatenation
the construction looks like it could cause compilers to choke Also, what's the maximum length from concatenating string literals doing it Nicholas Clark |
From @bulk88On Thu Jul 18 12:35:21 2013, nicholas wrote:
OS C library is irrelevant. The format string is parsed by Perl itself
The current preproc conditional code could in theory allow 2 different
larry wall copy right+gpl string came out to 411 bytes+alignment padding If CPP token catting doesn't work, I dont think Perl will compile Perl_croak(aTHX_ ifdefs evaled before string litteral catting, -- |
From @tonycozOn Thu Jul 18 11:57:36 2013, bulk88 wrote:
Thanks for the newSVpvn change, unfortunately the format string error CCCMD = cc -DPERL_CORE -c -D_REENTRANT -D_GNU_SOURCE From clang: perl.c:3555:9: warning: format specifies type 'char *' but the argument About the only way I can see to avoid this type of error would be const char copyrights[] = LOCAL_PATCH_COUNT == 0 The compiler should optimize away all but one branch, but it's This updated patch does nothing about the confusing "where's the end of (I almost posted about %zu too, since that's not C89, but you're right, WRT your response to Nicholas:
The C89 limit is 509 characters, and it's a limit on string literals in So, as you said, we're fine here.
http://stackoverflow.com/questions/1476892/poster-with-the-8-phases-of-translation-in-the-c-language While you're right about the behaviour the C specification requires, You're also comparing two different code constructs here, Nicholas said "literal1" may cause a compiler to choke (such a compiler is buggy, string literal Your example contains no preprocessor directives, just macro replacements. I wonder if we should have a document describing such compiler bugs that Tony |
From @doughera88On Thu, Jul 18, 2013 at 06:32:43PM -0700, "Tony Cook via RT" wrote:
I confess I didn't dig through the code to figure out why this was a -- |
From @bulk88On Thu Jul 18 18:32:42 2013, tonyc wrote:
Patch remade, warning silenced on my clang 3.1 and gcc 4.5.3. I thought of doing a "#pragma clang diagnostic ignored "-Wformat"" also, but it seems unnecessary since clang implemented enough of gcc warnings pragma to work. -- |
From @bulk880001-remove-PL_patchlevel-from-and-optimize-S_minus_v.patchFrom ef64b0eddac3937d643ae3f0ec79de8648ec9b72 Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Tue, 29 Oct 2013 18:01:09 -0400
Subject: [PATCH] remove PL_patchlevel from and optimize S_minus_v
|SvPVX(vstringify(PL_patchlevel))| is the same string as
|"v" PERL_VERSION_STRING|. No need to go through the overhead of using a
version object. Instead of creating a SV, then further manipulating it.
Create and manipulate it at the same time with Perl_newSVpvf_nocontext or
newSVpvn. "num_len >= level_len " will be folded away by the compiler,
previously, since both lengths came from SVs, they were not const
technically. A very smart compiler might see strncmp/strnEQ takes all
const arguments, and will optimize that away also. VC 2003 didnt do that.
Change SvREFCNT_dec to SvREFCNT_dec_NN for obvious reasons.
With the copyright lines, use more CPP cating. This reduced the number of
instructions, and removes a couple alignment null in .rdata image section.
UNDER_CE presents a challange because of the wce_hitreturn(); call.
BINARY_BUILD_NOTICE is arbitrary code. ActivePerl uses it and it contains
a perlio printf call. So that can not be CPP catted unfortunatly. If a
build is not UNDER_CE and BINARY_BUILD_NOTICE is undef, then the copyright
line is catted with the license line.
Although S_minus_v is not hot and is rarely used, making the interp
image smaller, and therefore faster is a good idea. There should not be
any user visible changes to -v with this patch. switches.t contains a
regexp for -v already so no further tests were added.
---
perl.c | 113 ++++++++++++++++++++++++++++++++++++++++-------------------------
1 file changed, 69 insertions(+), 44 deletions(-)
diff --git a/perl.c b/perl.c
index 4c80edb..2dbc9ee 100644
--- a/perl.c
+++ b/perl.c
@@ -3525,35 +3525,47 @@ Perl_moreswitches(pTHX_ const char *s)
return NULL;
}
-
+#ifdef __GNUC__
+# if __GNUC__ == 4 && __GNUC_MINOR__ >= 6 || __GNUC__ > 4 /* 4.6 -> */
+# pragma GCC diagnostic push
+# endif
+# if __GNUC__ == 4 && __GNUC_MINOR__ >= 2 || __GNUC__ > 4 /* 4.2 -> */
+# pragma GCC diagnostic ignored "-Wformat"
+# endif
+#endif
STATIC void
S_minus_v(pTHX)
{
PerlIO * PIO_stdout;
- if (!sv_derived_from(PL_patchlevel, "version"))
- upg_version(PL_patchlevel, TRUE);
{
- SV* level= vstringify(PL_patchlevel);
+ const char * const level_str = "v" PERL_VERSION_STRING;
+ const STRLEN level_len = sizeof("v" PERL_VERSION_STRING)-1;
#ifdef PERL_PATCHNUM
+ SV* level;
# ifdef PERL_GIT_UNCOMMITTED_CHANGES
- SV *num = newSVpvs(PERL_PATCHNUM "*");
+ static const char num [] = PERL_PATCHNUM "*";
# else
- SV *num = newSVpvs(PERL_PATCHNUM);
+ static const char num [] = PERL_PATCHNUM;
# endif
{
- STRLEN level_len, num_len;
- char * level_str, * num_str;
- num_str = SvPV(num, num_len);
- level_str = SvPV(level, level_len);
- if (num_len>=level_len && strnEQ(num_str,level_str,level_len)) {
- SvREFCNT_dec(level);
- level= num;
+ const STRLEN num_len = sizeof(num)-1;
+ /* A very advanced compiler would fold away the strnEQ
+ and this whole conditional, but most (all?) won't do it.
+ SV level could also be replaced by with preprocessor
+ catenation.
+ */
+ if (num_len >= level_len && strnEQ(num,level_str,level_len)) {
+ /* per 46807d8e80, PERL_PATCHNUM is outside of the control
+ of the interp so it might contain format characters
+ */
+ level = newSVpvn(num, num_len);
} else {
- Perl_sv_catpvf(aTHX_ level, " (%"SVf")", num);
- SvREFCNT_dec(num);
+ level = Perl_newSVpvf_nocontext("%s (%s)", level_str, num);
}
}
- #endif
+#else
+ SV* level = newSVpvn(level_str, level_len);
+#endif /* #ifdef PERL_PATCHNUM */
PIO_stdout = PerlIO_stdout();
PerlIO_printf(PIO_stdout,
"\nThis is perl " STRINGIFY(PERL_REVISION)
@@ -3561,59 +3573,65 @@ S_minus_v(pTHX)
", subversion " STRINGIFY(PERL_SUBVERSION)
" (%"SVf") built for " ARCHNAME, level
);
- SvREFCNT_dec(level);
+ SvREFCNT_dec_NN(level);
}
-#if defined(LOCAL_PATCH_COUNT)
- if (LOCAL_PATCH_COUNT > 0)
- PerlIO_printf(PIO_stdout,
- "\n(with %d registered patch%s, "
- "see perl -V for more detail)",
- LOCAL_PATCH_COUNT,
- (LOCAL_PATCH_COUNT!=1) ? "es" : "");
-#endif
+/* gcc pragma above silences "perl.c: In function 'S_minus_v':
+ warning: format '%.0s' expects type 'char *', but argument 3 has type
+ 'unsigned int'" below */
PerlIO_printf(PIO_stdout,
- "\n\nCopyright 1987-2013, Larry Wall\n");
+#if defined(LOCAL_PATCH_COUNT)
+ /* LOCAL_PATCH_COUNT is a C constant, not a preprocessor constant */
+ LOCAL_PATCH_COUNT == 1 ?
+ "\n(with %zu registered patch, see perl -V for more detail)%s"
+ : LOCAL_PATCH_COUNT > 0 ?
+ "\n(with %zu registered patches, see perl -V for more detail)%s"
+ : "%.0s%s",
+ INT2PTR(size_t, LOCAL_PATCH_COUNT),
+#endif
+ "\n\nCopyright 1987-2013, Larry Wall\n"
#ifdef MSDOS
- PerlIO_printf(PIO_stdout,
- "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n");
+ "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n"
#endif
#ifdef DJGPP
- PerlIO_printf(PIO_stdout,
"djgpp v2 port (jpl5003c) by Hirofumi Watanabe, 1996\n"
- "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n");
+ "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n"
#endif
#ifdef OS2
- PerlIO_printf(PIO_stdout,
"\n\nOS/2 port Copyright (c) 1990, 1991, Raymond Chen, Kai Uwe Rommel\n"
- "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n");
+ "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n"
#endif
#ifdef OEMVS
- PerlIO_printf(PIO_stdout,
- "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n");
+ "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"
#endif
#ifdef __VOS__
- PerlIO_printf(PIO_stdout,
- "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n");
+ "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n"
#endif
#ifdef POSIX_BC
- PerlIO_printf(PIO_stdout,
- "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n");
+ "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"
+#endif
+#ifdef __SYMBIAN32__
+ "Symbian port by Nokia, 2004-2005\n"
#endif
+/* UNDER_CE closes the printf call*/
#ifdef UNDER_CE
- PerlIO_printf(PIO_stdout,
"WINCE port by Rainer Keuchel, 2001-2002\n"
"Built on " __DATE__ " " __TIME__ "\n\n");
wce_hitreturn();
-#endif
-#ifdef __SYMBIAN32__
- PerlIO_printf(PIO_stdout,
- "Symbian port by Nokia, 2004-2005\n");
-#endif
+#endif /* end of additional copyright lines */
+
#ifdef BINARY_BUILD_NOTICE
+# ifndef UNDER_CE
+ ); /* close Larry Wall copyright printf */
+# endif
+/* ActivePerl uses BINARY_BUILD_NOTICE, it is a statement */
BINARY_BUILD_NOTICE;
#endif
+/* because of wce_hitreturn() open new printf for UNDER_CE */
+#if defined(BINARY_BUILD_NOTICE) || defined(UNDER_CE)
PerlIO_printf(PIO_stdout,
+/* else CPP catenation continues from Larry Wall copyright printf */
+#endif
"\n\
Perl may be copied only under the terms of either the Artistic License or the\n\
GNU General Public License, which may be found in the Perl 5 source kit.\n\n\
@@ -3622,6 +3640,13 @@ this system using \"man perl\" or \"perldoc perl\". If you have access to the\n
Internet, point your browser at http://www.perl.org/, the Perl Home Page.\n\n");
my_exit(0);
}
+#ifdef __GNUC__
+# if __GNUC__ == 4 && __GNUC_MINOR__ >= 6 || __GNUC__ > 4 /* 4.6 -> */
+# pragma GCC diagnostic pop
+# elif __GNUC__ == 4 && __GNUC_MINOR__ >= 2 || __GNUC__ > 4 /* 4.2 -> */
+# pragma GCC diagnostic warning "-Wformat"
+# endif
+#endif
/* compliments of Tom Christiansen */
--
1.8.0.msysgit.0
|
From @tonycozOn Tue Oct 29 15:07:46 2013, bulk88 wrote:
Silencing the warning does not make the code correct. It also doesn't fix the painful "where does this statement" end that was introduced in the original patch. To try and move forward: a) I'd love a patch with just the first part of the change - everything before "-#if defined(LOCAL_PATCH_COUNT)" in your latest patch. b) to make progress on the consolidation of the string literals I'd like to suggest the following changes to your approach: 1) retain the current #if defined(LOCAL_PATCH_COUNT) block, trying to fiddle with that in the way you have been is just too non-C. An optimizing compiler will remove the block when LOCAL_PATCH_COUNT is zero anyway 2) don't try to consolidate the "Perl may be copied ..." printf(), it's the main readability issue with your changes. 3) use a second #ifdef UNDER_CE for the wce_hitreturn rather than trying to close and open the printf around it. Tony |
From @bulk88On Mon Nov 11 16:53:38 2013, tonyc wrote:
1st half split out. Patch attached. I need more time to address the rest of your last post. The title was change, so the next half will be tentatively named "optimize the function " and not "remove the Foo from the function" which is this patch. Also RT # added in body of part 1/this patch. -- |
From @bulk880001-remove-PL_patchlevel-from-S_minus_v.patchFrom e8d5cdcfc749ebb9541705427c7eec832f432ace Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Tue, 12 Nov 2013 23:07:16 -0500
Subject: [PATCH] remove PL_patchlevel from S_minus_v
|SvPVX(vstringify(PL_patchlevel))| is the same string as
|"v" PERL_VERSION_STRING|. No need to go through the overhead of using a
version object. Instead of creating a SV, then further manipulating it.
Create and manipulate it at the same time with Perl_newSVpvf_nocontext or
newSVpvn. "num_len >= level_len " will be folded away by the compiler,
previously, since both lengths came from SVs, they were not const
technically. A very smart compiler might see strncmp/strnEQ takes all
const arguments, and will optimize that away also. VC 2003 didnt do that.
Change SvREFCNT_dec to SvREFCNT_dec_NN for obvious reasons.
There should not be any user visible changes to -v with this patch.
switches.t contains a regexp for -v already so no further tests were added.
This patch is part of #116296.
---
perl.c | 37 +++++++++++++++++++++----------------
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/perl.c b/perl.c
index 2892a87..e4310cd 100644
--- a/perl.c
+++ b/perl.c
@@ -3443,30 +3443,35 @@ STATIC void
S_minus_v(pTHX)
{
PerlIO * PIO_stdout;
- if (!sv_derived_from(PL_patchlevel, "version"))
- upg_version(PL_patchlevel, TRUE);
{
- SV* level= vstringify(PL_patchlevel);
+ const char * const level_str = "v" PERL_VERSION_STRING;
+ const STRLEN level_len = sizeof("v" PERL_VERSION_STRING)-1;
#ifdef PERL_PATCHNUM
+ SV* level;
# ifdef PERL_GIT_UNCOMMITTED_CHANGES
- SV *num = newSVpvs(PERL_PATCHNUM "*");
+ static const char num [] = PERL_PATCHNUM "*";
# else
- SV *num = newSVpvs(PERL_PATCHNUM);
+ static const char num [] = PERL_PATCHNUM;
# endif
{
- STRLEN level_len, num_len;
- char * level_str, * num_str;
- num_str = SvPV(num, num_len);
- level_str = SvPV(level, level_len);
- if (num_len>=level_len && strnEQ(num_str,level_str,level_len)) {
- SvREFCNT_dec(level);
- level= num;
+ const STRLEN num_len = sizeof(num)-1;
+ /* A very advanced compiler would fold away the strnEQ
+ and this whole conditional, but most (all?) won't do it.
+ SV level could also be replaced by with preprocessor
+ catenation.
+ */
+ if (num_len >= level_len && strnEQ(num,level_str,level_len)) {
+ /* per 46807d8e80, PERL_PATCHNUM is outside of the control
+ of the interp so it might contain format characters
+ */
+ level = newSVpvn(num, num_len);
} else {
- Perl_sv_catpvf(aTHX_ level, " (%"SVf")", num);
- SvREFCNT_dec(num);
+ level = Perl_newSVpvf_nocontext("%s (%s)", level_str, num);
}
}
- #endif
+#else
+ SV* level = newSVpvn(level_str, level_len);
+#endif /* #ifdef PERL_PATCHNUM */
PIO_stdout = PerlIO_stdout();
PerlIO_printf(PIO_stdout,
"\nThis is perl " STRINGIFY(PERL_REVISION)
@@ -3474,7 +3479,7 @@ S_minus_v(pTHX)
", subversion " STRINGIFY(PERL_SUBVERSION)
" (%"SVf") built for " ARCHNAME, level
);
- SvREFCNT_dec(level);
+ SvREFCNT_dec_NN(level);
}
#if defined(LOCAL_PATCH_COUNT)
if (LOCAL_PATCH_COUNT > 0)
--
1.8.0.msysgit.0
|
From @tonycozOn Tue Nov 12 20:15:03 2013, bulk88 wrote:
Thanks, applied as 709aee9. Tony |
From @bulk88On Mon Nov 11 16:53:38 2013, tonyc wrote:
CC warning free now, is it a ptr or is it an int in the format string design is gone.
.......
I changed the comments to be clearer where the concatenation goes. -- |
From @bulk880001-optimize-S_minus_v.patchFrom 407983156b9b3f0f58e179b653d900df7adb3b08 Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Sun, 22 Dec 2013 12:19:34 -0500
Subject: [PATCH] optimize S_minus_v
With the copyright lines, use more CPP cating. This reduced the number of
instructions, and removes a couple alignment null in .rdata image section.
UNDER_CE presents a challange because of the wce_hitreturn(); call.
BINARY_BUILD_NOTICE is arbitrary code. ActivePerl uses it and it contains
a perlio printf call. So that can not be CPP catted unfortunatly. If a
build is not UNDER_CE and BINARY_BUILD_NOTICE is undef, then the copyright
line is catted with the license line. LOCAL_PATCH_COUNT is a C expression,
like offsetof, which is not a preprocessor constant, so add
PERL_GIT_UNPUSHED_COMMITS_LEN and LOCAL_PATCH_COUNT_CONST which are pure
CPP constants. LOCAL_PATCH_COUNT can be changed to LOCAL_PATCH_COUNT_CONST
in the future.
Although S_minus_v is not hot and is rarely used, making the interp
image smaller, and therefore faster is a good idea. There should not be
any user visible changes to -v with this patch. switches.t contains a
regexp for -v already so no further tests were added.
This patch is part of #116296.
---
make_patchnum.pl | 9 ++++---
patchlevel.h | 10 +++++++++
perl.c | 60 ++++++++++++++++++++++++++++++-----------------------
3 files changed, 49 insertions(+), 30 deletions(-)
diff --git a/make_patchnum.pl b/make_patchnum.pl
index 3f857b5..82ec266 100644
--- a/make_patchnum.pl
+++ b/make_patchnum.pl
@@ -122,6 +122,7 @@ sub write_files {
return 0;
}
+my @unpushed_commits;
my $unpushed_commits = ' ';
my ($read, $branch, $snapshot_created, $commit_id, $describe)= ("") x 5;
my ($changed, $extra_info, $commit_title)= ("") x 3;
@@ -155,14 +156,13 @@ elsif (-d "$srcdir/.git") {
}
if (length $branch && length $remote) {
+ @unpushed_commits = grep {/\+/} backtick("git cherry $remote/$merge");
# git cherry $remote/$branch | awk 'BEGIN{ORS=","} /\+/ {print $2}' | sed -e 's/,$//'
my $unpushed_commit_list =
- join ",", map { (split /\s/, $_)[1] }
- grep {/\+/} backtick("git cherry $remote/$merge");
+ join ",", map { (split /\s/, $_)[1] } @unpushed_commits;
# git cherry $remote/$branch | awk 'BEGIN{ORS="\t\\\\\n"} /\+/ {print ",\"" $2 "\""}'
$unpushed_commits =
- join "", map { ',"'.(split /\s/, $_)[1]."\"\t\\\n" }
- grep {/\+/} backtick("git cherry $remote/$merge");
+ join "", map { ',"'.(split /\s/, $_)[1]."\"\t\\\n" } @unpushed_commits;
if (length $unpushed_commits) {
$commit_title = "Local Commit:";
my $ancestor = backtick("git rev-parse $remote/$merge");
@@ -185,6 +185,7 @@ write_files(<<"EOF_HEADER", <<"EOF_CONFIG");
* DO NOT EDIT DIRECTLY - edit make_patchnum.pl instead
***************************************************************************/
@{[$describe ? "#define PERL_PATCHNUM \"$describe\"" : ()]}
+#define PERL_GIT_UNPUSHED_COMMITS_LEN ${\scalar(@unpushed_commits)}
#define PERL_GIT_UNPUSHED_COMMITS\t\t\\
$unpushed_commits/*leave-this-comment*/
@{[$changed ? "#define PERL_GIT_UNCOMMITTED_CHANGES" : ()]}
diff --git a/patchlevel.h b/patchlevel.h
index a7e8dac..cd60a71 100644
--- a/patchlevel.h
+++ b/patchlevel.h
@@ -125,9 +125,11 @@ hunk.
# if defined(PERL_IS_MINIPERL)
# define PERL_PATCHNUM "UNKNOWN-miniperl"
# define PERL_GIT_UNPUSHED_COMMITS /*leave-this-comment*/
+# define PERL_GIT_UNPUSHED_COMMITS_LEN 0
# elif defined(PERL_MICRO)
# define PERL_PATCHNUM "UNKNOWN-microperl"
# define PERL_GIT_UNPUSHED_COMMITS /*leave-this-comment*/
+# define PERL_GIT_UNPUSHED_COMMITS_LEN 0
# else
#include "git_version.h"
# endif
@@ -146,6 +148,14 @@ static const char * const local_patches[] = {
# define LOCAL_PATCH_COUNT \
((int)(sizeof(local_patches)/sizeof(local_patches[0])-2))
+#ifdef PERL_GIT_UNCOMMITTED_CHANGES
+# define LOCAL_PATCH_COUNT_CONST \
+ (PERL_GIT_UNPUSHED_COMMITS_LEN+1)
+#else
+# define LOCAL_PATCH_COUNT_CONST \
+ (PERL_GIT_UNPUSHED_COMMITS_LEN)
+#endif
+
/* the old terms of reference, add them only when explicitly included */
#define PATCHLEVEL PERL_VERSION
#undef SUBVERSION /* OS/390 has a SUBVERSION in a system header */
diff --git a/perl.c b/perl.c
index 8271915..ad7db6a 100644
--- a/perl.c
+++ b/perl.c
@@ -3480,56 +3480,64 @@ S_minus_v(pTHX)
SvREFCNT_dec_NN(level);
}
#if defined(LOCAL_PATCH_COUNT)
- if (LOCAL_PATCH_COUNT > 0)
- PerlIO_printf(PIO_stdout,
- "\n(with %d registered patch%s, "
- "see perl -V for more detail)",
- LOCAL_PATCH_COUNT,
- (LOCAL_PATCH_COUNT!=1) ? "es" : "");
+ assert(LOCAL_PATCH_COUNT_CONST == LOCAL_PATCH_COUNT);
#endif
-
+/* start of Larry Wall copyright printf */
PerlIO_printf(PIO_stdout,
- "\n\nCopyright 1987-2013, Larry Wall\n");
+#if defined(LOCAL_PATCH_COUNT) && LOCAL_PATCH_COUNT_CONST != 0
+ /* LOCAL_PATCH_COUNT is a C constant, not a preprocessor constant */
+# if LOCAL_PATCH_COUNT_CONST == 1
+ "\n(with %d registered patch, see perl -V for more detail)%s"
+# else
+ "\n(with %d registered patches, see perl -V for more detail)%s"
+# endif
+ ,LOCAL_PATCH_COUNT_CONST,
+#else
+ /* use copyrights for format string */
+#endif
+ "\n\nCopyright 1987-2013, Larry Wall\n"
#ifdef MSDOS
- PerlIO_printf(PIO_stdout,
- "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n");
+ "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n"
#endif
#ifdef DJGPP
- PerlIO_printf(PIO_stdout,
"djgpp v2 port (jpl5003c) by Hirofumi Watanabe, 1996\n"
- "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n");
+ "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n"
#endif
#ifdef OS2
- PerlIO_printf(PIO_stdout,
"\n\nOS/2 port Copyright (c) 1990, 1991, Raymond Chen, Kai Uwe Rommel\n"
- "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n");
+ "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n"
#endif
#ifdef OEMVS
- PerlIO_printf(PIO_stdout,
- "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n");
+ "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"
#endif
#ifdef __VOS__
- PerlIO_printf(PIO_stdout,
- "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n");
+ "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n"
#endif
#ifdef POSIX_BC
- PerlIO_printf(PIO_stdout,
- "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n");
+ "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"
+#endif
+#ifdef __SYMBIAN32__
+ "Symbian port by Nokia, 2004-2005\n"
#endif
+/* UNDER_CE closes Larry Wall copyright printf */
#ifdef UNDER_CE
- PerlIO_printf(PIO_stdout,
"WINCE port by Rainer Keuchel, 2001-2002\n"
"Built on " __DATE__ " " __TIME__ "\n\n");
wce_hitreturn();
-#endif
-#ifdef __SYMBIAN32__
- PerlIO_printf(PIO_stdout,
- "Symbian port by Nokia, 2004-2005\n");
-#endif
+#endif /* end of additional copyright lines */
+
#ifdef BINARY_BUILD_NOTICE
+# ifndef UNDER_CE
+ ); /* close Larry Wall copyright printf */
+# endif
+/* ActivePerl uses BINARY_BUILD_NOTICE, it is a statement */
BINARY_BUILD_NOTICE;
#endif
+/* because of wce_hitreturn() open new printf for UNDER_CE */
+#if defined(BINARY_BUILD_NOTICE) || defined(UNDER_CE)
PerlIO_printf(PIO_stdout,
+/* else CPP catenation continues from Larry Wall copyright printf */
+#endif
"\n\
Perl may be copied only under the terms of either the Artistic License or the\n\
GNU General Public License, which may be found in the Perl 5 source kit.\n\n\
--
1.7.9.msysgit.0
|
From @bulk88On Sun Dec 22 09:23:16 2013, bulk88 wrote:
-- |
From @tonycozOn Wed Mar 05 04:49:43 2014, bulk88 wrote:
This fails to apply to blead, I haven't tried to apply it manually. Tony |
From @bulk88On Fri Mar 07 19:00:30 2014, tonyc wrote:
I fixed the conflicts and created a new patch. Patch is testing right now on my system. -- |
From @bulk88On Fri Mar 07 20:09:09 2014, bulk88 wrote:
-- |
From @bulk88On Fri Mar 07 20:09:36 2014, bulk88 wrote:
New patch against blead attached. -- |
From @bulk880001-optimize-S_minus_v.patchFrom b5787d4e10533e116b931570f821bb7254fe2721 Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Sun, 22 Dec 2013 12:19:34 -0500
Subject: [PATCH] optimize S_minus_v
With the copyright lines, use more CPP cating. This reduced the number of
instructions, and removes a couple alignment null in .rdata image section.
UNDER_CE presents a challange because of the wce_hitreturn(); call.
BINARY_BUILD_NOTICE is arbitrary code. ActivePerl uses it and it contains
a perlio printf call. So that can not be CPP catted unfortunatly. If a
build is not UNDER_CE and BINARY_BUILD_NOTICE is undef, then the copyright
line is catted with the license line. LOCAL_PATCH_COUNT is a C expression,
like offsetof, which is not a preprocessor constant, so add
PERL_GIT_UNPUSHED_COMMITS_LEN and LOCAL_PATCH_COUNT_CONST which are pure
CPP constants. LOCAL_PATCH_COUNT can be changed to LOCAL_PATCH_COUNT_CONST
in the future.
Although S_minus_v is not hot and is rarely used, making the interp
image smaller, and therefore faster is a good idea. There should not be
any user visible changes to -v with this patch. switches.t contains a
regexp for -v already so no further tests were added.
This patch is part of #116296.
---
make_patchnum.pl | 9 ++++---
patchlevel.h | 10 +++++++++
perl.c | 60 ++++++++++++++++++++++++++++++-----------------------
3 files changed, 49 insertions(+), 30 deletions(-)
diff --git a/make_patchnum.pl b/make_patchnum.pl
index 3f857b5..82ec266 100644
--- a/make_patchnum.pl
+++ b/make_patchnum.pl
@@ -122,6 +122,7 @@ sub write_files {
return 0;
}
+my @unpushed_commits;
my $unpushed_commits = ' ';
my ($read, $branch, $snapshot_created, $commit_id, $describe)= ("") x 5;
my ($changed, $extra_info, $commit_title)= ("") x 3;
@@ -155,14 +156,13 @@ elsif (-d "$srcdir/.git") {
}
if (length $branch && length $remote) {
+ @unpushed_commits = grep {/\+/} backtick("git cherry $remote/$merge");
# git cherry $remote/$branch | awk 'BEGIN{ORS=","} /\+/ {print $2}' | sed -e 's/,$//'
my $unpushed_commit_list =
- join ",", map { (split /\s/, $_)[1] }
- grep {/\+/} backtick("git cherry $remote/$merge");
+ join ",", map { (split /\s/, $_)[1] } @unpushed_commits;
# git cherry $remote/$branch | awk 'BEGIN{ORS="\t\\\\\n"} /\+/ {print ",\"" $2 "\""}'
$unpushed_commits =
- join "", map { ',"'.(split /\s/, $_)[1]."\"\t\\\n" }
- grep {/\+/} backtick("git cherry $remote/$merge");
+ join "", map { ',"'.(split /\s/, $_)[1]."\"\t\\\n" } @unpushed_commits;
if (length $unpushed_commits) {
$commit_title = "Local Commit:";
my $ancestor = backtick("git rev-parse $remote/$merge");
@@ -185,6 +185,7 @@ write_files(<<"EOF_HEADER", <<"EOF_CONFIG");
* DO NOT EDIT DIRECTLY - edit make_patchnum.pl instead
***************************************************************************/
@{[$describe ? "#define PERL_PATCHNUM \"$describe\"" : ()]}
+#define PERL_GIT_UNPUSHED_COMMITS_LEN ${\scalar(@unpushed_commits)}
#define PERL_GIT_UNPUSHED_COMMITS\t\t\\
$unpushed_commits/*leave-this-comment*/
@{[$changed ? "#define PERL_GIT_UNCOMMITTED_CHANGES" : ()]}
diff --git a/patchlevel.h b/patchlevel.h
index 37dda6a..39191fb 100644
--- a/patchlevel.h
+++ b/patchlevel.h
@@ -125,9 +125,11 @@ hunk.
# if defined(PERL_IS_MINIPERL)
# define PERL_PATCHNUM "UNKNOWN-miniperl"
# define PERL_GIT_UNPUSHED_COMMITS /*leave-this-comment*/
+# define PERL_GIT_UNPUSHED_COMMITS_LEN 0
# elif defined(PERL_MICRO)
# define PERL_PATCHNUM "UNKNOWN-microperl"
# define PERL_GIT_UNPUSHED_COMMITS /*leave-this-comment*/
+# define PERL_GIT_UNPUSHED_COMMITS_LEN 0
# else
#include "git_version.h"
# endif
@@ -146,6 +148,14 @@ static const char * const local_patches[] = {
# define LOCAL_PATCH_COUNT \
((int)(sizeof(local_patches)/sizeof(local_patches[0])-2))
+#ifdef PERL_GIT_UNCOMMITTED_CHANGES
+# define LOCAL_PATCH_COUNT_CONST \
+ (PERL_GIT_UNPUSHED_COMMITS_LEN+1)
+#else
+# define LOCAL_PATCH_COUNT_CONST \
+ (PERL_GIT_UNPUSHED_COMMITS_LEN)
+#endif
+
/* the old terms of reference, add them only when explicitly included */
#define PATCHLEVEL PERL_VERSION
#undef SUBVERSION /* OS/390 has a SUBVERSION in a system header */
diff --git a/perl.c b/perl.c
index 36d33d9..cb7d62a 100644
--- a/perl.c
+++ b/perl.c
@@ -3498,56 +3498,64 @@ S_minus_v(pTHX)
SvREFCNT_dec_NN(level);
}
#if defined(LOCAL_PATCH_COUNT)
- if (LOCAL_PATCH_COUNT > 0)
- PerlIO_printf(PIO_stdout,
- "\n(with %d registered patch%s, "
- "see perl -V for more detail)",
- LOCAL_PATCH_COUNT,
- (LOCAL_PATCH_COUNT!=1) ? "es" : "");
+ assert(LOCAL_PATCH_COUNT_CONST == LOCAL_PATCH_COUNT);
#endif
-
+/* start of Larry Wall copyright printf */
PerlIO_printf(PIO_stdout,
- "\n\nCopyright 1987-2014, Larry Wall\n");
+#if defined(LOCAL_PATCH_COUNT) && LOCAL_PATCH_COUNT_CONST != 0
+ /* LOCAL_PATCH_COUNT is a C constant, not a preprocessor constant */
+# if LOCAL_PATCH_COUNT_CONST == 1
+ "\n(with %d registered patch, see perl -V for more detail)%s"
+# else
+ "\n(with %d registered patches, see perl -V for more detail)%s"
+# endif
+ ,LOCAL_PATCH_COUNT_CONST,
+#else
+ /* use copyrights for format string */
+#endif
+ "\n\nCopyright 1987-2014, Larry Wall\n"
#ifdef MSDOS
- PerlIO_printf(PIO_stdout,
- "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n");
+ "\nMS-DOS port Copyright (c) 1989, 1990, Diomidis Spinellis\n"
#endif
#ifdef DJGPP
- PerlIO_printf(PIO_stdout,
"djgpp v2 port (jpl5003c) by Hirofumi Watanabe, 1996\n"
- "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n");
+ "djgpp v2 port (perl5004+) by Laszlo Molnar, 1997-1999\n"
#endif
#ifdef OS2
- PerlIO_printf(PIO_stdout,
"\n\nOS/2 port Copyright (c) 1990, 1991, Raymond Chen, Kai Uwe Rommel\n"
- "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n");
+ "Version 5 port Copyright (c) 1994-2002, Andreas Kaiser, Ilya Zakharevich\n"
#endif
#ifdef OEMVS
- PerlIO_printf(PIO_stdout,
- "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n");
+ "MVS (OS390) port by Mortice Kern Systems, 1997-1999\n"
#endif
#ifdef __VOS__
- PerlIO_printf(PIO_stdout,
- "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n");
+ "Stratus OpenVOS port by Paul.Green@stratus.com, 1997-2013\n"
#endif
#ifdef POSIX_BC
- PerlIO_printf(PIO_stdout,
- "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n");
+ "BS2000 (POSIX) port by Start Amadeus GmbH, 1998-1999\n"
+#endif
+#ifdef __SYMBIAN32__
+ "Symbian port by Nokia, 2004-2005\n"
#endif
+/* UNDER_CE closes Larry Wall copyright printf */
#ifdef UNDER_CE
- PerlIO_printf(PIO_stdout,
"WINCE port by Rainer Keuchel, 2001-2002\n"
"Built on " __DATE__ " " __TIME__ "\n\n");
wce_hitreturn();
-#endif
-#ifdef __SYMBIAN32__
- PerlIO_printf(PIO_stdout,
- "Symbian port by Nokia, 2004-2005\n");
-#endif
+#endif /* end of additional copyright lines */
+
#ifdef BINARY_BUILD_NOTICE
+# ifndef UNDER_CE
+ ); /* close Larry Wall copyright printf */
+# endif
+/* ActivePerl uses BINARY_BUILD_NOTICE, it is a statement */
BINARY_BUILD_NOTICE;
#endif
+/* because of wce_hitreturn() open new printf for UNDER_CE */
+#if defined(BINARY_BUILD_NOTICE) || defined(UNDER_CE)
PerlIO_printf(PIO_stdout,
+/* else CPP catenation continues from Larry Wall copyright printf */
+#endif
"\n\
Perl may be copied only under the terms of either the Artistic License or the\n\
GNU General Public License, which may be found in the Perl 5 source kit.\n\n\
--
1.7.9.msysgit.0
|
From @bulk88On Fri Mar 07 22:02:52 2014, bulk88 wrote:
Bump. -- |
From @rjbsWhile I'd like to see this ticket reviewed, I am declining to mark it as a blocker for 5.20.0. -- |
From @bulk88On Mon Mar 24 07:16:06 2014, rjbs wrote:
Bump. -- |
From @tonycozOn Thu Apr 03 23:23:16 2014, bulk88 wrote:
I won't accept it while that printf() can end in more than one place. Right now it can end in three different places: #ifdef UNDER_CE Unfortunately I expect that will remove the rather trivial space improvement the patch brings. Otherwise, I'm worried about the issue Nicholas mentioned, where some compilers might choke on the mix of pre-processor directives and string literal concatenation. Tony |
Migrated from rt.perl.org#116296 (status was 'open')
Searchable as RT116296$
The text was updated successfully, but these errors were encountered: