Bug 174516 - Butterfly storage need not be initialized for indexing type Undecided.
Summary: Butterfly storage need not be initialized for indexing type Undecided.
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robin Morisset
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-07-14 11:17 PDT by Mark Lam
Modified: 2018-02-20 10:47 PST (History)
10 users (show)

See Also:


Attachments
proposed patch. (3.86 KB, patch)
2017-07-14 11:29 PDT, Mark Lam
saam: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (993.17 KB, application/zip)
2017-07-14 18:16 PDT, Build Bot
no flags Details
patch for landing. (3.88 KB, patch)
2017-07-18 12:26 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
Patch (8.67 KB, patch)
2018-02-12 18:34 PST, Robin Morisset
no flags Details | Formatted Diff | Diff
New patch, includes some refactoring, waiting for EWS to test it before asking for review (20.93 KB, patch)
2018-02-14 10:22 PST, Robin Morisset
no flags Details | Formatted Diff | Diff
New patch, includes some refactoring, waiting for EWS to test it before asking for review (fixed test) (20.93 KB, patch)
2018-02-14 10:26 PST, Robin Morisset
no flags Details | Formatted Diff | Diff
New patch, includes some refactoring (21.52 KB, patch)
2018-02-14 11:53 PST, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (23.62 KB, patch)
2018-02-15 11:26 PST, Robin Morisset
saam: 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 2017-07-14 11:17:13 PDT
While it's not incorrect to initialize the butterfly storage when the indexingType is Undecided, it is inefficient as we'll end up initializing it again later when we convert the storage to a different indexingType.  Some of our code already skips initializing Undecided butterflies.  This patch makes it the consistent behavior everywhere.
Comment 1 Mark Lam 2017-07-14 11:29:31 PDT
Created attachment 315464 [details]
proposed patch.
Comment 2 Saam Barati 2017-07-14 11:34:44 PDT
r=me
Comment 3 Build Bot 2017-07-14 18:16:06 PDT
Comment on attachment 315464 [details]
proposed patch.

Attachment 315464 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4122591

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html
http/tests/canvas/canvas-tainted-after-draw-image.html
Comment 4 Build Bot 2017-07-14 18:16:07 PDT
Created attachment 315517 [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.12.5
Comment 5 Mark Lam 2017-07-18 12:26:52 PDT
Created attachment 315816 [details]
patch for landing.

Thanks for the review.
Comment 6 WebKit Commit Bot 2017-07-18 15:41:03 PDT
Comment on attachment 315816 [details]
patch for landing.

Clearing flags on attachment: 315816

Committed r219636: <http://trac.webkit.org/changeset/219636>
Comment 7 WebKit Commit Bot 2017-07-18 15:41:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Robin Morisset 2018-01-23 11:11:59 PST
I have to rollout this patch due to a bug, so I reopen the bug to remember to recommit a fixed version later on.
Comment 9 Robin Morisset 2018-02-12 18:34:42 PST
Created attachment 333655 [details]
Patch
Comment 10 Robin Morisset 2018-02-12 18:40:16 PST
(In reply to Robin Morisset from comment #9)
> Created attachment 333655 [details]
> Patch

This patch fixes the immediate problem, but I am not entirely happy with it. I see two problems:
- It is a bit repetitive, with the two cases (ArrayWithDouble, and !ArrayWithUndecided) only differing by a few characters. I have not managed to simplify it because of typing  issues.
- It only fixes the fast path, the slow path (a dozen lines above, using moveElements) appears to have exactly the same problem. Sadly, it is not as easily testable, because it uses constructEmptyArray instead of tryCreateUninitializedRestricted, and as a result is not monitored by ObjectInitializationScope.

Should I try fixing the slow path even though it currently already pass the test, should I force it to use ObjectInitializationScope somehow, or should I just ignore it?
Comment 11 Robin Morisset 2018-02-14 10:22:16 PST
Created attachment 333811 [details]
New patch, includes some refactoring, waiting for EWS to test it before asking for review
Comment 12 Robin Morisset 2018-02-14 10:26:13 PST
Created attachment 333812 [details]
New patch, includes some refactoring, waiting for EWS to test it before asking for review (fixed test)
Comment 13 EWS Watchlist 2018-02-14 11:44:52 PST
Comment on attachment 333812 [details]
New patch, includes some refactoring, waiting for EWS to test it before asking for review (fixed test)

Attachment 333812 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/6502895

New failing tests:
ChakraCore.yaml/ChakraCore/test/Array/protoLookup.js.default
Comment 14 Robin Morisset 2018-02-14 11:53:02 PST
Created attachment 333826 [details]
New patch, includes some refactoring
Comment 15 EWS Watchlist 2018-02-14 15:33:03 PST
Comment on attachment 333826 [details]
New patch, includes some refactoring

Attachment 333826 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/6506695

New failing tests:
stress/ftl-put-by-id-setter-exception-interesting-live-state.js.dfg-eager-no-cjit-validate
Comment 16 Robin Morisset 2018-02-14 16:37:04 PST
Comment on attachment 333826 [details]
New patch, includes some refactoring

I cannot reproduce this test failing. I suspect it might be flaky, especially considering this test does not seem to be calling concat. I would like a review while I check in more depth what could have made the bot fail.
Comment 17 Robin Morisset 2018-02-14 16:49:26 PST
I have confirmed that neither JSArray::appendMemcpy nor arrayProtoPrivateFuncConcatMemcpy are called by this test.
Comment 18 Robin Morisset 2018-02-15 11:26:15 PST
Created attachment 333923 [details]
Patch
Comment 19 Saam Barati 2018-02-20 10:47:45 PST
Comment on attachment 333923 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:19
> +        That patch had revealed an underlying bug: arrayProtoPrivateFunConcatMemcpy forgot to initialize
> +        part of its result when given an array with undecided and an array with doubles.
> +        As a result it was rolled-out in r227435
> +

Is there anywhere else that might depend on this behavior?

> Source/JavaScriptCore/runtime/ArrayConventions.h:154
> +    else if (type == ArrayWithInt32 || type == ArrayWithContiguous)

What if these aren't arrays? Or are we guaranteed type here has the IsArray bit set? If so, it's worth an assert.

e.g, do we see Int32Shape/ContigousShape here?

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1206
> +    } else if (UNLIKELY(source->indexingType() == ArrayWithUndecided)) {

Do we not care about undecided shape? Or only ArrayWithUndecided?

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1213
> +            JSValue value = source->tryGetIndexQuickly(i);

This changes the code to no longer test if the result is empty.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1221
> +static bool moveElements(ExecState* exec, VM& vm, JSArray* target, unsigned targetOffset, JSArray* source)

Maybe we should make this not return a boolean. Either it throws or it succeeds.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1267
> +    if (UNLIKELY(!success))
> +        return encodedJSValue();

It would be wrong to just return JSValue() to JS code. A javascript call should never return JSValue(). However, this code is not wrong, since moveElements *only* returns false if an exception happens. Let's just turn this into an assert then. I'm not sure why we had this check before given the behavior.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1320
> +    if (type == NonArray) {

I don't get how this is correct. mergeIndexingTypeForCopying returns NonArray for ArrayStorage.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1340
> +    if (UNLIKELY(!success))
> +        return encodedJSValue();

ditto here. This branch is never taken.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1344
> +    if (UNLIKELY(!success))
>          return encodedJSValue();

ditto here, and you don't reassign to success, so it would've done nothing regardless.