If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Comparing atoms to nsAString is slow

RESOLVED FIXED

Status

()

Core
XPCOM
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: bz, Assigned: dougt)

Tracking

({perf})

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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).
Mmmm, factoring.  If C++ doesn't help, use a C preprocessor macro!  Ok, I'm
old.... ;-)

/be

Comment 2

12 years ago
I thought that jst (or bryner?) had code for this already and found that it made
no measurable improvement.  Am I dreaming?
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.
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.
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.
note also bug 231782

Comment 7

12 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!
(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? ;-)
(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.
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
Blocks: 307832
Depends on: 314465
Fixed by patch for bug 314465.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.