Bug 194021 - Eliminate unnecessary String temporaries by using StringConcatenateNumbers
Summary: Eliminate unnecessary String temporaries by using StringConcatenateNumbers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-30 08:17 PST by Darin Adler
Modified: 2019-02-09 13:04 PST (History)
6 users (show)

See Also:

Attachments
Patch (73.78 KB, patch)
2019-01-30 08:37 PST, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (74.14 KB, patch)
2019-01-30 09:03 PST, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (74.25 KB, patch)
2019-01-30 09:05 PST, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (74.92 KB, patch)
2019-01-30 09:17 PST, Darin Adler 		no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (2.50 MB, application/zip)
2019-01-30 10:29 PST, EWS Watchlist 		no flags Details
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.62 MB, application/zip)
2019-01-30 10:29 PST, EWS Watchlist 		no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (525.16 KB, application/zip)
2019-01-30 10:47 PST, EWS Watchlist 		no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (2.15 MB, application/zip)
2019-01-30 11:08 PST, EWS Watchlist 		no flags Details
Patch (75.63 KB, patch)
2019-02-01 19:19 PST, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (75.88 KB, patch)
2019-02-01 19:42 PST, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (75.87 KB, patch)
2019-02-01 20:14 PST, Darin Adler 		no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (4.59 MB, application/zip)
2019-02-01 21:26 PST, EWS Watchlist 		no flags Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (4.73 MB, application/zip)
2019-02-01 21:34 PST, EWS Watchlist 		no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (4.19 MB, application/zip)
2019-02-01 22:07 PST, EWS Watchlist 		no flags Details
Archive of layout-test-results from ews206 for win-future (14.97 MB, application/zip)
2019-02-01 22:33 PST, EWS Watchlist 		no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (732.89 KB, application/zip)
2019-02-01 23:22 PST, EWS Watchlist 		no flags Details
Patch (83.51 KB, patch)
2019-02-03 09:35 PST, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (98.87 KB, patch)
2019-02-05 21:26 PST, Darin Adler 		no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.49 MB, application/zip)
2019-02-05 23:15 PST, EWS Watchlist 		no flags Details
Patch (98.90 KB, patch)
2019-02-08 18:55 PST, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (99.07 KB, patch)
2019-02-09 08:13 PST, Darin Adler 		ggaren: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.49 MB, application/zip)
2019-02-09 10:22 PST, EWS Watchlist 		no flags Details
Show Obsolete (20) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2019-01-30 08:17:41 PST 
Eliminate unnecessary String temporaries by using StringConcatenateNumbers
Comment 1 Darin Adler 2019-01-30 08:37:44 PST Comment hidden (obsolete)
Comment 2 Darin Adler 2019-01-30 09:03:07 PST Comment hidden (obsolete)
Comment 3 Darin Adler 2019-01-30 09:05:21 PST Comment hidden (obsolete)
Comment 4 Darin Adler 2019-01-30 09:17:10 PST Comment hidden (obsolete)
Comment 5 EWS Watchlist 2019-01-30 10:29:29 PST Comment hidden (obsolete)
Comment 6 EWS Watchlist 2019-01-30 10:29:31 PST Comment hidden (obsolete)
Comment 7 EWS Watchlist 2019-01-30 10:29:57 PST Comment hidden (obsolete)
Comment 8 EWS Watchlist 2019-01-30 10:29:59 PST Comment hidden (obsolete)
Comment 9 EWS Watchlist 2019-01-30 10:47:24 PST Comment hidden (obsolete)
Comment 10 EWS Watchlist 2019-01-30 10:47:25 PST Comment hidden (obsolete)
Comment 11 EWS Watchlist 2019-01-30 11:08:04 PST Comment hidden (obsolete)
Comment 12 EWS Watchlist 2019-01-30 11:08:06 PST Comment hidden (obsolete)
Comment 13 Darin Adler 2019-02-01 19:19:41 PST Comment hidden (obsolete)
Comment 14 Darin Adler 2019-02-01 19:42:04 PST Comment hidden (obsolete)
Comment 15 Darin Adler 2019-02-01 20:14:56 PST Comment hidden (obsolete)
Comment 16 EWS Watchlist 2019-02-01 21:26:42 PST Comment hidden (obsolete)
Comment 17 EWS Watchlist 2019-02-01 21:26:43 PST Comment hidden (obsolete)
Comment 18 EWS Watchlist 2019-02-01 21:34:18 PST Comment hidden (obsolete)
Comment 19 EWS Watchlist 2019-02-01 21:34:20 PST Comment hidden (obsolete)
Comment 20 EWS Watchlist 2019-02-01 22:07:08 PST Comment hidden (obsolete)
Comment 21 EWS Watchlist 2019-02-01 22:07:10 PST Comment hidden (obsolete)
Comment 22 EWS Watchlist 2019-02-01 22:33:28 PST Comment hidden (obsolete)
Comment 23 EWS Watchlist 2019-02-01 22:33:41 PST Comment hidden (obsolete)
Comment 24 EWS Watchlist 2019-02-01 23:22:12 PST Comment hidden (obsolete)
Comment 25 EWS Watchlist 2019-02-01 23:22:14 PST Comment hidden (obsolete)
Comment 26 Darin Adler 2019-02-03 09:35:13 PST Comment hidden (obsolete)
Comment 27 Darin Adler 2019-02-05 21:26:32 PST Comment hidden (obsolete)
Comment 28 EWS Watchlist 2019-02-05 23:15:49 PST Comment hidden (obsolete)
Comment 29 EWS Watchlist 2019-02-05 23:15:50 PST Comment hidden (obsolete)
Comment 30 Darin Adler 2019-02-08 18:55:12 PST Comment hidden (obsolete)
Comment 31 Darin Adler 2019-02-09 08:13:18 PST 
Created attachment 361606 [details]
Patch
Comment 32 Darin Adler 2019-02-09 10:10:07 PST 
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.
Comment 33 EWS Watchlist 2019-02-09 10:22:08 PST 
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
Comment 34 EWS Watchlist 2019-02-09 10:22:10 PST 
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
Comment 35 Darin Adler 2019-02-09 10:34:51 PST 
I can’t see any way these failures would be due to the patch.
Comment 36 Darin Adler 2019-02-09 10:35:40 PST 
The failure is a crash inside WebCore::PixelBufferConformerCV::convert.
Comment 37 Darin Adler 2019-02-09 12:05:41 PST 
I’d love to land this now. Looking for a reviewer.
Comment 38 Geoffrey Garen 2019-02-09 12:52:43 PST 
Comment on attachment 361606 [details]
Patch

r=me
Comment 39 Darin Adler 2019-02-09 13:01:41 PST 
https://trac.webkit.org/changeset/241244
Comment 40 Radar WebKit Bug Importer 2019-02-09 13:04:41 PST 
<rdar://problem/47943924>