Closed Bug 1461339 Opened 5 years ago Closed 4 years ago

Don't eagerly sparsify dense elements in NativeDefineProperty


(Core :: JavaScript Engine, enhancement)

Not set



Tracking Status
firefox62 --- fixed


(Reporter: anba, Assigned: anba)



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

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]

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

Attachment #8975492 - Flags: review?(jdemooij) → review+
Pushed by
Don't sparsify dense elements eagerly in NativeDefineProperty. r=jandem
Keywords: checkin-needed
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.