Bug 152662 - StringBuilder often creates two StringImpls.
Summary: StringBuilder often creates two StringImpls.
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords: Performance
Depends on:
Blocks:
 
Reported: 2016-01-03 01:25 PST by Andreas Kling
Modified: 2016-01-03 12:37 PST (History)
6 users (show)

See Also:


Attachments
Proposed patch (2.50 KB, patch)
2016-01-03 01:29 PST, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2016-01-03 01:25:45 PST
Sometimes StringBuilder will keep the internal buffer used during building, and the returned "built" StringImpl will be a substring that retains the buffer. This way we keep two StringImpls around instead of one.
Comment 1 Andreas Kling 2016-01-03 01:29:15 PST
Created attachment 268134 [details]
Proposed patch

Here's one way of making this better: adjusting the length field of the buffer and then returning it as the final string. This way we keep the slightly oversized buffer allocation, but don't allocate an extra StringImpl.
Comment 2 WebKit Commit Bot 2016-01-03 08:12:42 PST
Comment on attachment 268134 [details]
Proposed patch

Clearing flags on attachment: 268134

Committed r194510: <http://trac.webkit.org/changeset/194510>
Comment 3 WebKit Commit Bot 2016-01-03 08:12:47 PST
All reviewed patches have been landed.  Closing bug.
Comment 4 Simon Fraser (smfr) 2016-01-03 09:36:00 PST
This broke the windows build:

C:\cygwin\home\buildbot\slave\win-debug\build\Source\WTF\wtf\text\StringBuilder.cpp(63): error C2248: 'WTF::StringImpl::m_length': cannot access private member declared in class 'WTF::StringImpl' [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WTF\wtf\WTF.vcxproj]
Comment 5 Simon Fraser (smfr) 2016-01-03 09:37:08 PST
It also broke two tests:

Tests that failed:
  StringBuilderTest.Append
  StringBuilderTest.ToStringPreserveCapacity
Comment 6 Andreas Kling 2016-01-03 12:37:52 PST
Reverted r194510 for reason:

Broke 2 API tests, no time to investigate right now

Committed r194519: <http://trac.webkit.org/changeset/194519>