Bug 194021 – Eliminate unnecessary String temporaries by using StringConcatenateNumbers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Darin Adler]

Eliminate unnecessary String temporaries by using StringConcatenateNumbers

[Darin Adler]

Created attachment 360581 [details]
Patch

[Darin Adler]

Created attachment 360584 [details]
Patch

[Darin Adler]

Created attachment 360585 [details]
Patch

[Darin Adler]

Created attachment 360586 [details]
Patch

[EWS Watchlist]

Comment on attachment 360586 [details]
Patch

Attachment 360586 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10954224

New failing tests:
svg/dom/SVGAnimatedEnumeration-SVGMarkerElement.html
svg/dom/length-list-parser.html
fast/canvas/webgl/copy-tex-image-and-sub-image-2d-bad-input.html
svg/dom/SVGAngle.html
svg/dom/SVGLength-px-with-context.html
fast/css/aspect-ratio-parsing-tests.html
svg/dom/SVGLength-px.html

[EWS Watchlist]

Created attachment 360591 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360586 [details]
Patch

Attachment 360586 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10954202

New failing tests:
svg/dom/SVGAnimatedEnumeration-SVGMarkerElement.html
svg/dom/length-list-parser.html
fast/canvas/webgl/copy-tex-image-and-sub-image-2d-bad-input.html
svg/dom/SVGAngle.html
svg/dom/SVGLength-px-with-context.html
fast/css/aspect-ratio-parsing-tests.html
svg/dom/SVGLength-px.html

[EWS Watchlist]

Created attachment 360592 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360586 [details]
Patch

Attachment 360586 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10954223

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360595 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360586 [details]
Patch

Attachment 360586 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10954298

New failing tests:
svg/dom/SVGAnimatedEnumeration-SVGMarkerElement.html
svg/dom/length-list-parser.html
fast/canvas/webgl/copy-tex-image-and-sub-image-2d-bad-input.html
svg/dom/SVGAngle.html
svg/dom/SVGLength-px-with-context.html
fast/css/aspect-ratio-parsing-tests.html
svg/dom/SVGLength-px.html

[EWS Watchlist]

Created attachment 360599 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Darin Adler]

Created attachment 360949 [details]
Patch

[Darin Adler]

Created attachment 360951 [details]
Patch

[Darin Adler]

Created attachment 360953 [details]
Patch

[EWS Watchlist]

Comment on attachment 360953 [details]
Patch

Attachment 360953 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11003644

New failing tests:
svg/dynamic-updates/SVGMaskElement-svgdom-width-prop.html
fast/css/aspect-ratio-inheritance.html
svg/dynamic-updates/SVGRadialGradientElement-svgdom-fy-prop.html
svg/dynamic-updates/SVGTextElement-svgdom-textLength-prop.html
svg/dom/SVGAnimatedEnumeration-SVGMarkerElement.html
svg/dynamic-updates/SVGLinearGradientElement-svgdom-x1-prop.html
svg/dynamic-updates/SVGRadialGradientElement-svgdom-cx-prop.html
svg/dynamic-updates/SVGMaskElement-svgdom-y-prop.html
svg/dynamic-updates/SVGLinearGradientElement-svgdom-x2-prop.html
svg/dom/SVGLength-px.html
svg/custom/svg-xml-dom-sync.html
svg/dom/SVGAngle.html
svg/dynamic-updates/SVGRadialGradientElement-svgdom-fx-prop.html
svg/dom/valueAsString-null.html
svg/dom/SVGLength-px-with-context.html
svg/dynamic-updates/SVGMaskElement-svgdom-x-prop.html
fast/css/aspect-ratio-parsing-tests.html
svg/dynamic-updates/SVGRadialGradientElement-svgdom-cy-prop.html
svg/dynamic-updates/SVGLinearGradientElement-svgdom-y2-prop.html
svg/dynamic-updates/SVGMaskElement-svgdom-height-prop.html
svg/dom/length-list-parser.html
svg/custom/invalid-length-units.html
svg/dom/SVGLength.html
svg/dynamic-updates/SVGLinearGradientElement-svgdom-y1-prop.html
svg/dom/SVGLengthList-basics.xhtml
svg/dynamic-updates/SVGRadialGradientElement-svgdom-r-prop.html

[EWS Watchlist]

