develooper Front page | perl.perl5.porters | Postings from July 2013

[perl #116296] [PATCH] remove PL_patchlevel from and optimize S_minus_v

Thread Previous | Thread Next
From:
bulk88 via RT
Date:
July 8, 2013 22:36
Subject:
[perl #116296] [PATCH] remove PL_patchlevel from and optimize S_minus_v
Message ID:
rt-3.6.HEAD-2552-1373322995-645.116296-15-0@perl.org
On Sat Jul 06 22:00:36 2013, public@khwilliamson.com wrote:
> 
> I agree with Dave, and I believe it is counterproductive to write code 
> based on how current particular hardware works, when that code will long 
> outlive the hardware and likely the whole underlying mechanism.
>

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
for current hardware.

Some commits (all authors) talk about cache alignment and struct
alignment. Perl already has a bias towards current hardware and not
theoretical hardware from the future, and a strong "there is no C
compiler but GCC bias" (why all the PERL_GCC_BRACE_GROUPS_FORBIDDEN
code?). Arguing over what the future hardware will look like isn't
productive so I won't go down that path. My point is Perl is biased
towards today's processors, and today's OSes, and today's compilers, and
biased towards different ones at the same time. When I hear "your
favorites aren't my favorites so burn in hell", I see that commenter is
wearing rose glasses and it is political/religious BS.

> But suppose we are wrong; suppose it has to be implemented the way you 
> did.  It's not at all obvious that it has to be that way.  Therefore 
> there should be comments in the code as to your reasoning.  If not, the 
> next guy might come along and not realize that this implementation has 
> to be this particular way, and change it out.  If indeed it is required 
> to be this way, that might introduce subtle failures that the 
> accompanying tests don't catch.  It's unfair to those who follow to not 
> comment such subtleties.
>

There is a comment that explains what is going on "+/* else CPP
catenation continues from Larry Wall copyright printf */". That explains
the design of what the CPP conditionals are doing and suggests to keep
the CPP concating.

On Sun Jul 07 23:20:13 2013, tonyc wrote:
> 
> While Dave was wrong about the registers, his general argument about
> the micro-optimization is correct, MSVC isn't the only compiler.
> 
> With gcc/clang the change *costs* 5 bytes[1] - while making the code a
> little harder to read.  Both gcc and clang optimize out the
> conditional.
>

Those are good compilers if they do that then but gcc/clang aren't the
whole world.  I will change the "level = Perl_newSVpvf_nocontext("%s",
num);" branch to newSVpvn.

> > > The downside is that the src code becomes more bloated and confusing.
> > 
> > And this is why people give up, switch languages, then tell all their
> > friends Perl [5 or Perl world] is dead. If the git repo is locked, code
> > will never become more confusing. The assumption that progress is for
> > communists and hippies is the death of Perl 5.
> 
> I think it's great that you're enthusiastic about optimizing perl's
> implementation, but treating a comment about fairly questionable
> optimization as an attack on all changes is reaching.

The same opinions are coming out again, self-righteous "if I wouldn't
change it, you have no right to either"
https://rt.perl.org/rt3/Ticket/Display.html?id=116443
https://rt.perl.org/rt3/Ticket/Display.html?id=115894
https://rt.perl.org/rt3/Ticket/Display.html?id=116188

> 
> We optimize for several things, generated code size is important, but
> not more important than a) developer time - the poor sob who comes
> along in 6 months or 5 years and needs to understand the code, b)
> performance.

Who is "the poor sob who comes along in 6 months or 5 years and needs to
understand the code"? Commiters are held to no standard of code clarity
or documentation. I've never seen a revert commit to revert something
for lack of comments or clarity. Why is it said by others in the Perl
world (YAPCNA) only a dozen people on earth can hack the C guts? 

a recent one
http://perl5.git.perl.org/perl.git/commitdiff/0c0d42fffa5c2ddd284610f681b65d355471189b
What systems? Tested on any? Seen it in a man page?

http://perl5.git.perl.org/perl.git/commit/28e464fba839d02f376952199261fc8b7d58a0d8
nothing! get the Ouija board out!

http://perl5.git.perl.org/perl.git/commit/88675427278c9e6329fba9382072cae9dd00c163
what commit?

http://perl5.git.perl.org/perl.git/commit/175bc858cac2126947f4b8fa838835417c1430d1
faster? slower?

https://rt.perl.org/rt3/Ticket/Display.html?id=116925 when will
THINKFIRST be documented?

