Bug 50517 – Add a fast case for ASCII strings in HashAndUTF8CharactersTranslator::equal

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Patrick R. Gansterer]

Add a fast case for ASCII strings in HashAndUTF8CharactersTranslator::equal

[Patrick R. Gansterer]

Created attachment 75607 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 75607 [details]
Patch

Again, on what benchmark?  So much code for unclear gains.  Please document your measurement techniques in the ChangeLog.

[Darin Adler]

Comment on attachment 75607 [details]
Patch

Again, I don’t quite see eye to eye with Eric on this. This optimization looks like a good idea and is not a lot more code.

But I’d like to understand more what this makes faster. What platform? What test?

[Eric Seidel (no email)]

I'm also not sure Darin and I disagree here either. :)

[Patrick R. Gansterer]

Same as in https://bugs.webkit.org/show_bug.cgi?id=50516#c9 (and sorry for the delay):

                     avg        median    stdev
 
OS X 10.6 original   >4000      >4000     ;-)
r74063               2185.72    2182      12.126895728091345
r74063 with patch    2127.83    2118.5    75.47424130125455
                     -2.65%     -2,91%


Maybe someone else can confirm the speed increase? I'm not sure if this (and the change in bug 50516 - both) cause such a "huge" performance win.

[Darin Adler]

Comment on attachment 75607 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75607&action=review

I think it’s OK to land this patch. I’m going to change the review to review+. Eric, please do let me know if you object. I am not trying to override you.

> JavaScriptCore/wtf/text/AtomicString.cpp:235
> +            return equalUTF16WithUTF8(string->characters(), string->characters() + string->length(), buffer.characters, buffer.characters + buffer.length);

We have stringCharacters in a local variable, and should use it instead of string->characters().

[Eric Seidel (no email)]

I would like Patrick to update the ChangeLog to explain his testing, and ideally add the benchmark to our repository under WebCore/benchmarks.

Without clear (repeatable) testing, optimizing like this makes little sense to me. :)

Thank you for reviewing Darin.

[Eric Seidel (no email)]

Comment on attachment 75607 [details]
Patch

Eh.  Looking at this patch again, it looks clearly right.  Still would be good to know what perf testing is being done here.

[Patrick R. Gansterer]

Created attachment 77465 [details]
Patch

I'll cq+ this, when the xml parser benchmark is landed.

[WebKit Commit Bot]

Comment on attachment 77465 [details]
Patch

Clearing flags on attachment: 77465

Committed r74829: <http://trac.webkit.org/changeset/74829>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.