Bug 152662

Summary: StringBuilder often creates two StringImpls.
Product: WebKit Reporter: Andreas Kling <kling>
Component: Web Template FrameworkAssignee: Andreas Kling <kling>
Status: REOPENED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, commit-queue, kling, simon.fraser
Priority: P2 Keywords: Performance
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Proposed patch none

Andreas Kling
Reported 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.
Attachments
Proposed patch (2.50 KB, patch)
2016-01-03 01:29 PST, Andreas Kling 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Andreas Kling
Comment 1 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.
WebKit Commit Bot
Comment 2 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>
WebKit Commit Bot
Comment 3 2016-01-03 08:12:47 PST
All reviewed patches have been landed. Closing bug.
Simon Fraser (smfr)
Comment 4 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]
Simon Fraser (smfr)
Comment 5 2016-01-03 09:37:08 PST
It also broke two tests: Tests that failed: StringBuilderTest.Append StringBuilderTest.ToStringPreserveCapacity
Andreas Kling
Comment 6 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>
Note You need to log in before you can comment on or make changes to this bug.