415262 - Make general use of new NSPR rotate macros

Status: RESOLVED FIXED

(Core :: General, defect, P2)

Description

[Steve Snyder]

Attachment: General use of NSPR rotate macros

Replace several uses of shift/xor, intended to rotate bits, with the use of the new NSPR rotate macros.  The goal is to improve performance by replacing the existing code with the hardware rotate instructions that the macros alias.

Comment 1

[Wan-Teh Chang]

Comment on attachment 300895 [details] [diff] [review]
General use of NSPR rotate macros

These should all use the equivalent PR_ROTATE_LEFT32(x, 4) instead.

Comment 2

[Steve Snyder]

Attachment: Rotate bits left by 4, not right by 28

Per comment.

Comment 3

[Mike Schroepfer]

Noming for the easy perf win.

Comment 4

[Robert Sayre]

Comment on attachment 300900 [details] [diff] [review]
Rotate bits left by 4, not right by 28

let's get it reviewed

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 300900 [details] [diff] [review]
Rotate bits left by 4, not right by 28

>diff -Nru mozilla-1.9b3pre-20080201.orig/content/base/src/nsDOMAttributeMap.h mozilla/content/base/src/nsDOMAttributeMap.h
>-      return (aKey->mNamespaceID >> 28) ^
>-             (aKey->mNamespaceID << 4) ^
>-             NS_PTR_TO_INT32(aKey->mLocalName);
>+      return( PR_ROTATE_LEFT32(aKey->mNamespaceID, 4) ^ NS_PTR_TO_INT32(aKey->mLocalName) );

You should also cast aKey->mNamespaceID to PRUint32 here inside the macro parameter.  (It's currently PRInt32, which means behavior is implementation-defined, although as long as it's consistent it wouldn't break anything, although could slow it down.  If it's a sign-extend, there's clearly no real functional harm, except it prevents good compilers (as documented in http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/nsprpub/pr/include/prbit.h&rev=3.12&mark=115-116#110 ) from converting to a rotate.)

sr=dbaron with that change

(I wonder whether the macro itself should contain the cast.)

Comment 6

[Wan-Teh Chang]

Comment on attachment 300900 [details] [diff] [review]
Rotate bits left by 4, not right by 28

r=wtc.  Note the following comments.

In mozilla/content/base/src/nsDOMAttributeMap.h:

>-      return (aKey->mNamespaceID >> 28) ^
>-             (aKey->mNamespaceID << 4) ^
>-             NS_PTR_TO_INT32(aKey->mLocalName);
>+      return( PR_ROTATE_LEFT32(aKey->mNamespaceID, 4) ^ NS_PTR_TO_INT32(aKey->mLocalName) );

Please remove the outermost parentheses.

Since aKey->mNamespaceID is PRInt32, a signed type, the new code
is not equivalent to the original code (see bug 331043 comment 86).
But I think that's OK.

Comment 7

[Wan-Teh Chang]

Comment on attachment 300900 [details] [diff] [review]
Rotate bits left by 4, not right by 28

>+      return( PR_ROTATE_LEFT32(aKey->mNamespaceID, 4) ^ NS_PTR_TO_INT32(aKey->mLocalName) );

It may be preduent to use static_cast<PRUint32>(aKey->mNamespaceID)
here.

Comment 8

[Steve Snyder]

Attachment: Per comments #6 and #7

Comment 9

[Jeff Walden [:Waldo]]

(In reply to comment #7)
> It may be preduent to use static_cast<PRUint32>(aKey->mNamespaceID)
> here.

Sorry, gotta ask -- what word did you mean instead of "preduent", which Google's only seen 102 times?  I don't recognize any obvious misspelling.  :-)

Comment 10

[Wan-Teh Chang]

Sorry.  I meant "prudent".  I'm surprised that other people misspelled it
the same way :-)

Comment 11

[Reed Loden [:reed]]

Checking in content/base/src/nsDOMAttributeMap.h;
/cvsroot/mozilla/content/base/src/nsDOMAttributeMap.h,v  <--  nsDOMAttributeMap.h
new revision: 1.26; previous revision: 1.25
done
Checking in content/xul/document/src/nsElementMap.cpp;
/cvsroot/mozilla/content/xul/document/src/nsElementMap.cpp,v  <--  nsElementMap.cpp
new revision: 1.29; previous revision: 1.28
done
Checking in netwerk/cache/src/nsDiskCacheDevice.cpp;
/cvsroot/mozilla/netwerk/cache/src/nsDiskCacheDevice.cpp,v  <--  nsDiskCacheDevice.cpp
new revision: 1.108; previous revision: 1.107
done
Checking in netwerk/protocol/http/src/nsHttp.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttp.cpp,v  <--  nsHttp.cpp
new revision: 1.12; previous revision: 1.11
done
Checking in rdf/base/src/nsRDFService.cpp;
/cvsroot/mozilla/rdf/base/src/nsRDFService.cpp,v  <--  nsRDFService.cpp
new revision: 1.114; previous revision: 1.113
done
Checking in xpcom/ds/nsCRT.cpp;
/cvsroot/mozilla/xpcom/ds/nsCRT.cpp,v  <--  nsCRT.cpp
new revision: 3.63; previous revision: 3.62
done
Checking in xpcom/ds/nsStaticNameTable.cpp;
/cvsroot/mozilla/xpcom/ds/nsStaticNameTable.cpp,v  <--  nsStaticNameTable.cpp
new revision: 1.25; previous revision: 1.24
done
Checking in xpcom/glue/nsTHashtable.cpp;
/cvsroot/mozilla/xpcom/glue/nsTHashtable.cpp,v  <--  nsTHashtable.cpp
new revision: 3.8; previous revision: 3.7
done
Checking in xpcom/glue/pldhash.c;
/cvsroot/mozilla/xpcom/glue/pldhash.c,v  <--  pldhash.c
new revision: 3.12; previous revision: 3.11
done

Comment 12

[jag (Peter Annema)]

I think the PRUint32 cast should be done inside the PR_ROTATE macros. Otherwise someone will, sooner or later, get bitten when they pass in a PRInt32 and no one notices.