Boris Zbarsky [:bzbarsky] Reporter
	Description • 19 years ago

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).

	Brendan Eich [:brendan]
	Comment 1 • 19 years ago

Mmmm, factoring.  If C++ doesn't help, use a C preprocessor macro!  Ok, I'm
old.... ;-)

/be

	Darin Fisher
	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?

	Boris Zbarsky [:bzbarsky] 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.

	Brian Ryner (not reading)
	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.

	Peter Van der Beken [:peterv]
	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.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 6 • 19 years ago

note also bug 231782

	Benjamin Smedberg
	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!

	Peter Van der Beken [:peterv]
	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? ;-)

	Johnny Stenback (:jst)
	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.

	Boris Zbarsky [:bzbarsky] 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?

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 11 • 19 years ago

Fixed by patch for bug 314465.