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

RESOLVED FIXED in mozilla22

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(4 attachments, 2 obsolete attachments)

Objects will not always have an arena header available to look at, so redirect the access to the shape.
This is happening for zones anyway.
Ah, thanks! Fixed the title.
Depends on: 841065
Summary: GC: access object compartment through the shape → GC: access object zone through the shape
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.
Summary: GC: access object zone through the shape → GC: restrict JSObject's access to its ArenaHeader to the GC
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.
Attachment #713614 - Flags: review?(wmccloskey)
This is the main patch in the series.
Attachment #713619 - Flags: review?(wmccloskey)
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.
Attachment #715695 - Flags: review?(wmccloskey)
Posted patch v0: trivial (obsolete) — Splinter Review
Attachment #715698 - Flags: review?(wmccloskey)
It is important not to call sizeOfThis() on an uninitialized object. This should also make the cached object creation path a bit faster.
Attachment #715698 - Attachment is obsolete: true
Attachment #715698 - Flags: review?(wmccloskey)
Attachment #716113 - Flags: review?(wmccloskey)
Depends on: 843337
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
Attachment #713614 - Flags: review?(wmccloskey) → review+
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).
Attachment #713619 - Flags: review?(wmccloskey) → review+
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.
Attachment #715695 - Flags: review?(wmccloskey)
Attachment #716113 - Flags: review?(wmccloskey) → review+
(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.
Posted patch 3 of 4: v1Splinter Review
This patch assumes we're going to keep getAllocKind on ObjectImpl. If you can think of a good solution, I'll re-roll it.
Attachment #715695 - Attachment is obsolete: true
Attachment #719643 - Flags: review?(wmccloskey)
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?
Attachment #719643 - Flags: review?(wmccloskey) → review+
Attachment #716113 - Flags: checkin+
Attachment #713614 - Flags: checkin+
Whiteboard: [js:t] → [js:t] [leave-open]
Whiteboard: [js:t] [leave-open] → [js:t]
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/79f5f64f33b1 for complete scorched-earth failure.
Depends on: 852667
Whiteboard: [js:t] → [js:t] [leave-open]
Followup fix for a zone() call I missed in the root analysis build:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f447e09d5a0c
Attachment #713619 - Flags: checkin+
Comment on attachment 719643 [details] [diff] [review]
3 of 4: v1

Thanks for pushing this!
Attachment #719643 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/5a97d33c3a07
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.