Bug 196055 - Cap length of an array with spread to MIN_ARRAY_STORAGE_CONSTRUCTION_LENGTH.
Summary: Cap length of an array with spread to MIN_ARRAY_STORAGE_CONSTRUCTION_LENGTH.
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
Keywords: InRadar
Depends on:
Reported: 2019-03-20 21:46 PDT by Mark Lam
Modified: 2019-03-21 00:52 PDT (History)
9 users (show)

See Also:

proposed patch. (12.07 KB, patch)
2019-03-20 22:19 PDT, Mark Lam
ysuzuki: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2019-03-20 21:46:32 PDT
We are doing this because:
1. We expect the array to be densely packed.
2. SpeculativeJIT::compileAllocateNewArrayWithSize() (and the FTL equivalent) expects the array length to be less than MIN_ARRAY_STORAGE_CONSTRUCTION_LENGTH if we don't want to use an ArrayStorage shape.
3. There's no reason why an array with spread needs to be that large anyway.  MIN_ARRAY_STORAGE_CONSTRUCTION_LENGTH is plenty.

Comment 1 Mark Lam 2019-03-20 22:19:08 PDT
Created attachment 365484 [details]
proposed patch.

Let's try this on the EWS first.
Comment 2 Mark Lam 2019-03-20 23:00:47 PDT
Comment on attachment 365484 [details]
proposed patch.

The JSC tests run to completion locally without any failures.  Let's get a review.
Comment 3 Yusuke Suzuki 2019-03-21 00:18:56 PDT
Comment on attachment 365484 [details]
proposed patch.

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


> Source/JavaScriptCore/dfg/DFGOperations.cpp:2727
> +    }

If some program hits this, we could

1. make `length >= MIN_ARRAY_STORAGE_CONSTRUCTION_LENGTH` OSR exit with Overflow (this is already done in this patch)
2. In operationNewArrayWithSpreadSlow, we return some information, and cause OSR exit with Overflow
3. In baseline / LLInt, we just allocate ArrayStorage JSArray
4. avoids emitting NewArrayWithSpread DFG nodes if hasExitSite(Overflow) = true in DFG

but I think throwing OOM error is OK until we find some real programs hit this condition.
Comment 4 Mark Lam 2019-03-21 00:52:13 PDT
Thanks for the review.  Landed in r243280: <http://trac.webkit.org/r243280>.