921902 - Separate generation and attaching of heap property type constraints

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

For bug 785905, IonBuilder will need to run off thread and keep track of its dependencies on heap property type sets via freeze/etc. type constraints.  This is problematic for the way that these constraints are currently generated, as IonBuilder directly attaches the type constraints to the associated properties.  While things could be relaxed so that type constraints can be attached off thread (using a lock), doing this attaching requires lazy generation of singleton object types and type object properties, and the latter two definitely need to happen only on the main thread --- otherwise the main thread could not read type information without taking a lock.

If IonBuilder kept track of its dependencies on heap properties in a way that didn't require immediately attaching the constraints and generating lazy types / properties, this conundrum would be avoided.  The constraints would be generated off thread, then attached on the main thread any time after the IonBuilder finishes.

The attached patch makes preparations for these changes, with interface changes so that freeze constraints are generated using TypeObjectKeys and HeapTypeSetKeys which can be generated without a JSContext or VM mutations.  Under the hood things behave exactly the same, however --- the JSContext and VM mutations still occur.  The only functional changes made by this patch are:

- Constraints are not attached to type sets until the entire compilation finishes.  When using parallel compilation, in progress compilations won't be invalidated by type changes.  This change doesn't seem to affect benchmarks.

- The frontend was updated to not generate {GET,SET,INIT}PROP ops for property names which TI considers to be indexes.  This means that IonBuilder doesn't need to use IdToTypeId anywhere (yay).  While this change could be split out, most of what it touches is also touched by the rest of the patch and I added it late in the game on this patch to clean up name/id functions in the builder.

Comment 1

[Jan de Mooij [:jandem]]

Comment on attachment 811757 [details] [diff] [review]
patch

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

::: js/src/jsinfer.cpp
@@ +926,3 @@
>      {
> +        // FIXME: There is not yet any way to test if constraints with no
> +        // associated flags (i.e. those invalidated via |force|) still hold.

Is this a problem we should fix now?

Comment 2

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48582b2df0af

Comment 3

[Phil Ringnalda (:philor)]

That broke the Android no-ion build, https://tbpl.mozilla.org/php/getParsedLog.php?id=28733108&tree=Mozilla-Inbound. Lucky for you, now you get to break it without getting backed out since it's hidden pending removal, but that probably also means you broke semi-supported no-ion builds, and you'll want to fix it eventually.

Comment 4

[Hannes Verschore [:h4writer]]

According to awfy we have a regression in
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c7bb40f2578a&tochange=a587ed24ade0

Brian, could it be that this patch caused this? It could also be one of sunfish his patches, but first want to exclude you, before searching which one of his patched could have caused this...

Comment 5

[Brian Hackett [Laid off!]]

(In reply to Hannes Verschore [:h4writer] (PTO:Sept 23-27) from comment #4)
> According to awfy we have a regression in
> http://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=c7bb40f2578a&tochange=a587ed24ade0
> 
> Brian, could it be that this patch caused this? It could also be one of
> sunfish his patches, but first want to exclude you, before searching which
> one of his patched could have caused this...

This is being tracked in bug 923659.  I can't reproduce the regression.

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/48582b2df0af

Comment 7

[Brian Hackett [Laid off!]]

Fix for --disable-ion breakage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b6ee2745980