1137180 - Extensible unboxed objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

The original/current design of unboxed objects fixes a set of properties for an unboxed object at creation, and converts the object to a native if that set of properties ever changes.

It seems now that this design is not robust enough.  It is common to add new properties to objects, and this may happen well after the object is created.  Moreover, doing so still can have a large associated penalty, even after bug 1133254 and bug 1133369.  This penalty is because the unboxed object is smaller than the native object would have otherwise been, so more properties of the native object will end up being in dynamic slots, which are slow to access (partly due to an extra pointer chase, but mostly I think due to cache issues since the slot array could be at some random place in the malloc heap or nursery).

This hurts on octane-box2d, and hurts a lot on kraken-ai-astar.  One possible workaround for these tests is to do a nursery GC when first converting the objects to natives, which in both these cases would catch all live unboxed objects and allow us to make sure they are all native with some larger allocation kind.  I think this approach is cheesy, though, and unlikely to help in all that many real world cases (I doubt it would fix ai-astar in the browser, for example).

A better approach is to support extending unboxed objects with new boxed properties, converting them to native objects only on other property changes (deletion, make non-configurable, etc.) or type changes to the unboxed properties.

This should be doable without sacrificing the space efficiency of unboxed objects.  In an email, Luke pointed out that the Shape* of an object is pretty much vestigial for non-native objects.  If we remove the shape from unboxed objects, its space can be repurposed to store a set of boxed properties for the object, probably something like:

[group, properties, ... unboxed data ...]
            ||
            \/
          [shape, ... boxed values ...]

The shape in the properties structure would store the names and ordering of the extra properties on the object, and could be guarded on in JIT caches.  The properties pointer would be null if the object has no extra properties.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Fwiw, we have JIT guards that guard on the shape of proxies solely because shape covers class and hence the handler.

We could change those guards, of course; just need to be a bit careful about dropping shapes from proxies.

I guess unboxed objects flowing into a jit shape guard would be ok, since they'd just fail the shape guard which is what's wanted anyway?

Comment 2

[Brian Hackett [Laid off!]]

(In reply to Not doing reviews right now from comment #1)
> Fwiw, we have JIT guards that guard on the shape of proxies solely because
> shape covers class and hence the handler.
> 
> We could change those guards, of course; just need to be a bit careful about
> dropping shapes from proxies.
> 
> I guess unboxed objects flowing into a jit shape guard would be ok, since
> they'd just fail the shape guard which is what's wanted anyway?

For now, proxies and typed objects would still have a Shape pointer in the same place.  We need this regardless because the shape stores non-property information like object flags and things that can be set on non-native objects (e.g. whether the object is being watched()).  If we try to set one of these flags on an unboxed object we can just convert it to a native, but that option doesn't exist for other non-native objects.

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Not doing reviews right now from comment #1)
> I guess unboxed objects flowing into a jit shape guard would be ok, since
> they'd just fail the shape guard which is what's wanted anyway?

Yes, the value stored in the |properties| field of an unboxed object will never hit on a shape guard.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: WIP

This WIP goes on top of bug 1137497 and adds the ability to extend unboxed objects with new properties distinct from their unboxed properties.  Instead of using a custom rolled structure for the extra properties this patch just stores them via a DOM style expando object.  This costs a little more memory (four words of header for an object vs. one word for a shape+array), but is a lot more flexible, allows for a lot more code reuse, and doesn't require fixing the generational collector to allow nursery things to hold onto new kinds of heap memory.  This patch also adds baseline and Ion ICs for getting, setting, and adding new properties to unboxed expando objects.  This seems to fix the remaining box2d regression (hard to tell though since it's so small) and improves kraken-ai-astar by a lot, from 125ms to 65ms.  Running without --unboxed-objects gives 82ms (all numbers x86 10.9).  I think this improvement is because the hot loop in ai-astar accesses unboxed properties of various unboxed objects in the grid, and the unboxed objects are much more tightly packed than native objects (FINALIZE_OBJECT2 instead of FINALIZE_OBJECT8) so we get much better data locality.

So this patch works but is a WIP since it is pretty messy and needs bug 1135816 to land to allow cleaning up a lot of the IC changes.  It also needs a bunch of tests.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: patch

Updated patch which makes JIT stubs for constructing unboxed objects while parsing JSON or script sources.  If I run kraken-json-parse-financial ten times as long on x86 10.9, I get 470ms before this patch (both with/without unboxed objects), 460ms after this patch without unboxed objects, and 410ms after this patch with unboxed objects.

We could improve this in the future to e.g. allow these JIT stubs to create boxed objects as well, but the heuristics involved and management of the JitCode* make this kind of complicated so this patch does the simple thing.  This should be the last feature patch for unboxed objects (at least until we start having unboxed arrays), thanks for all the reviews!

Comment 6

[Brian Hackett [Laid off!]]

Oh jeez, I attached this to the wrong bug.

Comment 7

[Brian Hackett [Laid off!]]

Attachment: improve bounds check failure handling

Fix an issue I ran into while testing the main patch.  On ai-astar we ended up with bounds checks in the main loop because some random other script that got inlined into the same outer script had a bounds check failure.  Bounds checks failures should be marked on the innermost script to improve robustness in this sort of case.

