Consider splitting NativeObject::addProperty/putProperty in separate accessor vs data prop methods

RESOLVED FIXED in Firefox 58

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed, firefox59 fixed)

Details

Attachments

(19 attachments)

16.68 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
17.44 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
3.85 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
12.62 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
6.32 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
6.78 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
11.77 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
8.26 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
6.93 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
5.35 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
11.05 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
3.50 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
9.28 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
18.03 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
15.77 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
6.29 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
5.95 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
5.42 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
5.58 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
I'm doing this in bug 1394365 for the common NativeSetProperty case, but we might want to split these cases up more in general.

Basically addProperty and putProperty are Swiss Army knives that are used to add/put all kinds of properties and that makes them slower than necessary and hard to understand.

There's one wrinkle: after bug 1389510, slotful getters/setters don't really make sense, but I think our APIs allow you to define such a thing. I'd like to get rid of that possibility (and some crufty code) by making this impossible.
status-firefox57: --- → fix-optional
Priority: -- → P3
(Assignee)

Comment 1

a year ago
I have a stack of patches to massively simplify our Shape add/put code, and it's beautiful. Will post tomorrow.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
(Assignee)

Comment 2

a year ago
This splits addProperty and addPropertyInternal into addDataProperty* and addAccessorProperty*.

A bunch of code duplication here, I'll deal with that in later patches.
Attachment #8922733 - Flags: review?(bhackett1024)
(Assignee)

Updated

a year ago
status-firefox57: fix-optional → wontfix
(Assignee)

Comment 3

a year ago
Attachment #8922734 - Flags: review?(bhackett1024)
(Assignee)

Comment 4

a year ago
Attachment #8922735 - Flags: review?(bhackett1024)
(Assignee)

Comment 5

a year ago
Just a helper RAII class so we don't need to call obj->checkShapeConsistency() on every return.
Attachment #8922736 - Flags: review?(bhackett1024)
(Assignee)

Comment 6

a year ago
flags is always 0, slot is always SHAPE_INVALID_SLOT, so let's remove these arguments.
Attachment #8922738 - Flags: review?(bhackett1024)
(Assignee)

Comment 7

a year ago
changeProperty (only called for the array length property) passes shape->flags, but I don't think this is necessary.
Attachment #8922740 - Flags: review?(bhackett1024)
(Assignee)

Comment 8

a year ago
flags is always 0, allowDictionary is always true.

This also lets us remove a now-redundant addDataProperty overload.
Attachment #8922741 - Flags: review?(bhackett1024)
(Assignee)

Comment 9

a year ago
It's always 0.
Attachment #8922742 - Flags: review?(bhackett1024)
(Assignee)

Comment 10

a year ago
We pass allowDictionary = false in AddLengthProperty, but I don't think this is necessary.
Attachment #8922743 - Flags: review?(bhackett1024)
(Assignee)

Comment 11

a year ago
addDataProperty takes a |slot| argument. We can't remove it, but we can enforce that it's either SHAPE_INVALID_SLOT (allocate a new slot) or a reserved slot.

I think we can then remove the stableSlot check. Note that we assert this invariant in getChildProperty so I just removed that check - http://searchfox.org/mozilla-central/rev/dd47bee6468de7e1221b4d006342ad6b9813d0e5/js/src/vm/Shape.cpp#322-336
Attachment #8922746 - Flags: review?(bhackett1024)
(Assignee)

Comment 12

a year ago
This splits getChildProperty in getChildDataProperty and getChildAccessorProperty.
Attachment #8922748 - Flags: review?(bhackett1024)
(Assignee)

Comment 14

a year ago
It's 0 most of the time. In fixupShapeTreeAfterMovingGC we might pass a different value, but it doesn't matter as (re)hashing/matching ignores the flags argument.
Attachment #8922750 - Flags: review?(bhackett1024)
(Assignee)

Comment 15

a year ago
Attachment #8922751 - Flags: review?(bhackett1024)
(Assignee)

Comment 16

a year ago
Right now we pass ShapeTable::Entry* but not the ShapeTable* itself, so we need a separate ensureTableForDictionary call. That's a bit awkward and unnecessary - let's just pass the table with the entry.
Attachment #8922752 - Flags: review?(bhackett1024)
(Assignee)

Comment 17