Created attachment 360959 [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

[EWS Watchlist]

Comment on attachment 360953 [details]
Patch

Attachment 360953 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11003696

New failing tests:
svg/dynamic-updates/SVGMaskElement-svgdom-width-prop.html
fast/css/aspect-ratio-inheritance.html
svg/dynamic-updates/SVGRadialGradientElement-svgdom-fy-prop.html
svg/dynamic-updates/SVGTextElement-svgdom-textLength-prop.html
svg/dom/SVGAnimatedEnumeration-SVGMarkerElement.html
svg/dynamic-updates/SVGLinearGradientElement-svgdom-x1-prop.html
svg/dynamic-updates/SVGRadialGradientElement-svgdom-cx-prop.html
svg/dynamic-updates/SVGMaskElement-svgdom-y-prop.html
svg/dynamic-updates/SVGLinearGradientElement-svgdom-x2-prop.html
svg/dom/SVGLength-px.html
svg/custom/svg-xml-dom-sync.html
svg/dom/SVGAngle.html
svg/dynamic-updates/SVGRadialGradientElement-svgdom-fx-prop.html
svg/dom/valueAsString-null.html
svg/dom/SVGLength-px-with-context.html
svg/dynamic-updates/SVGMaskElement-svgdom-x-prop.html
fast/css/aspect-ratio-parsing-tests.html
svg/dynamic-updates/SVGRadialGradientElement-svgdom-cy-prop.html
svg/dynamic-updates/SVGLinearGradientElement-svgdom-y2-prop.html
svg/dynamic-updates/SVGMaskElement-svgdom-height-prop.html
svg/dom/length-list-parser.html
svg/custom/invalid-length-units.html
svg/dom/SVGLength.html
svg/dynamic-updates/SVGLinearGradientElement-svgdom-y1-prop.html
svg/dom/SVGLengthList-basics.xhtml
svg/dynamic-updates/SVGRadialGradientElement-svgdom-r-prop.html

[EWS Watchlist]

Created attachment 360960 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360953 [details]
Patch

Attachment 360953 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11003717

New failing tests:
svg/dynamic-updates/SVGMaskElement-svgdom-width-prop.html
fast/css/aspect-ratio-inheritance.html
svg/dynamic-updates/SVGRadialGradientElement-svgdom-fy-prop.html
svg/dynamic-updates/SVGTextElement-svgdom-textLength-prop.html
svg/dom/SVGAnimatedEnumeration-SVGMarkerElement.html
svg/dynamic-updates/SVGLinearGradientElement-svgdom-x1-prop.html
svg/dynamic-updates/SVGRadialGradientElement-svgdom-cx-prop.html
svg/dynamic-updates/SVGMaskElement-svgdom-y-prop.html
svg/dynamic-updates/SVGLinearGradientElement-svgdom-x2-prop.html
svg/dom/SVGLength-px.html
svg/custom/svg-xml-dom-sync.html
svg/dom/SVGAngle.html
svg/dynamic-updates/SVGRadialGradientElement-svgdom-fx-prop.html
svg/dom/valueAsString-null.html
svg/dom/SVGLength-px-with-context.html
svg/dynamic-updates/SVGMaskElement-svgdom-x-prop.html
fast/css/aspect-ratio-parsing-tests.html
svg/dynamic-updates/SVGRadialGradientElement-svgdom-cy-prop.html
svg/dynamic-updates/SVGLinearGradientElement-svgdom-y2-prop.html
svg/dynamic-updates/SVGMaskElement-svgdom-height-prop.html
svg/dom/length-list-parser.html
svg/custom/invalid-length-units.html
svg/dom/SVGLength.html
svg/dynamic-updates/SVGLinearGradientElement-svgdom-y1-prop.html
svg/dom/SVGLengthList-basics.xhtml
svg/dynamic-updates/SVGRadialGradientElement-svgdom-r-prop.html

[EWS Watchlist]

Created attachment 360962 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360953 [details]
Patch

Attachment 360953 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11003899

New failing tests:
svg/dynamic-updates/SVGMaskElement-svgdom-width-prop.html
fast/css/aspect-ratio-inheritance.html
svg/dynamic-updates/SVGRadialGradientElement-svgdom-fy-prop.html
svg/custom/svg-xml-dom-sync.html
svg/dom/SVGAnimatedEnumeration-SVGMarkerElement.html
svg/dynamic-updates/SVGLinearGradientElement-svgdom-x1-prop.html
svg/dynamic-updates/SVGRadialGradientElement-svgdom-cx-prop.html
svg/dynamic-updates/SVGMaskElement-svgdom-y-prop.html
svg/dynamic-updates/SVGLinearGradientElement-svgdom-x2-prop.html
svg/dom/SVGLength-px.html
svg/dynamic-updates/SVGTextElement-svgdom-textLength-prop.html
svg/dom/SVGAngle.html
svg/dynamic-updates/SVGRadialGradientElement-svgdom-fx-prop.html
svg/dom/valueAsString-null.html
svg/dom/SVGLength-px-with-context.html
svg/dynamic-updates/SVGMaskElement-svgdom-x-prop.html
fast/css/aspect-ratio-parsing-tests.html
svg/dynamic-updates/SVGRadialGradientElement-svgdom-cy-prop.html
svg/dynamic-updates/SVGLinearGradientElement-svgdom-y2-prop.html
svg/dynamic-updates/SVGMaskElement-svgdom-height-prop.html
svg/dom/length-list-parser.html
svg/custom/invalid-length-units.html
svg/dom/SVGLength.html
svg/dynamic-updates/SVGLinearGradientElement-svgdom-y1-prop.html
svg/dom/SVGLengthList-basics.xhtml
svg/dynamic-updates/SVGRadialGradientElement-svgdom-r-prop.html

[EWS Watchlist]

Created attachment 360965 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 360953 [details]
Patch

Attachment 360953 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11004350

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360966 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Darin Adler]

