Bug 174516 – Butterfly storage need not be initialized for indexing type Undecided.

• Component: JavaScriptCore
• Status: REOPENED

[Mark Lam]

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.

[Mark Lam]

Created attachment 315464 [details]
proposed patch.

[Saam Barati]

r=me

[Build Bot]

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

[Build Bot]

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

[Mark Lam]

Created attachment 315816 [details]
patch for landing.

Thanks for the review.

[WebKit Commit Bot]

Comment on attachment 315816 [details]
patch for landing.

Clearing flags on attachment: 315816

Committed r219636: <http://trac.webkit.org/changeset/219636>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Robin Morisset]

I have to rollout this patch due to a bug, so I reopen the bug to remember to recommit a fixed version later on.

[Robin Morisset]

Created attachment 333655 [details]
Patch

[Robin Morisset]

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

[Robin Morisset]

Created attachment 333811 [details]
New patch, includes some refactoring, waiting for EWS to test it before asking for review

[Robin Morisset]

Created attachment 333812 [details]
New patch, includes some refactoring, waiting for EWS to test it before asking for review (fixed test)

[EWS Watchlist]

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

[Robin Morisset]

Created attachment 333826 [details]
New patch, includes some refactoring

[EWS Watchlist]

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

[Robin Morisset]

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.

[Robin Morisset]

I have confirmed that neither JSArray::appendMemcpy nor arrayProtoPrivateFuncConcatMemcpy are called by this test.

[Robin Morisset]

Created attachment 333923 [details]
Patch

[Saam Barati]

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.