WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
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
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 333655
[details]
Patch
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
Created
attachment 333923
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug