RESOLVED FIXED 193912
[JSC] Repeat string created from Array.prototype.join() take too much memory
https://bugs.webkit.org/show_bug.cgi?id=193912
Summary [JSC] Repeat string created from Array.prototype.join() take too much memory
Guillaume Emont
Reported 2019-01-28 09:49:34 PST
The following code takes over 600M of rss on my linux x86_64 desktop: var a = new Array(10 * 1024 * 1024).join('M'); and takes over 2s to run (on a dual Xeon E5-2630). I would expect it to take much less time and use an order of magnitude less memory. As a comparison, the following code runs in ~27ms: var a = 'M'.repeat(10 * 1024 * 1024);
Attachments
Screenshot form massif-visualizer showing where memory was allocated (801.64 KB, image/png)
2019-01-31 13:04 PST, Guillaume Emont
no flags
Patch (4.47 KB, patch)
2019-02-04 17:04 PST, Guillaume Emont
no flags
Archive of layout-test-results from ews121 for ios-simulator-wk2 (9.41 MB, application/zip)
2019-02-04 21:38 PST, EWS Watchlist
no flags
Patch (4.47 KB, patch)
2019-02-05 13:07 PST, Guillaume Emont
no flags
Patch (6.42 KB, patch)
2019-02-11 13:03 PST, Guillaume Emont
no flags
Patch (7.05 KB, patch)
2019-02-25 11:30 PST, Guillaume Emont
no flags
Guillaume Emont
Comment 1 2019-01-28 09:52:54 PST
(In reply to Guillaume Emont from comment #0) > The following code takes over 600M of rss on my linux x86_64 desktop: > var a = new Array(10 * 1024 * 1024).join('M'); The rss measure was taken on the jsc standalone interpreter.
Saam Barati
Comment 2 2019-01-31 12:32:12 PST
Seems reasonable to make this better.
Guillaume Emont
Comment 3 2019-01-31 13:00:41 PST
The first idea I'm currently exploring is to change fastJoin() in ArrayPrototype.cpp so that if the array is uninitialized (I think that's equivalent to indexingType() == ArrayType), we call String.prototype.repeat(). That's a relatively quick fix that should help in this specific test case, though I'm still curious to understand why a string of 10M chars seems to take 485MB of memory (or at least we reserve that much, according to massif). That's an average of 48.5 bytes per char, I think we could do better, even if it was a totally random string.
Guillaume Emont
Comment 4 2019-01-31 13:04:46 PST
Created attachment 360761 [details] Screenshot form massif-visualizer showing where memory was allocated In the attached screenshot, we see that about 485M is allocated by JSRopeString::create() and about 80M by JSArray::tryCreate().
Guillaume Emont
Comment 5 2019-02-04 17:04:28 PST
Created attachment 361132 [details] Patch This seems to fix the time and memory consumption of things like \'var a = new Array(10 * 1024 * 1024).join('M');\' by adding a fast case for uninitialize arrays.
EWS Watchlist
Comment 6 2019-02-04 21:37:58 PST
Comment on attachment 361132 [details] Patch Attachment 361132 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11034977 New failing tests: editing/pasteboard/smart-paste-007.html editing/pasteboard/smart-paste-008.html
EWS Watchlist
Comment 7 2019-02-04 21:38:00 PST
Created attachment 361157 [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
Guillaume Emont
Comment 8 2019-02-05 13:07:25 PST
Created attachment 361213 [details] Patch Submitting same patch again to see if ios-sim-ews error was a flakey couple of tests
Saam Barati
Comment 9 2019-02-10 20:18:18 PST
Comment on attachment 361213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361213&action=review Mostly LGTM, but some comments. > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:510 > + RELEASE_AND_RETURN(scope, jsEmptyString(&state)); Is this correct even if length >= 2? Wouldn't we still access those properties? Don't we then need to check if holesMustForwardToPrototype? If that's the correct behavior, please add a test where this would've had the wrong behavior. > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:513 > + if (length <= 1) > + RELEASE_AND_RETURN(scope, jsEmptyString(&state)); Why can this go before the holesMustForwardToPrototypeCheck? Do we not access any properties if length is 1? > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:518 > + else nit: no else needed here since previous if ends in a return
Guillaume Emont
Comment 10 2019-02-11 13:03:36 PST
(In reply to Saam Barati from comment #9) > Comment on attachment 361213 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=361213&action=review > > Mostly LGTM, but some comments. > > > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:510 > > + RELEASE_AND_RETURN(scope, jsEmptyString(&state)); > > Is this correct even if length >= 2? Wouldn't we still access those > properties? Don't we then need to check if holesMustForwardToPrototype? If > that's the correct behavior, please add a test where this would've had the > wrong behavior. Indeed, this is not correct, even with length==1, this was an oversight on my part. > > > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:513 > > + if (length <= 1) > > + RELEASE_AND_RETURN(scope, jsEmptyString(&state)); > > Why can this go before the holesMustForwardToPrototypeCheck? Do we not > access any properties if length is 1? Same as above. > > > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:518 > > + else > > nit: no else needed here since previous if ends in a return
Guillaume Emont
Comment 11 2019-02-11 13:03:57 PST
Created attachment 361705 [details] Patch New version that addresses the issues raised by Saam. I also added a test, since I did not see that issue when running tests.
Guillaume Emont
Comment 12 2019-02-12 09:57:30 PST
WPE error looks like the EWS was temporarily running out of RAM, I don't think we should be too worried about that.
Guillaume Emont
Comment 13 2019-02-14 02:02:14 PST
Ping reviewer.
Saam Barati
Comment 14 2019-02-25 00:13:09 PST
Comment on attachment 361705 [details] Patch r=me
Saam Barati
Comment 15 2019-02-25 00:14:19 PST
You should also add a microbenchmark
Guillaume Emont
Comment 16 2019-02-25 11:30:10 PST
Created attachment 362913 [details] Patch Patch for landing.
WebKit Commit Bot
Comment 17 2019-02-26 00:50:54 PST
Comment on attachment 362913 [details] Patch Clearing flags on attachment: 362913 Committed r242081: <https://trac.webkit.org/changeset/242081>
WebKit Commit Bot
Comment 18 2019-02-26 00:50:56 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 19 2019-02-26 01:00:59 PST
Note You need to log in before you can comment on or make changes to this bug.