Bug 200862

Summary: Use makeString and multi-argument StringBuilder::append instead of less efficient multiple appends
Product: WebKit Reporter: Darin Adler <darin>
Component: Web Template FrameworkAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, ews-watchlist, rniwa, sam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 200921    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews101 for mac-highsierra
none
Archive of layout-test-results from ews115 for mac-highsierra
none
Archive of layout-test-results from ews215 for win-future
none
Patch
none
Archive of layout-test-results from ews100 for mac-highsierra
none
Archive of layout-test-results from ews115 for mac-highsierra
none
Patch rniwa: review+

Darin Adler
Reported 2019-08-17 14:03:27 PDT
Use makeString and multi-argument StringBuilder::append instead of less efficient multiple appends
Attachments
Patch (61.56 KB, patch)
2019-08-17 16:35 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews101 for mac-highsierra (3.65 MB, application/zip)
2019-08-17 17:39 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews115 for mac-highsierra (3.88 MB, application/zip)
2019-08-17 18:16 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews215 for win-future (13.97 MB, application/zip)
2019-08-17 18:28 PDT, EWS Watchlist 		no flags
Details
Patch (61.65 KB, patch)
2019-08-18 08:18 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews100 for mac-highsierra (3.26 MB, application/zip)
2019-08-18 09:24 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews115 for mac-highsierra (3.51 MB, application/zip)
2019-08-18 10:16 PDT, EWS Watchlist 		no flags
Details
Patch (69.74 KB, patch)
2019-08-21 19:05 PDT, Darin Adler 		rniwa: review+
DetailsFormatted DiffDiff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.
Darin Adler
Comment 1 2019-08-17 16:35:58 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 2 2019-08-17 17:39:40 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 3 2019-08-17 17:39:41 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 4 2019-08-17 18:16:29 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 5 2019-08-17 18:16:31 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 6 2019-08-17 18:28:24 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 7 2019-08-17 18:28:27 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 8 2019-08-17 18:44:17 PDT Comment hidden (obsolete)
Darin Adler
Comment 9 2019-08-18 08:18:14 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 10 2019-08-18 09:24:28 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 11 2019-08-18 09:24:30 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 12 2019-08-18 10:16:26 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 13 2019-08-18 10:16:27 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 14 2019-08-18 10:25:36 PDT Comment hidden (obsolete)
Sam Weinig
Comment 15 2019-08-18 15:32:30 PDT
Comment on attachment 376639 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376639&action=review > Source/WebCore/css/CSSFontFaceRule.cpp:53 > StringBuilder result; This is unused and can be removed. > Source/WebCore/css/CSSFontFaceSrcValue.cpp:77 > result.appendLiteral("local("); > else > result.appendLiteral("url("); > - result.append(m_resource); > - result.append(')'); > - if (!m_format.isEmpty()) { > - result.appendLiteral(" format("); > - result.append(m_format); > - result.append(')'); > - } > + result.append(m_resource, ')'); It might read a little nicer to put literals ("local(" or "url(") as a ternary expression for the first parameter. > Source/WebCore/css/CSSGradientValue.cpp:941 > result.appendLiteral("color-stop("); > result.appendFixedPrecisionNumber(position); > - result.appendLiteral(", "); > - result.append(stop.m_color->cssText()); > - result.append(')'); > + result.append(", ", stop.m_color->cssText(), ')'); If you wanted to, you could collapse all the appends here doing, result.append("color-stop(", FormattedNumber::fixedPrecision(position), ", ", stop.m_color->cssText(), ')').
Darin Adler
Comment 16 2019-08-18 16:37:36 PDT
Looks like the test failures indicate a failure of StringBuilder::append!
Darin Adler
Comment 17 2019-08-18 16:47:23 PDT
(In reply to Darin Adler from comment #16) > Looks like the test failures indicate a failure of StringBuilder::append! Yes, I think I see a bug in StringBuilder::appendForAdapters when the string is 8-bit, the thing to be appended is 16-bit, and the 8-bit buffer is big enough to hold the new length. There's no call to allocateBufferUpConvert in that case, yet there needs to be.
Darin Adler
Comment 18 2019-08-18 16:56:05 PDT
Comment on attachment 376639 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376639&action=review I’ll upload a new patch after I fix the bug in StringBuilder::append. >> Source/WebCore/css/CSSFontFaceRule.cpp:53 >> StringBuilder result; > > This is unused and can be removed. Done. >> Source/WebCore/css/CSSFontFaceSrcValue.cpp:77 >> + result.append(m_resource, ')'); > > It might read a little nicer to put literals ("local(" or "url(") as a ternary expression for the first parameter. Done, and then converted the function to makeString instead of StringBuilder. >> Source/WebCore/css/CSSGradientValue.cpp:941 >> + result.append(", ", stop.m_color->cssText(), ')'); > > If you wanted to, you could collapse all the appends here doing, result.append("color-stop(", FormattedNumber::fixedPrecision(position), ", ", stop.m_color->cssText(), ')'). Yes, happy to do that. I think there’s quite a bit more to collapse elsewhere too; I was working on doing more and more of this and it got a little "out of hand" and so I stopped myself and wanted to land what I had. Done.
Darin Adler
Comment 19 2019-08-18 16:56:53 PDT
(In reply to Darin Adler from comment #17) > (In reply to Darin Adler from comment #16) > > Looks like the test failures indicate a failure of StringBuilder::append! > > Yes, I think I see a bug in StringBuilder::appendForAdapters when the string > is 8-bit, the thing to be appended is 16-bit, and the 8-bit buffer is big > enough to hold the new length. There's no call to allocateBufferUpConvert in > that case, yet there needs to be. Sadly, in the case where the test was failing the string doesn't even need to be 16-bit, but one of the substrings happens to be allocated as 16-bit even though all the characters in it are Latin-1.
Darin Adler
Comment 20 2019-08-18 17:20:09 PDT
Comment on attachment 376639 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376639&action=review > Source/WebCore/Modules/indexeddb/shared/IDBDatabaseInfo.cpp:166 > builder.append(objectStore.loggingString(1)); > builder.append('\n'); Should probably combine these two also.
Darin Adler
Comment 21 2019-08-20 09:05:32 PDT
Fixing the StringBuilder::appendForAdapters bug in bug 200921, and then will post a new patch for this bug so EWS can show us the tests succeeding.
Darin Adler
Comment 22 2019-08-21 19:05:48 PDT
Created attachment 376963 [details] Patch
Ryosuke Niwa
Comment 23 2019-08-21 21:03:10 PDT
Comment on attachment 376963 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376963&action=review > Source/JavaScriptCore/runtime/TypeSet.cpp:416 > + representation.append(StringView { field.get() }, ", "); Do we need to explicitly create StringView here? > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:114 > + This seems unrelated not that it matters either way. > Tools/WebKitTestRunner/TestController.cpp:1101 > + response = makeString(TestController::webProcessName(), ": ", webContentPID, '\n', > + TestController::networkProcessName(), ": ", networkingPID, '\n'); Should we just call dumpResponse instead? > Tools/WebKitTestRunner/TestController.cpp:1103 > - builder.append('\n'); > + response = "\n"_s; And here?
Darin Adler
Comment 24 2019-08-22 09:02:32 PDT
I can’t figure out what the Windows build failure is or if it was caused by this patch. I don’t know why most EWS results are missing. Maybe "New EWS" isn’t working at all at this time?
Darin Adler
Comment 25 2019-08-22 09:26:12 PDT
Committed r249013: <https://trac.webkit.org/changeset/249013>
Radar WebKit Bug Importer
Comment 26 2019-08-22 09:27:45 PDT
<rdar://problem/54601523>
Note You need to log in before you can comment on or make changes to this bug.