Use makeString and multi-argument StringBuilder::append instead of less efficient multiple appends
Created attachment 376614 [details] Patch
Comment on attachment 376614 [details] Patch Attachment 376614 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12933334 New failing tests: imported/w3c/web-platform-tests/css/css-shapes/parsing/shape-outside-valid.html css3/shapes/shape-outside/values/shape-outside-inset-008.html fast/shapes/parsing/parsing-shape-lengths.html imported/w3c/web-platform-tests/css/css-shapes/shape-outside/values/shape-outside-inset-001.html imported/w3c/web-platform-tests/hr-time/idlharness.html imported/w3c/web-platform-tests/html/webappapis/system-state-and-capabilities/the-navigator-object/content/005.xhtml css3/shapes/shape-outside/values/shape-outside-inset-001.html imported/w3c/web-platform-tests/html/webappapis/system-state-and-capabilities/the-navigator-object/protocol.html imported/w3c/web-platform-tests/css/css-shapes/shape-outside/values/shape-outside-inset-002.html imported/w3c/web-platform-tests/css/css-shapes/shape-outside/values/shape-outside-inset-003.html css3/shapes/shape-outside/values/shape-outside-computed-shape-001.html fast/shapes/parsing/parsing-shape-outside.html imported/w3c/web-platform-tests/css/css-shapes/shape-outside/values/shape-outside-inset-006.html css3/shapes/shape-outside/values/shape-outside-inset-003.html imported/w3c/web-platform-tests/css/css-shapes/shape-outside/values/shape-outside-inset-000.html css3/shapes/shape-outside/values/shape-outside-inset-009.html css3/shapes/shape-outside/values/shape-outside-inset-002.html css3/shapes/shape-outside/values/shape-outside-inset-000.html fast/masking/parsing-clip-path-shape.html imported/w3c/web-platform-tests/html/webappapis/system-state-and-capabilities/the-navigator-object/protocol/005.xhtml css3/shapes/shape-outside/values/shape-outside-inset-006.html css3/shapes/shape-outside/values/shape-outside-inset-004.html imported/w3c/web-platform-tests/css/css-shapes/shape-outside/values/shape-outside-inset-009.html imported/w3c/web-platform-tests/css/css-shapes/shape-outside/values/shape-outside-computed-shape-001.html imported/w3c/web-platform-tests/css/css-shapes/shape-outside/values/shape-outside-inset-008.html imported/w3c/web-platform-tests/css/css-shapes/shape-outside/values/shape-outside-inset-004.html
Created attachment 376618 [details] Archive of layout-test-results from ews101 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 376614 [details] Patch Attachment 376614 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12933368 New failing tests: imported/w3c/web-platform-tests/css/css-shapes/parsing/shape-outside-valid.html css3/shapes/shape-outside/values/shape-outside-inset-008.html fast/shapes/parsing/parsing-shape-lengths.html imported/w3c/web-platform-tests/css/css-shapes/shape-outside/values/shape-outside-inset-001.html dom/xhtml/level3/core/elementsetidattributenode10.xhtml imported/w3c/web-platform-tests/hr-time/idlharness.html imported/w3c/web-platform-tests/html/webappapis/system-state-and-capabilities/the-navigator-object/content/005.xhtml css3/shapes/shape-outside/values/shape-outside-inset-001.html imported/w3c/web-platform-tests/html/webappapis/system-state-and-capabilities/the-navigator-object/protocol.html imported/w3c/web-platform-tests/css/css-shapes/shape-outside/values/shape-outside-inset-002.html imported/w3c/web-platform-tests/css/css-shapes/shape-outside/values/shape-outside-inset-003.html css3/shapes/shape-outside/values/shape-outside-computed-shape-001.html fast/shapes/parsing/parsing-shape-outside.html imported/w3c/web-platform-tests/css/css-shapes/shape-outside/values/shape-outside-inset-006.html css3/shapes/shape-outside/values/shape-outside-inset-003.html imported/w3c/web-platform-tests/css/css-shapes/shape-outside/values/shape-outside-inset-000.html css3/shapes/shape-outside/values/shape-outside-inset-009.html css3/shapes/shape-outside/values/shape-outside-inset-002.html css3/shapes/shape-outside/values/shape-outside-inset-000.html fast/masking/parsing-clip-path-shape.html imported/w3c/web-platform-tests/html/webappapis/system-state-and-capabilities/the-navigator-object/protocol/005.xhtml css3/shapes/shape-outside/values/shape-outside-inset-006.html css3/shapes/shape-outside/values/shape-outside-inset-004.html imported/w3c/web-platform-tests/css/css-shapes/shape-outside/values/shape-outside-inset-009.html dom/xhtml/level3/core/elementsetidattribute11.xhtml imported/w3c/web-platform-tests/css/css-shapes/shape-outside/values/shape-outside-computed-shape-001.html imported/w3c/web-platform-tests/css/css-shapes/shape-outside/values/shape-outside-inset-008.html imported/w3c/web-platform-tests/css/css-shapes/shape-outside/values/shape-outside-inset-004.html
Created attachment 376620 [details] Archive of layout-test-results from ews115 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 376614 [details] Patch Attachment 376614 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12933455 New failing tests: css3/shapes/shape-outside/values/shape-outside-inset-009.html css3/shapes/shape-outside/values/shape-outside-inset-008.html css3/shapes/shape-outside/values/shape-outside-inset-002.html css3/shapes/shape-outside/values/shape-outside-inset-001.html css3/shapes/shape-outside/values/shape-outside-inset-006.html fast/shapes/parsing/parsing-shape-lengths.html css3/shapes/shape-outside/values/shape-outside-inset-000.html css3/shapes/shape-outside/values/shape-outside-inset-004.html css3/shapes/shape-outside/values/shape-outside-computed-shape-001.html fast/shapes/parsing/parsing-shape-outside.html fast/masking/parsing-clip-path-shape.html css3/shapes/shape-outside/values/shape-outside-inset-003.html
Created attachment 376622 [details] Archive of layout-test-results from ews215 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews215 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment on attachment 376614 [details] Patch Attachment 376614 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12933360 New failing tests: slowMicrobenchmarks.yaml/slowMicrobenchmarks/function-constructor-with-huge-strings.js.no-ftl slowMicrobenchmarks.yaml/slowMicrobenchmarks/function-constructor-with-huge-strings.js.default slowMicrobenchmarks.yaml/slowMicrobenchmarks/function-constructor-with-huge-strings.js.ftl-no-cjit-validate-sampling-profiler slowMicrobenchmarks.yaml/slowMicrobenchmarks/function-constructor-with-huge-strings.js.no-cjit
Created attachment 376639 [details] Patch
Comment on attachment 376639 [details] Patch Attachment 376639 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12936206 New failing tests: imported/w3c/web-platform-tests/html/webappapis/system-state-and-capabilities/the-navigator-object/protocol/005.xhtml imported/w3c/web-platform-tests/html/webappapis/system-state-and-capabilities/the-navigator-object/protocol.html imported/w3c/web-platform-tests/html/webappapis/system-state-and-capabilities/the-navigator-object/content/005.xhtml imported/w3c/web-platform-tests/hr-time/idlharness.html
Created attachment 376640 [details] Archive of layout-test-results from ews100 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 376639 [details] Patch Attachment 376639 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12936243 New failing tests: imported/w3c/web-platform-tests/hr-time/idlharness.html dom/xhtml/level3/core/elementsetidattributenode10.xhtml imported/w3c/web-platform-tests/html/webappapis/system-state-and-capabilities/the-navigator-object/content/005.xhtml imported/w3c/web-platform-tests/html/webappapis/system-state-and-capabilities/the-navigator-object/protocol.html dom/xhtml/level3/core/elementsetidattribute11.xhtml imported/w3c/web-platform-tests/html/webappapis/system-state-and-capabilities/the-navigator-object/protocol/005.xhtml
Created attachment 376641 [details] Archive of layout-test-results from ews115 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 376639 [details] Patch Attachment 376639 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12936223 New failing tests: slowMicrobenchmarks.yaml/slowMicrobenchmarks/function-constructor-with-huge-strings.js.no-ftl slowMicrobenchmarks.yaml/slowMicrobenchmarks/function-constructor-with-huge-strings.js.default slowMicrobenchmarks.yaml/slowMicrobenchmarks/function-constructor-with-huge-strings.js.ftl-no-cjit-validate-sampling-profiler slowMicrobenchmarks.yaml/slowMicrobenchmarks/function-constructor-with-huge-strings.js.no-cjit
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(), ')').
Looks like the test failures indicate a failure of StringBuilder::append!
(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.
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.
(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.
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.
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.
Created attachment 376963 [details] Patch
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?
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?
Committed r249013: <https://trac.webkit.org/changeset/249013>
<rdar://problem/54601523>