841059 - GC: restrict JSObject's access to its ArenaHeader to the GC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Objects will not always have an arena header available to look at, so redirect the access to the shape.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This is happening for zones anyway.

Comment 2

[Terrence Cole [:terrence]]

Ah, thanks! Fixed the title.

Comment 3

[Terrence Cole [:terrence]]

Actually, I think I'm just going to broaden this bug and post all 4 patches here, since they'll probably be easier to review like that.

Comment 4

[Terrence Cole [:terrence]]

Attachment: v0: 1 of 4 - Add isTenured assertions.

This adds an isTenured() and calls it for all access to the arenaHeader. At the moment it just returns true, but the intent should be clear. Since access to the JSRuntime has to happen out of the header to avoid a circular dependency, it is added as #ifdef DEBUG to ensure that it is only used by assertions.

When creating this series, I noticed that Cell::chunk and Cell::address are only used internally to Cell and subclasses. I made these protected in this patch since I was already touching the Cell definition.

Comment 5

[Terrence Cole [:terrence]]

Attachment: v6: 2 of 4 - Specialize zone and compartment on JSObject.

This is the main patch in the series.

Comment 6

[Terrence Cole [:terrence]]

Attachment: v0: rename getAllocKind to tenuredGetAllocKind

Sorry for the delay, I wanted to test GGC on top of the last two patches in the series to make sure they wouldn't need any further tweaking.

It turns out we only need a non-tenured version of getAllocKind for JSFunction and JSObject. JSObject::getAllocKind is a fairly complex function, unfortunately, but it seems to pass all tests.

Comment 7

[Terrence Cole [:terrence]]

Attachment: v0: trivial

Comment 8

[Terrence Cole [:terrence]]

Attachment: v1: 4 of 4 Re-adding some bits that got dropped along the way.

It is important not to call sizeOfThis() on an uninitialized object. This should also make the cached object creation path a bit faster.

Comment 9

[Terrence Cole [:terrence]]

With the patch from Bug 843337, neither SS or v8 show a significant perf regressions on shookfoil. I've thrown up a talos run at: https://tbpl.mozilla.org/?tree=Try&rev=82ef8e5a1d63

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 713619 [details] [diff] [review]
v6: 2 of 4 - Specialize zone and compartment on JSObject.

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

Please don't land this until zones lands. The zones patch already adds a compartment() method to each GC thing type and fixes up the Cell uses, so you won't have to make any of the tenuredCompartment changes.

::: js/src/gc/Barrier-inl.h
@@ +42,5 @@
>  #endif
>  }
>  
> +/* static */ JS_ALWAYS_INLINE JS::Zone *
> +EncapsulatedValue::zone(const Value &value)

This is an odd place to put this function, especially since it would be nice to be able to use it outside the barrier code. I would just create a function called ZoneOfValue or something and put it somewhere central. Or else you could try to add it as a method of Value, although that might cause problems.

::: js/src/gc/Marking.cpp
@@ +1614,5 @@
>          return;
>  
>      UnmarkGrayGCThing(thing);
>  
> +    JSRuntime *rt = static_cast<Cell *>(thing)->tenuredZone()->rt;

Better just to convert this to thing->runtime().

::: js/src/jswrapper.cpp
@@ +1030,5 @@
>      Value old = wrapper->getSlot(slot);
>      if (old.isMarkable()) {
> +        Zone *zone = old.isObject()
> +                     ? old.toObject().zone()
> +                     : static_cast<Cell *>(old.toGCThing())->tenuredZone();

You could use ZoneOfValue here.

::: js/src/vm/ObjectImpl-inl.h
@@ +191,5 @@
>      return getSlot(slot);
>  }
>  
> +static JS_ALWAYS_INLINE JSCompartment *
> +ValueCompartment(const js::Value &value)

This function doesn't appear to be used. Also, it's incompatible with the zones work (because strings won't have a compartment).

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 715695 [details] [diff] [review]
v0: rename getAllocKind to tenuredGetAllocKind

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

Just waiting for an answer about ObjectImpl::getAllocKind...

::: js/src/jsfun.h
@@ +298,5 @@
>                                             bool singleton = false);
>  
> +    /* GC support. */
> +    js::gc::AllocKind getAllocKind() const {
> +        if (isExtended())

Can you add an assertion here, like the one above, that if it's tenured then we're returning the right alloc kind?

::: js/src/jsgcinlines.h
@@ +56,3 @@
>      JS_ASSERT(thing);
>      const Cell *cell = reinterpret_cast<const Cell *>(thing);
> +    return MapAllocToTraceKind(cell->tenuredGetAllocKind());

GetGCThingTraceKind is really called only for tenured stuff? It looks like the browser calls it, but I guess those can be fixed up later somehow. Maybe we could check if it's in the range of the nursery.

::: js/src/vm/ObjectImpl-inl.h
@@ +19,5 @@
>  #include "js/TemplateLib.h"
>  
>  #include "ObjectImpl.h"
>  
> +#include "jsgcinlines.h"

What's this for?

::: js/src/vm/ObjectImpl.cpp
@@ +317,5 @@
>      }
>  }
>  
> +gc::AllocKind
> +js::ObjectImpl::getAllocKind() const

