Closed Bug 1399533 Opened 5 years ago Closed 5 years ago

Remove duplicate checks in ForOfPIC::Chain


(Core :: JavaScript Engine, enhancement, P3)




Tracking Status
firefox57 --- wontfix
firefox58 --- fixed


(Reporter: anba, Assigned: anba)



(1 file)

While working on bug 1376572, I noticed that there are duplicate checks which can be removed.
Attached patch bug1399533.patchSplinter Review

- js::ForOfPIC::Chain::isArrayOptimized(...) is only called from js::ForOfPIC::Chain::tryOptimizeArray(...) and tryOptimizeArray(...) already ensures that isArrayStateStillSane() returns true, so we don't actually need to call isArrayStateStillSane() again in isArrayOptimized(...).

- The array's prototype was checked when a stub was found and when it wasn't found, we can simplify this to check the prototype once before searching for a matching stub.

- When no stub was found, and the number of stubs exceeded its limit, the stub chain was erased even when the new array will never be added to the stub chain, because it contains an own @@iterator property. It seems better to me to only erase the chain when the new array will definitely be added to the stub chain.

- ForOfPIC::Chain::getMatchingStub(...) returns the matched stub, but the returned stub was only used to test it against nullptr, so I renamed the method to hasMatchingStub(...) and changed the return type to |bool|.

- hasMatchingStub(...) contains dynamic checks to ensure that the chain is initialized and not disabled, but since both conditions are already ensured in tryOptimizeArray(...), I've changed the tests into an assertion.

- And I've changed the argument type in hasMatchingStub(...) from JSObject* to ArrayObject*, that way we can directly call ShapedObject::shape(), which saves us the type checks in JSObject::maybeShape().
Attachment #8907694 - Flags: review?(kvijayan)
Priority: -- → P3
Comment on attachment 8907694 [details] [diff] [review]

Review of attachment 8907694 [details] [diff] [review]:

Nice, looks good!
Attachment #8907694 - Flags: review?(kvijayan) → review+
Pushed by
Remove duplicate checks in ForOfPIC::Chain. r=djvj
Keywords: checkin-needed
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.