Created attachment 361015 [details]
Patch

[Darin Adler]

Created attachment 361277 [details]
Patch

[EWS Watchlist]

Comment on attachment 361277 [details]
Patch

Attachment 361277 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11049327

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 361279 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Darin Adler]

Created attachment 361579 [details]
Patch

[Darin Adler]

Created attachment 361606 [details]
Patch

[Darin Adler]

Comment on attachment 361606 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361606&action=review

I think this is ready for review now. Basically all tests passing everything compiling on all platforms.

One small nit right at the moment: Not sure why there are tests failing on ios-sim buildbot (still running, so maybe it's a glitch); did not see those tests fail with the almost identical previous version of the same patch (was all green).

I did not touch the serialization done by the function named makeString in Wasm nor call sites like JSC::BuiltinNames::getPublicName; there are some places in both categories that allocate unnecessary String temporaries and that could be using StringView or StringConcatenateNumbers instead, but things got a little too complicated for me to understand how to make safe fixes there without deeper thinking and more care. I think we should probably rename the makeString function in WasmOps.h to avoid confusion with the makeString in WTF since they are different and it’s easy to be confused about which one a given call site is calling.

Note however, that sometimes what looks like an unnecessary String temporary is just reference counting churn, making a String since makeString doesn’t work with StringImpl directly. That’s much less expensive than allocating/destroying a StringImpl, which is the bigger thing we are fixing in most cases here in this patch.

The only significant challenge in getting this patch correct was that for floating point numbers, the makeString default is ECMAScript-style shortest form, and the String::number and StringBuilder::appendNumber default is 6 digits, fixed precision, strip trailing zeroes. For many cases, the two approaches are basically equivalent and it’s nice to move to the shortest form as a more elegant solution to the same problem, but a big exception is when it’s a float rather than a double, because we don’t currently have a float shortest form implementation. So for those cases, I had to be careful to preserve the use of fixed precision. Would be nice to fix that eventually so we can reduce the unnecessary use of fixed precision, since shortest is almost always better. In fact, if we fixed that I would also want to change the String::number and StringBuilder::appendNumber defaults as well. So, for now, any place where String::number is replaced by the use of makeString, if it’s an integer or if it’s a double and shortest form is equivalent/OK we can do it in the simplest form, but if it’s a float then we almost always have to use FormattedNumber::fixedPrecision to preserve behavior for now.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:468
> -    NumberToStringBuffer buffer;
> -    return JSValue::encode(jsString(exec, String(numberToFixedWidthString(x, decimalPlaces, buffer))));
> +    return JSValue::encode(jsString(exec, String::numberToStringFixedWidth(x, decimalPlaces)));

This does add one function call: I’ll need to double check to be sure this doesn’t slow things down, but it’s unlikely.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:504
> -    NumberToStringBuffer buffer;
> -    return JSValue::encode(jsString(exec, String(numberToFixedPrecisionString(x, significantFigures, buffer))));
> +    return JSValue::encode(jsString(exec, String::number(x, significantFigures, KeepTrailingZeros)));

Ditto.

[EWS Watchlist]

Comment on attachment 361606 [details]
Patch

Attachment 361606 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11092013

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall-no-ssrcs.https.html

[EWS Watchlist]

Created attachment 361609 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Darin Adler]

I can’t see any way these failures would be due to the patch.

[Darin Adler]

The failure is a crash inside WebCore::PixelBufferConformerCV::convert.

[Darin Adler]

I’d love to land this now. Looking for a reviewer.

[Geoffrey Garen]

Comment on attachment 361606 [details]
Patch

r=me

[Darin Adler]

https://trac.webkit.org/changeset/241244

[Radar WebKit Bug Importer]

<rdar://problem/47943924>