From @bulk88

See attached patch.

From @bulk88

From 030ed137171a0d895b1b36f35c8a10c274a4535d Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Sun, 23 Dec 2012 21:18:24 -0500
Subject: [PATCH] hv.c: add some SvREFCNT_dec_NN and S_hv_free_ent_ret NULL
 check removal

Add some SvREFCNT_dec_NNs to hv.c. In hv_clear, stop 2 C level checks for
null. I didn't check the machine code to see if compiler optimized away
or not, but fix it on a C level anyways. Use Perl_croak_nocontext to save
a push instruction in threaded perls. In the 2 places where
S_hv_free_ent_ret is called (Perl_hv_free_ent and Perl_hfree_next_entry)
HE * entry is either checked for null, or dereferenced before the call to
S_hv_free_ent_ret. Change embed.fnc to reflect removal of the NULL check.

Purpose is less machine instructions/faster code.
---
 embed.fnc |    2 +-
 hv.c      |   19 +++++++++----------
 proto.h   |    5 +++--
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/embed.fnc b/embed.fnc
index 75688fb..1951bf3 100644
--- a/embed.fnc
+++ b/embed.fnc
@@ -1745,7 +1745,7 @@ po	|SV*	|hfree_next_entry	|NN HV *hv|NN STRLEN *indexp
 #if defined(PERL_IN_HV_C)
 s	|void	|hsplit		|NN HV *hv
 s	|void	|hfreeentries	|NN HV *hv
-s	|SV*	|hv_free_ent_ret|NN HV *hv|NULLOK HE *entryK
+s	|SV*	|hv_free_ent_ret|NN HV *hv|NN HE *entryK
 sa	|HE*	|new_he
 sanR	|HEK*	|save_hek_flags	|NN const char *str|I32 len|U32 hash|int flags
 sn	|void	|hv_magic_check	|NN HV *hv|NN bool *needs_copy|NN bool *needs_store
diff --git a/hv.c b/hv.c
index 966a12f..5f7ae85 100644
--- a/hv.c
+++ b/hv.c
@@ -1377,7 +1377,7 @@ Perl_hv_copy_hints_hv(pTHX_ HV *const ohv)
 	    else {
 		(void)hv_common(hv, heksv, HeKEY(entry), HeKLEN(entry),
 				 HeKFLAGS(entry), HV_FETCH_ISSTORE|HV_FETCH_JUST_SV, sv, HeHASH(entry));
-		SvREFCNT_dec(heksv);
+		SvREFCNT_dec_NN(heksv);
 	    }
 	}
 	HvRITER_set(ohv, riter);
@@ -1399,8 +1399,6 @@ S_hv_free_ent_ret(pTHX_ HV *hv, HE *entry)
 
     PERL_ARGS_ASSERT_HV_FREE_ENT_RET;
 