Comment 8

[Brian Hackett [Laid off!]]

Attachment: patch

VM and JIT changes to support attaching expando objects to unboxed objects.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Brian Hackett (:bhackett) from comment #7)
> Bounds checks failures should be marked on the innermost script to improve
> robustness in this sort of case.

I think this is a problem when the bounds check is hoisted into an outer script. When we bail, we can use some outer resume point and innerScript does not necessarily refer to the script that had the bounds check failure :(

Is this a bounds check failure we can easily avoid with Baseline feedback?

We do have a separate mechanism (CheckFrequentBailouts) that disables LICM when an IonScript keeps bailing, but for that to work here we probably don't want to invalidate if the hadBoundsCheckFailure flag is already set...

Comment 10

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #9)
> I think this is a problem when the bounds check is hoisted into an outer
> script. When we bail, we can use some outer resume point and innerScript
> does not necessarily refer to the script that had the bounds check failure :(

OK, but how is that worse than our current behavior, where we always mark a bounds check failure in the outer script, which has even less to do with the failure than the innermost script?

Well, if this happens then the true inner script's access was hoisted from a loop in the outer script, so we would have something like:

function functionA(a, i) {
  do_something_with(a[i]);
}

function functionB(a, n) {
  for (var i = 0; i < n; i++)
    functionA(a, i);
}

If we inline functionA into functionB and successfully hoist the bounds check in functionA out of the loop in functionB, if the hoisted bounds check fails then both before and after this patch we would invalidate functionB.  In later compilations we would not consolidate bounds checks in functionB or anything inlined into it, including functionA.

The difference with this patch is that if there is a functionC which inlined functionB, before this patch we mark functionC as having failed a bounds check.  After the patch we mark functionB as having failed.  I think marking functionB as failed is better than functionC, as functionC doesn't have anything to do with the location where the failure occurred, and marking functionC as having a failure can impact all sorts of other code which functionC also inlines (which is the problem I ran into with kraken-ai-astar).  Maybe it would be better to mark functionA itself as having failed, but I don't think it would be that much of an improvement, and it would be hard to implement as a hoisted/consolidated bounds check can cover checks from any number of other inlined functions.

> Is this a bounds check failure we can easily avoid with Baseline feedback?

Potentially, but it would be better imo to fix the underlying problem.  Otherwise we are just papering over things and will continue to hit this in the future.

> We do have a separate mechanism (CheckFrequentBailouts) that disables LICM
> when an IonScript keeps bailing, but for that to work here we probably don't
> want to invalidate if the hadBoundsCheckFailure flag is already set...

Well, the first time we have a bounds check failure we invalidate the script, and when recompiling will have the failedBoundsCheck flag set and won't move any of the bounds check instructions that are compiled.  If those immovable bounds checks keep failing though then we will keep invalidating the script, but that is an issue with the current design as well and seems hard to fix in general, e.g. by adding/using out-of-bounds handling instructions like MLoadElementHole instead of always generating a bounds check.

Comment 11

[Brian Hackett [Laid off!]]

Bounds check improvements:

https://hg.mozilla.org/integration/mozilla-inbound/rev/97d508451d6c

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/97d508451d6c

Comment 13

[Jan de Mooij [:jandem]]

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

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

Sorry for the delay. Looks good; and seems like it'll make unboxed objects a lot more robust.

::: js/src/jit/BaselineIC.h
@@ +4488,1 @@
>  class ICGetProp_CallNativePrototype : public ICGetPropCallPrototypeGetter

I removed this class and merged it with CallNative. CallScripted handles own scripted getters now. See bug 1128646; there'll probably be some merge conflicts because of that...

::: js/src/jit/MacroAssembler.cpp
@@ +1269,5 @@
>          }
>      } else if (templateObj->is<UnboxedPlainObject>()) {
>          const UnboxedLayout &layout = templateObj->as<UnboxedPlainObject>().layout();
>  
> +        storePtr(ImmWord(0), Address(obj, UnboxedPlainObject::offsetOfExpando()));

Nice.

::: js/src/vm/UnboxedObject.cpp
@@ +336,5 @@
>  
> +    if (expando) {
> +        // Add properties from the expando object to the object, in order.
> +        // Suppress GC here, so that callers don't need to worry about this
> +        // method collecitng. The stuff below can only fail due to OOM, in

Nit: "collecting"

Comment 14

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/df82a0b667bb

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for being the likely cause of bug 1143590.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a827268f69e

Comment 16

[Brian Hackett [Laid off!]]

I haven't narrowed down the exact cause of the failure but it seems to have been caused by the baseline changes.  This push includes everything else, though creation of the unboxed expandos will be disabled until the rest gets in:

https://hg.mozilla.org/integration/mozilla-inbound/rev/03b0e7eac69d
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cbee2841c90

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/03b0e7eac69d

Comment 18

[Brian Hackett [Laid off!]]

All the baseline caches, except for SETPROP/ADDPROP, which I reverted to their original form.  Still need to track down what the problem is there, but this change allows extensible unboxed objects to be turned on.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7147b36dd400

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7147b36dd400

Comment 20

[Brian Hackett [Laid off!]]

I'll write a patch in another bug for the SETPROP/ADDPROP stuff.