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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Jan de Mooij [:jandem]]

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.

Comment 1

[Jan de Mooij [:jandem]]

I have a stack of patches to massively simplify our Shape add/put code, and it's beautiful. Will post tomorrow.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Split addProperty/addPropertyInternal

This splits addProperty and addPropertyInternal into addDataProperty* and addAccessorProperty*.

A bunch of code duplication here, I'll deal with that in later patches.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Split putProperty

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Add early return, unindent code

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Add AutoCheckShapeConsistency

Just a helper RAII class so we don't need to call obj->checkShapeConsistency() on every return.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Remove slot and flags arguments from putDataProperty

flags is always 0, slot is always SHAPE_INVALID_SLOT, so let's remove these arguments.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 6 - Remove flags argument from putAccessorProperty

changeProperty (only called for the array length property) passes shape->flags, but I don't think this is necessary.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 7 - Remove flags and allowDictionary arguments from addDataProperty

flags is always 0, allowDictionary is always true.

This also lets us remove a now-redundant addDataProperty overload.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Part 8 - Remove flags argument from addAccessorProperty

It's always 0.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Part 9 - Remove allowDictionary argument from addAccessorProperty

We pass allowDictionary = false in AddLengthProperty, but I don't think this is necessary.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Part 10 - Simplify addDataProperty slot argument

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

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Part 11 - Split getChildProperty

This splits getChildProperty in getChildDataProperty and getChildAccessorProperty.

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Part 12 - Remove unused flags argument from matchesParamsAfterId

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Part 13 - Remove flags argument from StackShape constructor

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.

Comment 15

[Jan de Mooij [:jandem]]

Attachment: Part 14 - Fix comments

Comment 16

[Jan de Mooij [:jandem]]

Attachment: Part 15 - Pass ShapeTable to add*Property

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.

Comment 17

[Jan de Mooij [:jandem]]

Attachment: Part 16 - Scope table/entry better in putDataProperty/putAccessorProperty

Simplifies the code a bit.

Comment 18

[Jan de Mooij [:jandem]]

Attachment: Part 17 - Factor out maybeConvertToOrGrowDictionaryForAdd

This moves some code from addDataProperty and addAccessorProperty into maybeConvertToOrGrowDictionaryForAdd.

Comment 19

[Jan de Mooij [:jandem]]

Attachment: Part 18 - Factor out updateDictionaryTable

Comment 20

[Jan de Mooij [:jandem]]

Attachment: Part 19 - Factor out maybeToDictionaryModeForPut

Comment 21

[Jan de Mooij [:jandem]]

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.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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

Comment 23

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/b7cfc0c69ca7
https://hg.mozilla.org/mozilla-central/rev/1fccf231f26c
https://hg.mozilla.org/mozilla-central/rev/ec639f2d7888
https://hg.mozilla.org/mozilla-central/rev/d1489f9302ca
https://hg.mozilla.org/mozilla-central/rev/4fece077634f

Comment 24

[Pulsebot]

Pushed by [e-mail]:
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 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/88bfd12dc099
https://hg.mozilla.org/mozilla-central/rev/706c6d389111
https://hg.mozilla.org/mozilla-central/rev/444453765199
https://hg.mozilla.org/mozilla-central/rev/19d68ad55452

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9d8e2a8c4c03
https://hg.mozilla.org/mozilla-central/rev/b44bda5764ca
https://hg.mozilla.org/mozilla-central/rev/aba68ef3a080
https://hg.mozilla.org/mozilla-central/rev/fb4629b12e96
https://hg.mozilla.org/mozilla-central/rev/18f007118b1c

Comment 28

[Pulsebot]

Pushed by [e-mail]:
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

Comment 29

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/ef44b38b852a
https://hg.mozilla.org/mozilla-central/rev/ce448d8a91b3
https://hg.mozilla.org/mozilla-central/rev/f1b13194212c
https://hg.mozilla.org/mozilla-central/rev/53a3033449d3
https://hg.mozilla.org/mozilla-central/rev/cb39c30dc214

Comment 30

[Jan de Mooij [:jandem]]

Most of this (definitely the more important bits) landed in 58 so I think it's better to track this as fixed in 58.