Bug 73457

Summary: 8 bit string work slows down Kraken json-stringify-tinderbox
Product: WebKit Reporter: Michael Saboff <msaboff>
Component: JavaScriptCoreAssignee: Michael Saboff <msaboff>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, fpizlo, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 73236    
Attachments:
Description Flags
Patch
webkit-ews: commit-queue-
Patch updated with StringBuilder::characters() named correctly
webkit.review.bot: commit-queue-
Patch with speculative fix for chromium linux
none
Patch with fix for Win build failure oliver: review+

Michael Saboff
Reported 2011-11-30 09:35:27 PST
It appears that json-stringify-tinderbox slows down in part due to StringBuilder converting 8 bit string to 16 bit strings.
Attachments
Patch (20.75 KB, patch)
2011-11-30 12:58 PST, Michael Saboff
webkit-ews: commit-queue-
Patch updated with StringBuilder::characters() named correctly (20.79 KB, patch)
2011-11-30 13:55 PST, Michael Saboff
webkit.review.bot: commit-queue-
Patch with speculative fix for chromium linux (20.82 KB, patch)
2011-12-01 12:29 PST, Michael Saboff
no flags
Patch with fix for Win build failure (21.01 KB, patch)
2011-12-02 14:41 PST, Michael Saboff
oliver: review+
Michael Saboff
Comment 1 2011-11-30 12:58:20 PST
Michael Saboff
Comment 2 2011-11-30 13:06:20 PST
Forgot to mention that this change eliminates the slowdown to json-stringify-tinderbox. It does introduce a 4.4% slowdown to json-parse-financial. That will be addressed in a subsequent patch to StringImpl::equal. The attached proposed patch along with the proposed StringImpl::equal give Kraken a slight improvement (.5% using the arithmetic mean and 1% using the geometric mean) when compared to r100509 using bencher.
Early Warning System Bot
Comment 3 2011-11-30 13:14:37 PST
Gyuyoung Kim
Comment 4 2011-11-30 13:27:14 PST
Michael Saboff
Comment 5 2011-11-30 13:55:00 PST
Created attachment 117266 [details] Patch updated with StringBuilder::characters() named correctly The prior performance comments hold for this patch.
WebKit Review Bot
Comment 6 2011-11-30 21:54:53 PST
Comment on attachment 117266 [details] Patch updated with StringBuilder::characters() named correctly Attachment 117266 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10696506
Michael Saboff
Comment 7 2011-12-01 12:29:36 PST
Created attachment 117457 [details] Patch with speculative fix for chromium linux
Filip Pizlo
Comment 8 2011-12-01 14:36:49 PST
Can you include the perf effect in the ChangeLog, and post the results here? Makes it easier to track things down in case somewhere there is a slow-down.
Michael Saboff
Comment 9 2011-12-01 17:03:34 PST
(In reply to comment #8) > Can you include the perf effect in the ChangeLog, and post the results here? > > Makes it easier to track things down in case somewhere there is a slow-down. Added the following to the ChangeLog: This change eliminates 5% of the 7% slowdown to json-stringify-tinderbox. This change introduces a 4.8% slowdown to json-parse-financial. Both of these remaining slowdowns will be addressed in a subsequent patch to StringImpl::equal.
Sam Weinig
Comment 10 2011-12-01 19:42:06 PST
Comment on attachment 117457 [details] Patch with speculative fix for chromium linux View in context: https://bugs.webkit.org/attachment.cgi?id=117457&action=review > Source/JavaScriptCore/wtf/text/StringBuilder.cpp:156 > + else if (m_buffer->hasOneRef()) > + m_buffer = StringImpl::reallocate(m_buffer.release(), requiredLength, m_bufferCharacters16); Indentation issue.
Michael Saboff
Comment 11 2011-12-02 14:41:58 PST
Created attachment 117697 [details] Patch with fix for Win build failure This also includes the requested performance comment and indentation fix.
Michael Saboff
Comment 12 2011-12-05 11:09:00 PST
Ryosuke Niwa
Comment 13 2011-12-06 22:27:58 PST
It appears that this patch regressed StringBuilder: http://build.webkit.org/builders/Chromium%20Linux%20Release%20%28Tests%29/builds/26827/steps/run-api-tests/logs/stdio [ RUN ] StringBuilderTest.Append Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:53: Failure Value of: String(builder.characters(), builder.length()) Actual: 0123456789abcd7 Expected: String(expected) Which is: 0123456789abcdefg Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:53: Failure Value of: String(builder.characters(), builder.length()) Actual: 0123456789abcd7 Expected: String(expected) Which is: 0123456789abcdefg Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:53: Failure Value of: String(builder.characters(), builder.length()) Actual: 0123456789abcd7 Expected: String(expected) Which is: 0123456789abcdefg# Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:53: Failure Value of: String(builder.characters(), builder.length()) Actual: 0123456789abcd7 Expected: String(expected) Which is: 0123456789abcdefg# Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:53: Failure Value of: String(builder.characters(), builder.length()) Actual: 0123456789abcdefg#0123456789abcd7 Expected: String(expected) Which is: 0123456789abcdefg#0123456789abcdefg#XYZ [ FAILED ] StringBuilderTest.Append (1 ms) [----------] 1 test from StringBuilderTest (1 ms total) [ RUN ] StringBuilderTest.ToStringPreserveCapacity Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:153: Failure Value of: string1 Actual: 0123456789abcdefghijklmnopqrstuvwxyz Expected: String("0123456789abcdefghijklmnopqrstuvwxyzABCDEF") Which is: 0123456789abcdefghijklmnopqrstuvwxyzABCDEF [ FAILED ] StringBuilderTest.ToStringPreserveCapacity (0 ms)
Note You need to log in before you can comment on or make changes to this bug.