REOPENED 174516
Butterfly storage need not be initialized for indexing type Undecided.
https://bugs.webkit.org/show_bug.cgi?id=174516
Summary Butterfly storage need not be initialized for indexing type Undecided.
Mark Lam
Reported 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.
Attachments
proposed patch. (3.86 KB, patch)
2017-07-14 11:29 PDT, Mark Lam
saam: review+
buildbot: commit-queue-
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
patch for landing. (3.88 KB, patch)
2017-07-18 12:26 PDT, Mark Lam
no flags
Patch (8.67 KB, patch)
2018-02-12 18:34 PST, Robin Morisset
no flags
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
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
New patch, includes some refactoring (21.52 KB, patch)
2018-02-14 11:53 PST, Robin Morisset
no flags
Patch (23.62 KB, patch)
2018-02-15 11:26 PST, Robin Morisset
saam: review-
Mark Lam
Comment 1 2017-07-14 11:29:31 PDT
Created attachment 315464 [details] proposed patch.
Saam Barati
Comment 2 2017-07-14 11:34:44 PDT
r=me
Build Bot
Comment 3 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
Build Bot
Comment 4 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
Mark Lam
Comment 5 2017-07-18 12:26:52 PDT
Created attachment 315816 [details] patch for landing. Thanks for the review.
WebKit Commit Bot
Comment 6 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>
WebKit Commit Bot
Comment 7 2017-07-18 15:41:04 PDT
All reviewed patches have been landed. Closing bug.
Robin Morisset
Comment 8 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.
Robin Morisset
Comment 9 2018-02-12 18:34:42 PST
Robin Morisset
Comment 10 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?
Robin Morisset
Comment 11 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
Robin Morisset
Comment 12 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)
EWS Watchlist
Comment 13 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
Robin Morisset
Comment 14 2018-02-14 11:53:02 PST
Created attachment 333826 [details] New patch, includes some refactoring
EWS Watchlist
Comment 15 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
Robin Morisset
Comment 16 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.
Robin Morisset
Comment 17 2018-02-14 16:49:26 PST
I have confirmed that neither JSArray::appendMemcpy nor arrayProtoPrivateFuncConcatMemcpy are called by this test.
Robin Morisset
Comment 18 2018-02-15 11:26:15 PST
Saam Barati
Comment 19 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.
Note You need to log in before you can comment on or make changes to this bug.