> 
> And code size doesn't map one-to-one to performance - several
> performance optimizations produce significantly larger code.
> 
> Performance doesn't matter much for S_minus_v(), but developer time
> matters as much there as it does anywhere else.
>

S_minus_v never gets changed except for copyright dates. Ideally it
should be a single string written by some .pl script into a .h during
the build process, but that is too complex for me to implement (and will
break every build except Windows if I try), plus removing 
BINARY_BUILD_NOTICE and getting AS and DARKPAN to not use it and
introducing a BINARY_BUILD_NOTICE_S macro and "#ifdef
BINARY_BUILD_NOTICE\n#error BINARY_BUILD_NOTICE has been removed, use
BINARY_BUILD_NOTICE_S." is too difficult for me.

On Sun Jul 07 23:53:42 2013, tonyc wrote:
> > - #endif
> > +#else
> > +	SV* level = newSVpvn(level_str, level_len);
> > +#endif /* #ifdef PERL_PATCHNUM */
> 
> Except (as discussed) the use of newSVpvn(), I think it's good up to
> here.

Will switch to newSVpvn as said above.

> >  	}
> > -#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
> 
> 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,

I knew that would be a portability problem, so the NULL went there.
I didn't want to try to use more complicated features of
sv_vcatpvfn_flags (t or z letter) due to sv_vcatpvfn_flags crashing on
me when using fancy features (bug filed). Google says implicit type
promotion so I guess the size letters have to go in. "%.0s" produces no
output if the param is null, and therefore skips the param. "%.0s" is a
work around to my original idea which became bug #118775.

> 
> It's also unnecessarily duplicative, in that if the registered patches
> note needed to change, it will need to be changed in two places.

The alternative was more complicated (putting each litteral into a
macro, then putting many instances of each macro). The 2 lines are next
to each other, nobody is likely to change them. If they do, they can see
it on the same screen. 3 seconds of work is nothing.

> 
> gcc and clang complain about the type too, gcc:
> 
> perl.c: In function S_minus_v:
> perl.c:3551:39: warning: pointer/integer type mismatch in conditional
> expression [enabled by default]
> perl.c:3596:9: warning: format %d expects argument of type int,
> but argument 3 has type void * [-Wformat]
> 
> clang produces a more understandable message:
> 
> perl.c:3551:9: warning: format specifies type 'int' but the argument
> has type
>       'void *' [-Wformat]
>         LOCAL_PATCH_COUNT <= 0 ? NULL : LOCAL_PATCH_COUNT,
>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ./patchlevel.h:147:2: note: expanded from macro 'LOCAL_PATCH_COUNT'
>         ((int)(sizeof(local_patches)/sizeof(local_patches[0])-2))
>         ^
> perl.c:3551:9: warning: format specifies type 'int' but the argument
> has type
>       'void *' [-Wformat]
>         LOCAL_PATCH_COUNT <= 0 ? NULL : LOCAL_PATCH_COUNT,
>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ./patchlevel.h:147:2: note: expanded from macro 'LOCAL_PATCH_COUNT'
>         ((int)(sizeof(local_patches)/sizeof(local_patches[0])-2))
> 
> (I think it warns twice because you provide three different format
> strings, two of which don't match.)
> 
> The original code is clearer I think.
>

I could break out the LOCAL_PATCH_COUNT stuff into a separate
printf/remove it (as it was originally).

> 
> Rather than trying to build it all into one printf, I think it would
> be more understandable if the printf() started at the Larry Wall line,
> and the wce_hitreturn() got its own extra #ifdef UNDER_CE, so:
> 
> PerlIO_printf(PIO_stdout,
>      larry's line
> #ifdef platform
>      platform line
> #endif
> ...
>     );
> #ifdef UNDER_CE
>     wce_hitreturn();
> #endif
> 
> Tony
> 

The GPL line wont be concatted with the larry CR and platform CR in that
case(">     );").  Leaving the only optimization being dec_NN and
concating of larry CR with platform CR.
http://perl5.git.perl.org/perl.git/commit/e1caacb4fdb62cb28dc825ca0115faf95e569339?f=perl.c
I think the wce_hitreturn (implemented in wince.c) has a screen pager
purpose due to the tiny screen size of a WinCE device, so it has to be
between the larry CR/platform CR and the GPL part on WinCE.

-- 
bulk88 ~ bulk88 at hotmail.com

---
via perlbug:  queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=116296

Thread Previous | Thread Next


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About