Closed Bug 1461339 Opened 5 years ago Closed 4 years ago

Don't eagerly sparsify dense elements in NativeDefineProperty

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: anba, Assigned: anba)

Details

Attachments

(1 file)

When a dense element is redefined to a non-writable property in NativeDefineProperty (and the property descriptor has no value field!), the element is first changed to a sparse property and then AddOrChangeProperty is called which updates the property attributes. Similar code is present when redefining a dense element to an accessor property [2].

I don't think it's necessary to sparsify the dense element here, given that, for example, we don't do it when the redefining a dense element to a non-configurable property.


[1] https://searchfox.org/mozilla-central/rev/a85db9e29eb3f022dbaf8b9a6390ecbacf51e7dd/js/src/vm/NativeObject.cpp#1807-1812
[2] https://searchfox.org/mozilla-central/rev/a85db9e29eb3f022dbaf8b9a6390ecbacf51e7dd/js/src/vm/NativeObject.cpp#1789-1794
That means we can remove sparsifyDenseElement? Cool!
Attached patch bug1461339.patchSplinter Review
(In reply to Jan de Mooij [:jandem] from comment #1)
> That means we can remove sparsifyDenseElement? Cool!

Exactly! :-)


In addition to the sparsifyDenseElement removal, the patch:
- Changes |NativeObject::removeDenseElementForSparseIndex()| to a non-static method to better match its sibling methods in NativeObject.
- Adds an assertion to |removeDenseElementForSparseIndex()| to ensure a sparse property is actually present. (The assertion is not needed for correctness, it is only intended to help to understand the code.)
- And changes |removeDenseElementForSparseIndex()| to call |setDenseElement()| instead of |setDenseElementUnchecked()|, because |removeDenseElementForSparseIndex()| is now only called in situations where the elements are definitely not frozen. 
- |setDenseElementUnchecked()| is now only called in ||setDenseElement()|, so let's inline it and then remove |setDenseElementUnchecked()|. And do the same for |setDenseInitializedLengthUnchecked()|.
Attachment #8975492 - Flags: review?(jdemooij)
Comment on attachment 8975492 [details] [diff] [review]
bug1461339.patch

Review of attachment 8975492 [details] [diff] [review]:
-----------------------------------------------------------------

\o/
Attachment #8975492 - Flags: review?(jdemooij) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1910489ee839
Don't sparsify dense elements eagerly in NativeDefineProperty. r=jandem
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1910489ee839
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.