Closed
Bug 415262
Opened 16 years ago
Closed 16 years ago
Make general use of new NSPR rotate macros
Categories
(Core :: General, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla1.9beta4
People
(Reporter: swsnyder, Assigned: swsnyder)
References
Details
(Keywords: perf)
Attachments
(1 file, 2 obsolete files)
10.46 KB,
patch
|
Details | Diff | Splinter Review |
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•16 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.
Updated•16 years ago
|
Product: Firefox → Core
QA Contact: general → general
Updated•16 years ago
|
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Comment 4•16 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•16 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•16 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•16 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•16 years ago
|
||
Attachment #300900 -
Attachment is obsolete: true
Updated•16 years ago
|
Keywords: checkin-needed
Comment 9•16 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•16 years ago
|
||
Sorry. I meant "prudent". I'm surprised that other people misspelled it the same way :-)
Comment 11•16 years ago
|
||
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
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
Comment 12•16 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.
Description
•