Bug 73457 - 8 bit string work slows down Kraken json-stringify-tinderbox
: 8 bit string work slows down Kraken json-stringify-tinderbox
Status: RESOLVED FIXED
: WebKit
JavaScriptCore
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 73236
  Show dependency treegraph
 
Reported: 2011-11-30 09:35 PST by
Modified: 2011-12-06 22:27 PST (History)


Attachments
Patch (20.75 KB, patch)
2011-11-30 12:58 PST, Michael Saboff
webkit-ews: commit‑queue-
Review Patch | 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-
Review Patch | 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 Review Patch | 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+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2011-11-30 12:58:20 PST -------
Created an attachment (id=117258) [details]
Patch
------- Comment #2 From 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 From 2011-11-30 13:14:37 PST -------
(From update of attachment 117258 [details])
Attachment 117258 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10704273
------- Comment #4 From 2011-11-30 13:27:14 PST -------
(From update of attachment 117258 [details])
Attachment 117258 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10697288
------- Comment #5 From 2011-11-30 13:55:00 PST -------
Created an attachment (id=117266) [details]
Patch updated with StringBuilder::characters() named correctly

The prior performance comments hold for this patch.
------- Comment #6 From 2011-11-30 21:54:53 PST -------
(From update of attachment 117266 [details])
Attachment 117266 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10696506
------- Comment #7 From 2011-12-01 12:29:36 PST -------
Created an attachment (id=117457) [details]
Patch with speculative fix for chromium linux
------- Comment #8 From 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 From 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 From 2011-12-01 19:42:06 PST -------
(From update of attachment 117457 [details])
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 From 2011-12-02 14:41:58 PST -------
Created an attachment (id=117697) [details]
Patch with fix for Win build failure

This also includes the requested performance comment and indentation fix.
------- Comment #12 From 2011-12-05 11:09:00 PST -------
Committed r102017: <http://trac.webkit.org/changeset/102017>
------- Comment #13 From 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)