Make general use of new NSPR rotate macros

RESOLVED FIXED in mozilla1.9beta4

Status

()

Core
General
P2
normal
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Steve Snyder, Assigned: Steve Snyder)

Tracking

({perf})

Trunk
mozilla1.9beta4
x86
All
Points:
---
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

11 years ago
Created attachment 300895 [details] [diff] [review]
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.
(Assignee)

Updated

11 years ago
Keywords: perf
(Assignee)

Updated

11 years ago
Depends on: 331043

Comment 1

11 years ago
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.
(Assignee)

Comment 2

11 years ago
Created attachment 300900 [details] [diff] [review]
Rotate bits left by 4, not right by 28

Per comment.
Attachment #300895 - Attachment is obsolete: true
Component: General → General
Product: Firefox → Core
QA Contact: general → general

Comment 3

11 years ago
Noming for the easy perf win.
Flags: blocking1.9?

Updated

11 years ago
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2

Comment 4

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

let's get it reviewed
Attachment #300900 - Flags: review?(wtc)

Updated

11 years ago
Attachment #300900 - Flags: superreview?(dbaron)
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.)
Attachment #300900 - Flags: superreview?(dbaron) → superreview+

Comment 6

11 years ago
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.
Attachment #300900 - Flags: review?(wtc) → review+

Comment 7

11 years ago
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.
(Assignee)

Comment 8

11 years ago
Created attachment 302627 [details] [diff] [review]
Per comments #6 and #7
Attachment #300900 - Attachment is obsolete: true
Keywords: checkin-needed

Comment 9

11 years ago
(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

11 years ago
Sorry.  I meant "prudent".  I'm surprised that other people misspelled it
the same way :-)
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4

Comment 12

11 years ago
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.
You need to log in before you can comment on or make changes to this bug.