Bug 50517

Summary: Add a fast case for ASCII strings in HashAndUTF8CharactersTranslator::equal
Product: WebKit Reporter: Patrick R. Gansterer <paroga>
Component: New BugsAssignee: Patrick R. Gansterer <paroga>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, eric
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 51612    
Bug Blocks: 43085    
Attachments:
Description Flags
Patch
darin: review+
Patch none

Patrick R. Gansterer
Reported 2010-12-04 06:08:50 PST
Add a fast case for ASCII strings in HashAndUTF8CharactersTranslator::equal
Attachments
Patch (2.01 KB, patch)
2010-12-04 06:10 PST, Patrick R. Gansterer 		darin: review+
DetailsFormatted DiffDiff
Patch (1.94 KB, patch)
2010-12-26 15:31 PST, Patrick R. Gansterer 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Patrick R. Gansterer
Comment 1 2010-12-04 06:10:36 PST
Created attachment 75607 [details] Patch
Eric Seidel (no email)
Comment 2 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.
Darin Adler
Comment 3 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?
Eric Seidel (no email)
Comment 4 2010-12-13 11:38:33 PST
I'm also not sure Darin and I disagree here either. :)
Patrick R. Gansterer
Comment 5 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.
Darin Adler
Comment 6 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().
Eric Seidel (no email)
Comment 7 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.
Eric Seidel (no email)
Comment 8 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.
Patrick R. Gansterer
Comment 9 2010-12-26 15:31:30 PST
Created attachment 77465 [details] Patch I'll cq+ this, when the xml parser benchmark is landed.
WebKit Commit Bot
Comment 10 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>
WebKit Commit Bot
Comment 11 2010-12-31 04:43:55 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.