a year ago
Simplifies the code a bit.
Attachment #8922754 - Flags: review?(bhackett1024)
(Assignee)

Comment 18

a year ago
This moves some code from addDataProperty and addAccessorProperty into maybeConvertToOrGrowDictionaryForAdd.
Attachment #8922757 - Flags: review?(bhackett1024)
(Assignee)

Comment 19

a year ago
Attachment #8922758 - Flags: review?(bhackett1024)
(Assignee)

Comment 20

a year ago
Attachment #8922759 - Flags: review?(bhackett1024)
(Assignee)

Comment 21

a year ago
That's all for now.

Splitting up the accessor vs data property code makes us faster because it removes a lot of branches and arguments, but I hope it will also make it easier to allocate object slots for (scripted) getter/setter properties (bug 1389159) when accessors have their own code path.
Attachment #8922733 - Flags: review?(bhackett1024) → review+
Attachment #8922734 - Flags: review?(bhackett1024) → review+
Attachment #8922735 - Flags: review?(bhackett1024) → review+
Attachment #8922736 - Flags: review?(bhackett1024) → review+
Attachment #8922738 - Flags: review?(bhackett1024) → review+
Attachment #8922740 - Flags: review?(bhackett1024) → review+
Attachment #8922741 - Flags: review?(bhackett1024) → review+
Attachment #8922742 - Flags: review?(bhackett1024) → review+
Attachment #8922743 - Flags: review?(bhackett1024) → review+
Attachment #8922746 - Flags: review?(bhackett1024) → review+
Attachment #8922748 - Flags: review?(bhackett1024) → review+
Attachment #8922749 - Flags: review?(bhackett1024) → review+
Attachment #8922750 - Flags: review?(bhackett1024) → review+
Attachment #8922751 - Flags: review?(bhackett1024) → review+
Attachment #8922752 - Flags: review?(bhackett1024) → review+
Attachment #8922754 - Flags: review?(bhackett1024) → review+
Attachment #8922757 - Flags: review?(bhackett1024) → review+
Attachment #8922758 - Flags: review?(bhackett1024) → review+
Attachment #8922759 - Flags: review?(bhackett1024) → review+

Comment 22

a year ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7cfc0c69ca7
part 1 - Split addProperty(Internal) in separate accessor vs data property overloads. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fccf231f26c
part 2 - Split putProperty in separate accessor vs data property overloads. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec639f2d7888
part 3 - Add an early return, unindent some code. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1489f9302ca
part 4 - Add an AutoCheckShapeConsistency RAII class. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fece077634f
part 5 - Remove slot and flags arguments from putDataProperty. r=bhackett
(Assignee)

Updated

a year ago
Keywords: leave-open

Comment 24

a year ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/88bfd12dc099
part 6 - Remove flags argument from putAccessorProperty. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/706c6d389111
part 7 - Remove flags and allowDictionary arguments from addDataProperty. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/444453765199
part 8 - Remove flags argument from addAccessorProperty. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/19d68ad55452
part 9 - Remove allowDictionary argument from addAccessorProperty. r=bhackett

Comment 26

a year ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d8e2a8c4c03
part 10 - Simplify addDataProperty's slot argument. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/b44bda5764ca
part 11 - Split getChildProperty in data vs accessor versions. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/aba68ef3a080
part 12 - Remove unused flags argument from matchesParamsAfterId. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb4629b12e96
part 13 - Remove flags argument from StackShape constructor. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/18f007118b1c
part 14 - Fix comments. r=bhackett

Comment 28

a year ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef44b38b852a
part 15 - Pass ShapeTable* to add*Property. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce448d8a91b3
part 16 - Scope table/entry better in putDataProperty/putAccessorProperty. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1b13194212c
part 17 - Factor out maybeConvertToOrGrowDictionaryForAdd. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/53a3033449d3
part 18 - Factor out updateDictionaryTable. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb39c30dc214
part 19 - Factor out maybeToDictionaryModeForPut. r=bhackett
(Assignee)

Updated

a year ago
Keywords: leave-open
(Assignee)

Comment 30

a year ago
Most of this (definitely the more important bits) landed in 58 so I think it's better to track this as fixed in 58.
status-firefox58: --- → fixed
Target Milestone: mozilla59 → mozilla58
You need to log in before you can comment on or make changes to this bug.