1544429 - Improve IonBuilder's TI interface

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine: JIT, enhancement, P2)

Description

[Jan de Mooij [:jandem]]

IonBuilder currently uses raw TypeSets and object/group pointers but it can be hard to reason about the invariants and there's unnecessary boilerplate.

After some brainstorming, Ted and I came up with the following design:

• Add a TISnapshot class with higher-level methods for all TI-related queries.

• IonBuilder will stop using raw JSObject*, TypeSet*, ObjectKey. Instead, it will have opaque types/handles (TISnapshot::Object, TISnapshot::HeapTypeSet) it can pass to (and get from) TISnapshot.

• The goal is to make the TISnapshot class hard to misuse. The few places that want to do tricky things can (temporarily) opt-in via unsafeFoo() methods we can audit.

• TI queries will be modeled (as much as possible) after VM operations. For example, to check if a TISnapshot::Object has a property: snapshot.hasOwnProperty(object, id). TISnapshot will add the right constraints.

Comment 1

[Ted Campbell [:tcampbell]]

Another pattern that we observe happening in IonBuilder is accessing unguarded Object/TI data to decide if an optimization would apply before any constraints are fired. This is error-prone but could be handled relatively straightforward by having an RAII transaction class. The sequence of constraints would be collected during transaction and on abort they would be discarded before being applied. In practice, we would only have small amounts of constraints here since current code is manually juggling the constraints so doesn't get very complicated.

Comment 2

[Kannan Vijayan [:djvj]]

To add some notes here. I've been doing some relevant investigation into Ion invalidations during page-load, in bug 1543385. I have yet to summarize and report the findings, but there is some preliminary data there.

What I'm finding is that the amount of specificity in TI typesets (both stack typesets and heap typesets) is too high. There are a large number of invalidations coming from situations where we are adding a new type to a type-set that is already full.

Specifically, things like "add null to a type-set containing void,int,string,object", or "add Group-A to a type-set containing Group-B,Group-C,Group-D,Group-E". The existing type-set specificity in these cases is not enough to provide any additional optimization opportunity over the new (modified) type-set, yet there is a freeze taken on the specific contents.

Ion does not benefit from type-specialization with TI beyond monomorphic types. The type-sets currently are designed to introduce specificity in contexts that doesn't help us, and additionally introduce invalidations that should be spurious (we should have and probably are optimizing on the generalized type, but we are freezing on the list of specific types).

Part of this TI interface improvement can include a significant simplification of typesets. I would suggest that type-sets should generalize aggressively - quickly moving towards an any type.

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Kannan Vijayan [:djvj] from comment #2)

What I'm finding is that the amount of specificity in TI typesets (both stack typesets and heap typesets) is too high. There are a large number of invalidations coming from situations where we are adding a new type to a type-set that is already full.

This sounds orthogonal to the issue which is mentioned here, which is purely a cosmetic layer on top of what is implemented today, such that C++ type system enforces us to make use of TI to poke at GC things.

Basically what is described here is a membrane which you always have to go through, and which happen to record TI information as you query it.

Comment 4

[Kannan Vijayan [:djvj]]

It's somewhat orthogonal to the specific intention here. However - given that some of the implementation choices will end up running across each other, I thought it worthwhile to mention.

For example, fixing up the type-punning around objectSet (which can alternatively be an ObjectKey* (single object), ObjectKey** (array), or ObjectKey** (hashtable). Now, cleaning all of that up might be a bit redundant when we expect to eliminate the idea of storing multiple ObjectGroups in a single type-set in the first place.

So providing a safe wrapper type (ObjectKey instead of ObjectKey*) makes sense. We may care less about fixing up the array and hash-table punning of the double-indirection pointer type of TypeSet.objectSet.

Comment 5

[Iain Ireland [:iain]]

Attachment: Bug 1544429: Part 1: Add TIOracle and rewrite ElementAccessHasExtraIndexedProperty to use it

Comment 6

[Iain Ireland [:iain]]

Attachment: Bug 1544429: Part 2: Expand TIOracle to ElementAccessIs[Foo]

Depends on D30963

Comment 7

[Iain Ireland [:iain]]

Attachment: Bug 1544429: Part 3: Convert DenseNativeElementType to TIOracle.

Depends on D31332

Comment 8

[Iain Ireland [:iain]]

Attachment: Bug 1544429: Part 4: Introduce range-based iterator for iterating over objects in typeset

Depends on D31333

Comment 9

[Iain Ireland [:iain]]

Attachment: Bug 1544429: Part 5: Convert PropertyReadIsIdempotent to use TIOracle

Depends on D31334

Comment 10

[Iain Ireland [:iain]]

Attachment: Bug 1544429: Part 0: Fix non-unified ARM build r=tcampbell

When I added TIOracle.cpp, this missing header broke the unified ARM build. (We don't test non-unified ARM builds by default.)

Comment 11

[Iain Ireland [:iain]]

Attachment: Bug 1544429: Part 1: Add TIOracle r=tcampbell

Depends on D38729

Comment 12

[Iain Ireland [:iain]]

Attachment: Bug 1544429: Part 2: Introduce range-based iterator for iterating over objects in typeset r=tcampbell

Depends on D38730

Comment 13

[Iain Ireland [:iain]]

Attachment: Bug 1544429: Part 3: Convert ElementAccessHasExtraIndexedProperty into TIOracle::ElementHolesMayBeDefined r=tcampbell

"ExtraIndexedProperty" is overly vague. What we're actually querying is whether a hole in the dense elements array implies that the element is undefined, or whether holes may be defined (whether by a non-data property or somewhere on the prototype chain). If the query returns false, then JSWhyMagic::JS_ELEMENTS_HOLE can be safely interpreted as undefined (and constraints will be added to enforce that). If it returns true, then we have to go look up the property elsewhere.

ElementAccessHasExtraIndexedProperty and TypeCanHaveExtraIndexedProperties are folded together.

Depends on D38731

Comment 14

[Iain Ireland [:iain]]

Attachment: Bug 1544429: Part 4: Move ElementAccessIs[Foo] into TIOracle r=tcampbell

Depends on D38732

Comment 15

[Iain Ireland [:iain]]

Attachment: Bug 1544429: Part 5: Move PropertyReadIsIdempotent into TIOracle r=tcampbell

Depends on D38733

Comment 16

[Iain Ireland [:iain]]

Attachment: Bug 1544429: Part 6: Move DenseNativeElementType into TIOracle r=tcampbell

Depends on D38734

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20e1a3ecd04a
Part 0: Fix non-unified ARM build r=tcampbell

Comment 18

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/20e1a3ecd04a

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Comment 20

[Iain Ireland [:iain]]

Forgot the darn leave-open tag.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d1d7fc0e34c
Part 1: Add TIOracle r=tcampbell,jandem

Comment 22

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/6d1d7fc0e34c

Comment 24

[Iain Ireland [:iain]]

Our new plan for fixing the TI interface is to remove TI completely and use WarpBuilder instead. This bug is no longer relevant.