Who uses this? It seems a little dangerous to be exposing it at so low a level. If it's only needed for the nursery collection, then it would be better to make it a static function in jsgc.cpp.

::: js/src/vm/ObjectImpl.h
@@ +1067,5 @@
>                            "shadow placeholder must match actual elements");
>      }
>  
>      JSObject * asObjectPtr() { return reinterpret_cast<JSObject *>(this); }
> +    const JSObject * asConstObjectPtr() const { return reinterpret_cast<const JSObject *>(this); }

It's okay to call this asObjectPtr(), I believe. The "const" qualifier should be enough to distinguish them.

Comment 12

[Terrence Cole [:terrence]]

(In reply to Bill McCloskey (:billm) from comment #11)
> ::: js/src/jsgcinlines.h
> @@ +56,3 @@
> >      JS_ASSERT(thing);
> >      const Cell *cell = reinterpret_cast<const Cell *>(thing);
> > +    return MapAllocToTraceKind(cell->tenuredGetAllocKind());
> 
> GetGCThingTraceKind is really called only for tenured stuff? It looks like
> the browser calls it, but I guess those can be fixed up later somehow. Maybe
> we could check if it's in the range of the nursery.

This is by far the weakest link of the series. My rough plan is to have the browser pass us the runtime and/or a trace kind so we can do the right thing here.
 
> ::: js/src/vm/ObjectImpl-inl.h
> @@ +19,5 @@
> >  #include "js/TemplateLib.h"
> >  
> >  #include "ObjectImpl.h"
> >  
> > +#include "jsgcinlines.h"
> 
> What's this for?

It appears orphaned -- I had tried inlining getAllocKind, but it didn't work out for other reasons.
 
> ::: js/src/vm/ObjectImpl.cpp
> @@ +317,5 @@
> >      }
> >  }
> >  
> > +gc::AllocKind
> > +js::ObjectImpl::getAllocKind() const
> 
> Who uses this? It seems a little dangerous to be exposing it at so low a
> level. If it's only needed for the nursery collection, then it would be
> better to make it a static function in jsgc.cpp.

I see 5 main classes of user when building without it present:

1) Checking whether to call the finalizer in ::finalize, which Jon and I both have patches out to remove.
2) Basically every object subclass asserts that it got the right thing kind or is looking at the right thing kind in various places. These are nice, but not vital.
3) The NewObjectCache uses getAllocKind on the template -- I actually have a patch I've been meaning to ask you or Brian to review that switches this to using the kind that is cached with the object.
4) jsobj.cpp calls it on |this| all over the place when creating and modifying shapes.
5) JM and Ion call getAllocKind on the template object to select the AllocKind to bake into the inline NewObject paths.
6) The minor collection needs it.

1 - 3 we can kill easily, but I'm not sure how we'd handle 4 & 5.

Comment 13

[Terrence Cole [:terrence]]

Attachment: 3 of 4: v1

This patch assumes we're going to keep getAllocKind on ObjectImpl. If you can think of a good solution, I'll re-roll it.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 719643 [details] [diff] [review]
3 of 4: v1

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

OK. Can you add an assertion that ObjectImpl::getAllocKind returns the same as tenuredGetAllocKind as long as the object isn't in the nursery?

Comment 15

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ba3337f676f
https://hg.mozilla.org/integration/mozilla-inbound/rev/035af5615fb4

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/035af5615fb4
https://hg.mozilla.org/mozilla-central/rev/6ba3337f676f

Comment 17

[Terrence Cole [:terrence]]

Green try run at:
https://tbpl.mozilla.org/?tree=Try&rev=82ef8e5a1d63

Pushed at:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e0347b9a4e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/085337fa9658

Comment 18

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/79f5f64f33b1 for complete scorched-earth failure.

Comment 19

[Terrence Cole [:terrence]]

Re-pushing the tenured zone patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e658b893b5e

Green try run at:
https://tbpl.mozilla.org/?tree=Try&rev=75f8b09adb0a

Comment 20

[Terrence Cole [:terrence]]

Followup fix for a zone() call I missed in the root analysis build:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f447e09d5a0c

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7e658b893b5e
https://hg.mozilla.org/mozilla-central/rev/f447e09d5a0c

Comment 22

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a97d33c3a07

Comment 23

[Terrence Cole [:terrence]]

Comment on attachment 719643 [details] [diff] [review]
3 of 4: v1

Thanks for pushing this!

Comment 24

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/5a97d33c3a07