Closed
Bug 307601
Opened 19 years ago
Closed 19 years ago
Comparing atoms to nsAString is slow
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: dougt)
References
Details
(Keywords: perf)
nsIAtom.Equals(AString) involves a string copy plus conversion to do the comparison. I think it would be better to have a comparator that goes along and does on-the-fly comparison... This would probably mean factoring the conversion code out of ConvertUTF8toUTF16, which already lives in XPCOM. I know jst has mentioned this idea on and off, and I could really use it about now, what with bug 307600 (where we're possibly doing a bunch of direct string-to-atom compares).
Comment 1•19 years ago
|
||
Mmmm, factoring. If C++ doesn't help, use a C preprocessor macro! Ok, I'm old.... ;-) /be
Comment 2•19 years ago
|
||
I thought that jst (or bryner?) had code for this already and found that it made no measurable improvement. Am I dreaming?
![]() |
Reporter | |
Comment 3•19 years ago
|
||
Not sure... I did check for bugs on this and didn't see any... If there's a patch floating about, I would be happy to profile it on the testcases I have here.
Comment 4•19 years ago
|
||
I had written some code to do a non-copying comparison of UTF-8 to UTF-16 that could be used here. I didn't measure any speedup. Unfortunately, I'm fairly certain that I've lost that patch, unless jst has it.
Comment 5•19 years ago
|
||
See attachment 172366 [details] [diff] [review] in bug 277479 for jst's CompareUTF8toUTF16. Not sure why that never got checked in, iirc it was a nice speedup.
Comment 7•19 years ago
|
||
Bug 277479 was not checked in because there were no clients. As soon as there is a client which wants to use that code, let's get it landed!
Comment 8•19 years ago
|
||
(In reply to comment #7) > Bug 277479 was not checked in because there were no clients. As soon as there is > a client which wants to use that code, let's get it landed! https://bugzilla.mozilla.org/show_bug.cgi?id=277479#c14 quotes improvements for atom creation from char*, nsAString& and PRUnichar*. Is no one creating atoms anymore? ;-)
Comment 9•19 years ago
|
||
(In reply to comment #4) > I had written some code to do a non-copying comparison of UTF-8 to UTF-16 that > could be used here. I didn't measure any speedup. Unfortunately, I'm fairly > certain that I've lost that patch, unless jst has it. Bryner, I've still got that patch, it's part of attachment 172366 [details] [diff] [review] in bug 277479.
![]() |
Reporter | |
Comment 10•19 years ago
|
||
OK, I just tried that attachment on a simple testcase -- document.images['imageid'] for a document with a bunch of images. The results are: Without patch: 70098 hits under nsContentList::NamedItem, of which 21739 are under AtomImpl::Equals With patch: 59130 hits under nsContentList::NamedItem, of which 9822 are under AtomImpl::Equals So that patch makes AtomImpl::Equals over two times faster, and makes this quite real-life testcase about 15% faster. Given that, I think we should just get that patch in on the trunk, either in this bug or in bug 277479. Who would need to review it?
Depends on: 277479
![]() |
Reporter | |
Comment 11•19 years ago
|
||
Fixed by patch for bug 314465.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•