Bug 50517 - Add a fast case for ASCII strings in HashAndUTF8CharactersTranslator::equal
Summary: Add a fast case for ASCII strings in HashAndUTF8CharactersTranslator::equal
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Patrick R. Gansterer
URL:
Keywords:
Depends on: 51612
Blocks: 43085
  Show dependency treegraph
 
Reported: 2010-12-04 06:08 PST by Patrick R. Gansterer
Modified: 2010-12-31 04:43 PST (History)
3 users (show)

See Also:


Attachments
Patch (2.01 KB, patch)
2010-12-04 06:10 PST, Patrick R. Gansterer
darin: review+
Details | Formatted Diff | Diff
Patch (1.94 KB, patch)
2010-12-26 15:31 PST, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick R. Gansterer 2010-12-04 06:08:50 PST
Add a fast case for ASCII strings in HashAndUTF8CharactersTranslator::equal
Comment 1 Patrick R. Gansterer 2010-12-04 06:10:36 PST
Created attachment 75607 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-12-12 02:33:51 PST
Comment on attachment 75607 [details]
Patch

Again, on what benchmark?  So much code for unclear gains.  Please document your measurement techniques in the ChangeLog.
Comment 3 Darin Adler 2010-12-13 10:06:47 PST
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?
Comment 4 Eric Seidel (no email) 2010-12-13 11:38:33 PST
I'm also not sure Darin and I disagree here either. :)
Comment 5 Patrick R. Gansterer 2010-12-16 03:05:29 PST
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.
Comment 6 Darin Adler 2010-12-16 11:06:31 PST
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().
Comment 7 Eric Seidel (no email) 2010-12-16 13:57:15 PST
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.
Comment 8 Eric Seidel (no email) 2010-12-20 23:04:24 PST
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.
Comment 9 Patrick R. Gansterer 2010-12-26 15:31:30 PST
Created attachment 77465 [details]
Patch

I'll cq+ this, when the xml parser benchmark is landed.
Comment 10 WebKit Commit Bot 2010-12-31 04:43:48 PST
Comment on attachment 77465 [details]
Patch

Clearing flags on attachment: 77465

Committed r74829: <http://trac.webkit.org/changeset/74829>
Comment 11 WebKit Commit Bot 2010-12-31 04:43:55 PST
All reviewed patches have been landed.  Closing bug.