Bug 73584 - Changes proposed for 73457 slow down Kraken json-parse-financial
: Changes proposed for 73457 slow down Kraken json-parse-financial
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Michael Saboff
:
Depends on:
Blocks: 73236
  Show dependency treegraph
 
Reported: 2011-12-01 13:11 PST by Michael Saboff
Modified: 2011-12-05 15:20 PST (History)
0 users

See Also:


Attachments
Patch (4.97 KB, patch)
2011-12-01 13:25 PST, Michael Saboff
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2011-12-01 13:11:43 PST
The changes proposed for bug 73457 slow down Kraken json-parse-financial by about 3%.
Comment 1 Michael Saboff 2011-12-01 13:25:27 PST
Created attachment 117475 [details]
Patch
Comment 2 Darin Adler 2011-12-01 15:07:26 PST
Comment on attachment 117475 [details]
Patch

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

Why 4 bytes and not 8 on 64-bit architectures? Can this share code with StringHash.h instead of adding more copies of code that does the same thing?

> Source/JavaScriptCore/wtf/text/StringImpl.cpp:714
> +#if !(CPU(ARM) || CPU(SH4) || CPU(MIPS) || CPU(SPARC))

StringHash.h uses a different list for this.
Comment 3 Sam Weinig 2011-12-01 19:44:16 PST
Comment on attachment 117475 [details]
Patch

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

Please take Darin's comment into account.

> Source/JavaScriptCore/wtf/text/StringImpl.cpp:754
> +    if (length & 1 &&  *reinterpret_cast<const UChar*>(aCharacters) != *reinterpret_cast<const UChar*>(bCharacters))

Extra whitespace between &&  *
Comment 4 Michael Saboff 2011-12-01 22:35:08 PST
(In reply to comment #2)
> (From update of attachment 117475 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117475&action=review
> 
> Why 4 bytes and not 8 on 64-bit architectures? Can this share code with StringHash.h instead of adding more copies of code that does the same thing?
> 
> > Source/JavaScriptCore/wtf/text/StringImpl.cpp:714
> > +#if !(CPU(ARM) || CPU(SH4) || CPU(MIPS) || CPU(SPARC))
> 
> StringHash.h uses a different list for this.

Spent more time on this and added 64 bit comparisons for x86-64.  This change sped up SunSpider string-unpack-code by ~3%.

Made an initial stab at merging the StringHash code with this code, but it proved to be more work than expected. Putting the new equal() implementations in StringImpl.h and calling them from StringHash.h didn't work.  The compiler tried to convert the first argument LChar* to a String which ended up with a recursive call.  Created https://bugs.webkit.org/show_bug.cgi?id=73622 "Eliminate Duplicate word at a time equal code in StringImpl.cpp and StringHash.h" to address this duplicity.
Comment 5 Michael Saboff 2011-12-01 23:06:56 PST
Committed r101747: <http://trac.webkit.org/changeset/101747>