1461339 - Don't eagerly sparsify dense elements in NativeDefineProperty

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[André Bargull [:anba]]

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

Comment 1

[Jan de Mooij [:jandem]]

That means we can remove sparsifyDenseElement? Cool!

Comment 2

[André Bargull [:anba]]

Attachment: bug1461339.patch

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

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8975492 [details] [diff] [review]
bug1461339.patch

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

\o/

Comment 4

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9eb37cd3af9041b56671d6438b03cf339789234d

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1910489ee839
Don't sparsify dense elements eagerly in NativeDefineProperty. r=jandem

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/1910489ee839