Bug 73457 - 8 bit string work slows down Kraken json-stringify-tinderbox
Summary: 8 bit string work slows down Kraken json-stringify-tinderbox
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks: 73236
  Show dependency treegraph
 
Reported: 2011-11-30 09:35 PST by Michael Saboff
Modified: 2011-12-06 22:27 PST (History)
4 users (show)

See Also:


Attachments
Patch (20.75 KB, patch)
2011-11-30 12:58 PST, Michael Saboff
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch updated with StringBuilder::characters() named correctly (20.79 KB, patch)
2011-11-30 13:55 PST, Michael Saboff
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch with speculative fix for chromium linux (20.82 KB, patch)
2011-12-01 12:29 PST, Michael Saboff
no flags Details | Formatted Diff | Diff
Patch with fix for Win build failure (21.01 KB, patch)
2011-12-02 14:41 PST, Michael Saboff
oliver: 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-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.
Comment 1 Michael Saboff 2011-11-30 12:58:20 PST
Created attachment 117258 [details]
Patch
Comment 2 Michael Saboff 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.
Comment 3 Early Warning System Bot 2011-11-30 13:14:37 PST
Comment on attachment 117258 [details]
Patch

Attachment 117258 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10704273
Comment 4 Gyuyoung Kim 2011-11-30 13:27:14 PST
Comment on attachment 117258 [details]
Patch

Attachment 117258 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10697288
Comment 5 Michael Saboff 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.
Comment 6 WebKit Review Bot 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
Comment 7 Michael Saboff 2011-12-01 12:29:36 PST
Created attachment 117457 [details]
Patch with speculative fix for chromium linux
Comment 8 Filip Pizlo 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.
Comment 9 Michael Saboff 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.
Comment 10 Sam Weinig 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.
Comment 11 Michael Saboff 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.
Comment 12 Michael Saboff 2011-12-05 11:09:00 PST
Committed r102017: <http://trac.webkit.org/changeset/102017>
Comment 13 Ryosuke Niwa 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)