-    if (!entry)
-	return NULL;
     val = HeVAL(entry);
     if (HeKLEN(entry) == HEf_SVKEY) {
 	SvREFCNT_dec(HeKEY_sv(entry));
@@ -1481,14 +1479,15 @@ Perl_hv_clear(pTHX_ HV *hv)
 	    for (; entry; entry = HeNEXT(entry)) {
 		/* not already placeholder */
 		if (HeVAL(entry) != &PL_sv_placeholder) {
-		    if (HeVAL(entry) && SvREADONLY(HeVAL(entry))
-		     && !SvIsCOW(HeVAL(entry))) {
-			SV* const keysv = hv_iterkeysv(entry);
-			Perl_croak(aTHX_
-				   "Attempt to delete readonly key '%"SVf"' from a restricted hash",
-				   (void*)keysv);
+		    if (HeVAL(entry)) {
+			if (SvREADONLY(HeVAL(entry)) && !SvIsCOW(HeVAL(entry))) {
+			    SV* const keysv = hv_iterkeysv(entry);
+			    Perl_croak_nocontext(
+				"Attempt to delete readonly key '%"SVf"' from a restricted hash",
+				(void*)keysv);
+			}
+			SvREFCNT_dec_NN(HeVAL(entry));
 		    }
-		    SvREFCNT_dec(HeVAL(entry));
 		    HeVAL(entry) = &PL_sv_placeholder;
 		    HvPLACEHOLDERS(hv)++;
 		}
diff --git a/proto.h b/proto.h
index ee2b14e..413434b 100644
--- a/proto.h
+++ b/proto.h
@@ -5685,9 +5685,10 @@ STATIC struct xpvhv_aux*	S_hv_auxinit(HV *hv)
 
 STATIC SV*	S_hv_delete_common(pTHX_ HV *hv, SV *keysv, const char *key, STRLEN klen, int k_flags, I32 d_flags, U32 hash);
 STATIC SV*	S_hv_free_ent_ret(pTHX_ HV *hv, HE *entryK)
-			__attribute__nonnull__(pTHX_1);
+			__attribute__nonnull__(pTHX_1)
+			__attribute__nonnull__(pTHX_2);
 #define PERL_ARGS_ASSERT_HV_FREE_ENT_RET	\
-	assert(hv)
+	assert(hv); assert(entryK)
 
 STATIC void	S_hv_magic_check(HV *hv, bool *needs_copy, bool *needs_store)
 			__attribute__nonnull__(1)
-- 
1.7.9.msysgit.0

From @doughera88

On Sun, 23 Dec 2012, bulk88 wrote​:

# New Ticket Created by bulk88
# Please include the string​: [perl #116188]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=116188 >

[minor nit​: This patch breaks a debugging build because embed.fnc
incorrectly spells the second argument as 'entryK', while the actual
functions use 'entry'. I fixed this in commit
c77ce6f. I then tweaked this patch
to use 'entry' instead of 'entryK'.]

I tried this patch and tested it on Linux/amd64 with gcc-4.7.2,
with various optimizer settings.

It looks like this patch is doing good things -- avoiding redundant NULL
checks, avoiding duplicating a test, removing an unnecessary context,
etc., and doing it in a set of functions that gets called fairly often.
However, I found that it made no difference in performance in running a
pattern matching program similar to metaconfig, and (under -O3) the new
object files are actually larger than the old ones.

In both cases, gcc inlines the S_hv_free_ent_ret function anyway, and the
resulting Perl_hv_free_ent function is exactly the same size before and
after the patch.

I also tested with Sun's cc compiler. It ended up giving the same
performance and the same object size before and after the patch.

So I'm not sure if it's worth applying or not. What do you think?

--
  Andy Dougherty doughera@​lafayette.edu

The RT System itself - Status changed from 'new' to 'open'

From @bulk88

On Fri Jan 25 12​:44​:16 2013, doughera wrote​:

On Sun, 23 Dec 2012, bulk88 wrote​:

# New Ticket Created by bulk88
# Please include the string​: [perl #116188]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=116188 >

[minor nit​: This patch breaks a debugging build because embed.fnc
incorrectly spells the second argument as 'entryK', while the actual
functions use 'entry'. I fixed this in commit
c77ce6f. I then tweaked this patch
to use 'entry' instead of 'entryK'.]

I tried this patch and tested it on Linux/amd64 with gcc-4.7.2,
with various optimizer settings.

It looks like this patch is doing good things -- avoiding redundant NULL
checks, avoiding duplicating a test, removing an unnecessary context,
etc., and doing it in a set of functions that gets called fairly often.
However, I found that it made no difference in performance in running a
pattern matching program similar to metaconfig, and (under -O3) the new
object files are actually larger than the old ones.

I would say removing a null check is unmeasurable unless you are using
rdtsc. Can you post before and after perl binaries, and the perf numbers
you got in detail? I'd like to see myself how the object files got larger.

In both cases, gcc inlines the S_hv_free_ent_ret function anyway, and the
resulting Perl_hv_free_ent function is exactly the same size before and
after the patch.

VC -O1 doesn't inline SvREFCNT_dec_NN or S_hv_free_ent_ret.
__forceinline vs __inline issue specific to VC.

I also tested with Sun's cc compiler. It ended up giving the same
performance and the same object size before and after the patch.

So I'm not sure if it's worth applying or not. What do you think?

I would apply it. There were a couple of other places in hv.c where I
tried a _NN but got segvs. I think that I went through everything I
thought could get an _NN in hv.c and gave it a _NN if possible. This
patch was the only things that were successful in _NN conversion. I
don't remember if I skipped any SvREFCNT_decs due to lack of familiarity.

I use -O1. Are you using a threaded or unthreaded perl? is your sun x86
or sparc?

--
bulk88 ~ bulk88 at hotmail.com

From @tsee

On 01/25/2013 09​:43 PM, Andy Dougherty wrote​:

So I'm not sure if it's worth applying or not. What do you think?

All of this just my opinion​: Unless there's a particular reason
(maintainability, etc) not to, yes, worth applying. Removing a branching
instruction by itself wouldn't be measurable at a macroscopic level
unless it's in extraordinarily hot code. But doing so in many place
eventually should be.

--Steffen

From @iabyn

On Sat, Jan 26, 2013 at 11​:16​:37AM +0100, Steffen Mueller wrote​:

On 01/25/2013 09​:43 PM, Andy Dougherty wrote​:

So I'm not sure if it's worth applying or not. What do you think?

All of this just my opinion​: Unless there's a particular reason
(maintainability, etc) not to, yes, worth applying. Removing a
branching instruction by itself wouldn't be measurable at a
macroscopic level unless it's in extraordinarily hot code. But doing
so in many place eventually should be.

I agree. Regardless of any insignificant performance gains, the new code
itself is slightly cleaner, and it's always good to declare function args NN
where possible; so I've a applied it as

  a03199e

but I rewrote the commit message to be clearer.

--
I don't want to achieve immortality through my work...
I want to achieve it through not dying.
  -- Woody Allen

From @doughera88

On Fri, 25 Jan 2013, bulk88 via RT wrote​:

On Fri Jan 25 12​:44​:16 2013, doughera wrote​:

It looks like this patch is doing good things -- avoiding redundant NULL
checks, avoiding duplicating a test, removing an unnecessary context,
etc., and doing it in a set of functions that gets called fairly often.
However, I found that it made no difference in performance in running a
pattern matching program similar to metaconfig, and (under -O3) the new
object files are actually larger than the old ones.

I would say removing a null check is unmeasurable unless you are using
rdtsc. Can you post before and after perl binaries, and the perf numbers
you got in detail? I'd like to see myself how the object files got larger.

I'm afraid I have no idea what 'rdtsc' is, nor what you mean by 'perf
numbers'. In any case, I no longer have all the files on hand and doubt
its worth pursuing.

I use -O1. Are you using a threaded or unthreaded perl? is your sun x86
or sparc?

There are too many combinations. Depending on what else is going on, I
have access to a SPARC which can run Sun's cc or gcc, Solaris on amd64
which runs Sun's cc, and Linux on both x86 and amd64, which can run either
gcc or Sun's cc. Occasionally, I have access to various *BSDs, though
they all use gcc so I didn't bother checking with them. In this case, I
tried a variety of threaded and unthreaded builds on Linux, and unthreaded
on SPARC/cc. There are also different optimization flags, depending on
what you want to optimize for (e.g. size vs. speed) and how aggressively
you want to do so.

--
  Andy Dougherty doughera@​lafayette.edu

From @tonycoz

On Tue Jan 29 03​:19​:44 2013, davem wrote​:

I agree. Regardless of any insignificant performance gains, the new code
itself is slightly cleaner, and it's always good to declare function
args NN
where possible; so I've a applied it as

a03199eaa6f5d9e2d15c64750229e2adeebfbdce

but I rewrote the commit message to be clearer.

Unfortunately you messed up the author email address.

The attached patch has headers​:

From 030ed137171a0d895b1b36f35c8a10c274a4535d Mon Sep 17 00​:00​:00 2001
From​: Daniel Dragan <bulk88@​hotmail.com>
Date​: Sun, 23 Dec 2012 21​:18​:24 -0500

but I guess you <git am>ed the whole mailbox entry instead of the
attachment.

Tony

From @bulk88

On Tue Jan 29 19​:09​:07 2013, tonyc wrote​:

Unfortunately you messed up the author email address.

The attached patch has headers​:

From 030ed137171a0d895b1b36f35c8a10c274a4535d Mon Sep 17 00​:00​:00 2001
From​: Daniel Dragan <bulk88@​hotmail.com>
Date​: Sun, 23 Dec 2012 21​:18​:24 -0500

but I guess you <git am>ed the whole mailbox entry instead of the
attachment.

Tony

I think its time for a git hook to stop any commits with perlbug as the
author's email,
http​://perl5.git.perl.org/perl.git?a=search&h=HEAD&st=author&s=perlbug .

--
bulk88 ~ bulk88 at hotmail.com

@iabyn - Status changed from 'open' to 'resolved'