Bug 193912 – [JSC] Repeat string created from Array.prototype.join() take too much memory

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Guillaume Emont]

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);

[Guillaume Emont]

(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]

Seems reasonable to make this better.

[Guillaume Emont]

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]

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]

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 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]

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]

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 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]

(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]

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]

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]

Ping reviewer.

[Saam Barati]

Comment on attachment 361705 [details]
Patch

r=me

[Saam Barati]

You should also add a microbenchmark

[Guillaume Emont]

Created attachment 362913 [details]
Patch

Patch for landing.

[WebKit Commit Bot]

Comment on attachment 362913 [details]
Patch

Clearing flags on attachment: 362913

Committed r242081: <https://trac.webkit.org/changeset/242081>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48394256>