791611 - More exact rooting in jsinfer.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

jsinfer.cpp has lots of unrooted GCThings.  This bug is about reducing that number.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Exactly root most JSObjects in jsinfer.cpp.

This patch exactly roots most of the JSObject occurrences in
jsinfer{.h,inlines.h,.cpp}.  Nothing very surprising.  I left some JSObject 
fields in structs because I wasn't sure what to do with them.

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 661698 [details] [diff] [review]
(part 1) - Exactly root most JSObjects in jsinfer.cpp.

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

Because of MarkTypeObjectFlags, we need to do a bit more research before we can know whether this patch roots too aggressively or not enough.  See my comments inline.  These are some fairly subtle cases, so let me know if you have questions.

::: js/src/jsarrayinlines.h
@@ +13,5 @@
>  inline void
>  JSObject::markDenseArrayNotPacked(JSContext *cx)
>  {
>      JS_ASSERT(isDenseArray());
> +    js::RootedObject self(cx, this);

See my later comments about self.

::: js/src/jsinfer.cpp
@@ +1009,5 @@
>   */
>  static inline Type
> +GetSingletonPropertyType(JSContext *cx, RawObject rawObjArg, jsid id)
> +{
> +    RootedObject obj(cx, rawObjArg);    // root this locally because it's assigned to

Capitalize the first word and put a period at the end.

@@ +1133,5 @@
>                   * to remove the barrier after the property becomes defined,
>                   * even if no undefined value is ever observed at pc.
>                   */
> +                RootedObject singleton(cx, object->singleton);
> +                Shape *shape = GetSingletonShape(cx, singleton, id);

Might be worth making this RootedShape now, since you've already touched it.  Up to you.

@@ +1839,5 @@
>      return true;
>  }
>  
>  int
> +StackTypeSet::getTypedArrayType(JSContext *cx)

Do we GC in this method?  If you are only using cx to root proto, I think we can get away with making it a RawObject.  Perhaps the top of this method would be a good place to stick an AutoAssertNoGC?

@@ +2700,5 @@
>  
>      InferSpew(ISpewOps, "singletonTypeBarrier: #%u:%05u: %sT%p%s %p %s",
>                script->id(), pc - script->code,
>                InferSpewColor(target), target, InferSpewColorReset(),
> +              (void *) singleton.address(), TypeIdString(singletonId));

I think you want foo.get() here: foo.address() is equivalent to &fooRoot, whereas foo.get() will give you the JSObject* into the GC Heap.

@@ +3104,5 @@
>           * We don't need to handle arrays or other JIT'ed non-natives as
>           * these are not (yet) singletons.
>           */
>  
> +        RootedObject rootedSingleton(cx, singleton);

It's not entirely clear what the cutoff point is, but I think in this case - with only two uses - it would be better to use singleton in-place using fromMarkedLocation, rather than duplicating it to the stack first.

@@ +4706,5 @@
>  
>          jsbytecode *pc = script->code + uses->offset;
>          JSOp op = JSOp(*pc);
>  
> +        RootedObject obj(cx, pbaseobj);

We don't ever appear to assign to obj in this function: I think this was probably here because |(*pbaseobj)->| is significantly uglier than |obj->|.  With pbaseobj as a MutableHandleObject, you should be able to just replace all the |obj| here with |pbaseobj| rather than re-rooting: this should be clearer without adding significant ugliness.

@@ +4799,5 @@
>              StackTypeSet *funcallTypes = analysis->poppedTypes(pc, GET_ARGC(pc) + 1);
>              StackTypeSet *scriptTypes = analysis->poppedTypes(pc, GET_ARGC(pc));
>  
>              /* Need to definitely be calling Function.call on a specific script. */
> +            RootedObject funcallObj(cx, funcallTypes->getSingleton());

Only used in the test below: RawObject would be fine.

@@ +4800,5 @@
>              StackTypeSet *scriptTypes = analysis->poppedTypes(pc, GET_ARGC(pc));
>  
>              /* Need to definitely be calling Function.call on a specific script. */
> +            RootedObject funcallObj(cx, funcallTypes->getSingleton());
> +            RootedObject scriptObj(cx, scriptTypes->getSingleton());

I don't think we can GC across scriptObj's lifetime: RawObject as well.

@@ +5589,1 @@
>      RootedObject self(cx, this);

It may be outside the scope of this patch, but cases like this where we have to root |self| need to be made into static methods that take self as a HandleObject after cx: it's just too error prone to leave a potentially poisoned implicit |this| lying around.  See bug 782646 and bug 785452.

@@ +5648,5 @@
>      JS_ASSERT(cx->compartment == compartment());
>  
>      RootedObject self(cx, this);
>      JSProtoKey key = JSCLASS_CACHED_PROTO_KEY(getClass());
> +    RootedObject proto(cx, getProto());

Ah, a perfect example in the very next hunk: this should be self->getClass() and self->getProto().  It isn't a problem right now because there is nothing that can GC above them, but how long before someone shuffles the code around in a way that violates this?

@@ +5710,5 @@
>  
>  /* static */ inline HashNumber
> +TypeObjectEntry::hash(RawObject proto)
> +{
> +    return PointerHasher<RawObject, 3>::hash(proto);

Uhg.  I think this template param needs to stay as the low-level type, otherwise it will be problematic to switch RawObject to a class.

::: js/src/jsinferinlines.h
@@ +515,5 @@
>  }
>  
>  /* Set one or more dynamic flags on a type object. */
>  inline void
> +MarkTypeObjectFlags(JSContext *cx, HandleObject obj, TypeObjectFlags flags)

This one change seems to have incredibly wide sweeping impact.  TypeObject::setFlags does not obviously GC, but it doesn't obviously not GC either.  It would be nice to know Brian's intent with this method to be sure we're doing right thing here.

Could you trace this call all the way down to its leaves?  If it doesn't end up GCing anywhere, then we won't have to make the other half of JSObject's methods static.  If it does GC, this would be an excellent chokepoint to add another MaybeCheckStackRoots call.

::: js/src/jsobj.cpp
@@ +3259,5 @@
>      JS_ASSERT(newCount > oldCount);
>      JS_ASSERT(newCount >= SLOT_CAPACITY_MIN);
>      JS_ASSERT(!isDenseArray());
>  
> +    RootedObject self(cx, this);

Good catch!  This method has a rare GC in the NewReshapedObject block, then accesses slots extensively afterwards.  You should make this method static to ensure that you caught all the invalid implicit |this| accesses.

@@ +3312,5 @@
>      if (!newslots)
>          return false;  /* Leave slots at its old size. */
>  
>      bool changed = slots != newslots;
>      slots = newslots;

This should be self->slots.  There are other missing self-> as well, so just make this method static.

@@ +3332,5 @@
>  {
>      JS_ASSERT(newCount < oldCount);
>      JS_ASSERT(!isDenseArray());
>  
> +    RootedObject self(cx, this);

This method is much less pernicious than growSlots, but I think we should make it static as well.

::: js/src/jsobjinlines.h
@@ +370,5 @@
>          /*
>           * Mark the type of this object as possibly not a dense array, per the
>           * requirements of OBJECT_FLAG_NON_DENSE_ARRAY.
>           */
> +        js::RootedObject self(cx, this);

If we GC in this if block, then getElementsHeader() call below will fail because the implicit |this| is garbage.

@@ +440,5 @@
>  
>  inline void
>  JSObject::setDenseArrayElementWithType(JSContext *cx, unsigned idx, const js::Value &val)
>  {
> +    js::RootedObject self(cx, this);

Perhaps another candidate to make a static method.

@@ +448,5 @@
>  
>  inline void
>  JSObject::initDenseArrayElementWithType(JSContext *cx, unsigned idx, const js::Value &val)
>  {
> +    js::RootedObject self(cx, this);

Ditto.

@@ +930,5 @@
>  inline void
>  JSObject::nativeSetSlotWithType(JSContext *cx, js::Shape *shape, const js::Value &value)
>  {
>      nativeSetSlot(shape->slot(), value);
> +    js::RootedObject self(cx, this);

Ditto.

Comment 3

[Brian Hackett [Laid off!]]

FWIW the intent here was that we shouldn't be able to do moving collections, or any collections at all for that matter, inside an AutoEnterAnalysis / AutoEnterTypeInference / AutoEnterCompilation.  This is bug 772820, with some more rationale there.  That bug isn't done, and its motivating bug 767223 is DOA, so I don't know whether that course should still be pursued.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Exactly root most JSObjects in jsinfer.cpp.

Thanks for the detailed review!  Here's an updated, smaller patch.

You're right:  MarkTypeObjectFlags() can't GC.  I made it take a
|RawObject| and undid the relevant rooting.

I kept the |rootedSingleton| (renamed as |rSingleton|) instead of using
fromMarkedLocation(), because fromMarkedLocation() needs a const parameter.

I made a bunch of JSObject methods static to avoid rooting |this|.  Many
were needed because AddTypePropertyId() can trigger GC.  In
updateSlotsForSpan() I still root |this| but I think it's ok in that case.

Other comments have been addressed as necessary.  Happy reading.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove unnecessary |script| arg to TypeCompartment::newTypeObject().

A simple clean-up.

Comment 6

[Terrence Cole [:terrence]]

Comment on attachment 662038 [details] [diff] [review]
(part 1) - Exactly root most JSObjects in jsinfer.cpp.

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

Much better!  Only one nit, but you still have a bit more work to do for updateSlotsForSpan.

::: js/src/gc/Root.h
@@ +566,5 @@
>  
>  /*
>   * The scoped guard object AutoAssertNoGC forces the GC to assert if a GC is
>   * attempted while the guard object is live.  If you have a GC-unsafe operation
> + * to perform, use this guard object to protect your operation.

Doh!

::: js/src/jsinfer.cpp
@@ +4802,5 @@
>              /* Need to definitely be calling Function.call on a specific script. */
> +            JSFunction *function;
> +            {
> +                RawObject funcallObj = funcallTypes->getSingleton();
> +                RawObject scriptObj = scriptTypes->getSingleton();

Correct!  We need to limit the scope of Raw<T> if we want it to automatically assert NoGC ranges.

@@ +5591,1 @@
>      RootedObject self(cx, this);

Can you elaborate on why we should keep this self?

::: js/src/jsobj.cpp
@@ +3193,5 @@
>      size_t newCount = dynamicSlotsCount(numFixedSlots(), newSpan);
>  
>      if (oldSpan < newSpan) {
> +        RootedObject rThis(cx, this);
> +        if (oldCount < newCount && !JSObject::growSlots(cx, rThis, oldCount, newCount))

Eh?  If this GCs, the calls to this->initSlotUnchecked and this->initializeSlotRange below will fail.  I'll leave it up to you if you want to make this method static, but at the very least this should be |self| to match all of our other |this| roots.

@@ +3206,5 @@
>          prepareSlotRangeForOverwrite(newSpan, oldSpan);
>          invalidateSlotRange(newSpan, oldSpan - newSpan);
>  
> +        if (oldCount > newCount) {
> +            RootedObject rThis(cx, this);

|self|

@@ +3289,2 @@
>              RootedShape shape(cx, typeObj->newScript->shape);
> +            JSObject *reshapedObj = NewReshapedObject(cx, typeObj, obj->getParent(), kind, shape);

RawObject while you are here.

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 662039 [details] [diff] [review]
(part 2) - Remove unnecessary |script| arg to TypeCompartment::newTypeObject().

I am definitely not the right person for this review.

Comment 8

[Steve Fink [:sfink] [:s:]]

Comment on attachment 662039 [details] [diff] [review]
(part 2) - Remove unnecessary |script| arg to TypeCompartment::newTypeObject().

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

I was just looking at that with billm the other day. It's straightforward unused code right now, so I'm going to r+ and say bhackett has to add it back in if he comes up with a use for it again in the future.

Comment 9

[Nicholas Nethercote [inactive]]

Thanks for the fast review!


> @@ +5591,1 @@
> >      RootedObject self(cx, this);
> 
> Can you elaborate on why we should keep this self?

I didn't change any |self| cases that I didn't have to.  But I can get rid of this one.


> > +        RootedObject rThis(cx, this);
> > +        if (oldCount < newCount && !JSObject::growSlots(cx, rThis, oldCount, newCount))
> 
> Eh?  If this GCs, the calls to this->initSlotUnchecked and
> this->initializeSlotRange below will fail.  I'll leave it up to you if you
> want to make this method static, but at the very least this should be |self|
> to match all of our other |this| roots.

You're right, I'll make it static.  (I used |rThis| as the name because I figured all our existing |self| pointers are going to end up being removed anyway;  I should have followed that thought through to its logical conclusion and realized that this one would have to be removed too.)

Comment 10

[Nicholas Nethercote [inactive]]

> It's straightforward unused code right now

Yep.  I figured anyone could do the review, and I asked Terrence because he was in the neighbourhood.

Comment 11

[Nicholas Nethercote [inactive]]

I had to convert a few more JSObject methods to static because of updateSlotsForSpan().

https://hg.mozilla.org/integration/mozilla-inbound/rev/09c03f14f5f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/53c9b95ac864

Comment 12

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/09c03f14f5f5
https://hg.mozilla.org/mozilla-central/rev/53c9b95ac864

Comment 13

[Nicholas Nethercote [inactive]]

Terrence, is there anything about exact rooting that could trigger a "too much recursion" exception?  I'm hitting one on my next patch...

Comment 14

[Terrence Cole [:terrence]]

Not off hand, but I'm not at all familiar with how the recursion guards work.

Comment 15

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Exactly root most JSScripts in jsinfer.cpp.

This patch roots most |JSScript *| pointers in jsinfer.cpp.

Comment 16

[Terrence Cole [:terrence]]

Comment on attachment 663217 [details] [diff] [review]
(part 3) - Exactly root most JSScripts in jsinfer.cpp.

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

I think this looks pretty good except for one major thing and a few nits.  I'll give it a more thorough look once you've done the update below.

::: js/src/ion/IonBuilder.cpp
@@ +3932,5 @@
>      if (!templateObject)
>          return NULL;
>  
> +    RootedScript rScript(cx, script);
> +    if (types::UseNewTypeForInitializer(cx, rScript, pc, JSProto_Array)) {

Adding a new root for every access like this is going to get prohibitively expensive very fast.  Steve and I talked it over today and we cannot see any real danger in returning a Handle::fromMarkedLocation in places like this.  Please add an accessor for IonBuilder::script, like the one for JSFunction::script.  In general, I think we want to start moving all field accesses to accessor methods, many of which will probably want to be Handlified at the same time.

@@ +3981,5 @@
>      if (!templateObject)
>          return false;
>  
> +    RootedScript rScript(cx, script);
> +    if (types::UseNewTypeForInitializer(cx, rScript, pc, JSProto_Object)) {

Ditto.

::: js/src/ion/MCallOptimize.cpp
@@ +245,5 @@
>      types::StackTypeSet *thisTypes = getInlineArgTypeSet(argc, 0);
>      if (thisTypes->hasObjectFlags(cx, unhandledFlags))
>          return InliningStatus_NotInlined;
> +    RootedScript rScript(cx, script);
> +    if (types::ArrayPrototypeHasIndexedProperty(cx, rScript))

Ditto.

@@ +283,5 @@
>      types::StackTypeSet *thisTypes = getInlineArgTypeSet(argc, 0);
>      if (thisTypes->hasObjectFlags(cx, types::OBJECT_FLAG_NON_DENSE_ARRAY))
>          return InliningStatus_NotInlined;
> +    RootedScript rScript(cx, script);
> +    if (types::ArrayPrototypeHasIndexedProperty(cx, rScript))

Ditto.

::: js/src/ion/TypeOracle.cpp
@@ +468,5 @@
>  bool
>  TypeInferenceOracle::arrayPrototypeHasIndexedProperty()
>  {
> +    RootedScript rScript(cx, script);
> +    return ArrayPrototypeHasIndexedProperty(cx, rScript);

Ditto.

::: js/src/jscompartment.cpp
@@ +601,5 @@
>          if (types.inferenceEnabled) {
>              gcstats::AutoPhase ap2(rt->gcStats, gcstats::PHASE_DISCARD_TI);
>  
>              for (CellIterUnderGC i(this, FINALIZE_SCRIPT); !i.done(); i.next()) {
> +                RootedScript script(rt, i.get<JSScript>());

!?! We're in the middle of GC here: this should be RawScript.  This is actually a good argument for removing the |rt| constructors as well as the default constructors....

::: js/src/jsinfer.cpp
@@ +517,5 @@
>  template <PropertyAccessKind access>
>  class TypeConstraintProp : public TypeConstraint
>  {
>  public:
> +    JSScript *const script;

Because of the GC, const is almost always a lie for Cells, so I'm almost always against adding const to Cells in SpiderMonkey.  It isn't a problem in this particular case, but I'm still against it since it's only going to make this code harder for everyone else to interface with later.  Or is this doing something useful right now that I'm not seeing?

@@ +1158,5 @@
>  template <PropertyAccessKind access>
>  void
>  TypeConstraintProp<access>::newType(JSContext *cx, TypeSet *source, Type type)
>  {
> +    RootedScript rScript(cx, script);

Accessor.

@@ +1169,5 @@
> +        if (access == PROPERTY_WRITE) {
> +            cx->compartment->types.monitorBytecode(cx, rScript, pc - script->code);
> +        } else {
> +            MarkPropertyAccessUnknown(cx, rScript, pc, target);
> +        }

No {}'s here.

@@ +1194,5 @@
>  template <PropertyAccessKind access>
>  void
>  TypeConstraintCallProp<access>::newType(JSContext *cx, TypeSet *source, Type type)
>  {
> +    RootedScript rScript(cx, script);

Accessor.

@@ +1233,5 @@
>  
>  void
>  TypeConstraintSetElement::newType(JSContext *cx, TypeSet *source, Type type)
>  {
> +    RootedScript rScript(cx, script);

Accessor.

@@ +1375,5 @@
>           * possible this/callee correlations. This only comes into play for
>           * CALLPROP, for other calls we are past the type barrier and a
>           * TypeConstraintCall will also monitor the call.
>           */
> +        RootedScript rScript(cx, script);

Accessor.

@@ +1419,5 @@
>       * 1. Operations producing a double where no operand was a double.
>       * 2. Operations producing a string where no operand was a string (addition only).
>       * 3. Operations producing a value other than int/double/string.
>       */
> +    RootedScript rScript(cx, script);

Accessor.

@@ +1468,5 @@
>          target->addType(cx, type);
>          return;
>      }
>  
> +    RootedScript rScript(cx, script);

I'm just going to stop here.  I assume I'll get to read all of this again on the next pass anyway.

Comment 17

[Nicholas Nethercote [inactive]]

> Because of the GC, const is almost always a lie for Cells, so I'm almost
> always against adding const to Cells in SpiderMonkey.  It isn't a problem in
> this particular case, but I'm still against it since it's only going to make
> this code harder for everyone else to interface with later.  Or is this
> doing something useful right now that I'm not seeing?

The specific motivation was that in this patch in several places I do |RootedScript rScript(cx, this->script);| and then use |rScript| more than once.  Which leaves me wondering, is this safe, or is it possible that |this->script| will change in the meantime, leaving |rScript| stale?  If |this->script| is const, then you know that reusing |rScript| is safe.

And the general motivation is just that const members are nicer than non-const.  But it's a good point about the movement, so I'll un-const these.  Presumably all these fields will end up as HeapPtrs or something else like that.

Comment 18

[Nicholas Nethercote [inactive]]

> Steve and I talked it over today and we cannot see any
> real danger in returning a Handle::fromMarkedLocation in places like this. 

Is it safe because |this| is guaranteed to outlive the returned Handle when we are inside a class method?

Comment 19

[Nicholas Nethercote [inactive]]

> Is it safe because |this| is guaranteed to outlive the returned Handle when
> we are inside a class method?

In which case it's only safe if script() is a private method, is that right?  Hmm.

Comment 20

[Nicholas Nethercote [inactive]]

Attachment: (part 3b) - address comments.

Follow-ups to address your comments on part 3;  I've put them in a separate
patch, though I'll probably fold them together before landing.

Comment 21

[Terrence Cole [:terrence]]

Comment on attachment 664338 [details] [diff] [review]
(part 3b) - address comments.

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

Whew!  Thanks for making this change, it think takes us a good chunk of the way closer to a consistent style between IM and SM.  I hope you were able to script most of it, because that's a lot more |script_| than I was expecting to find.

::: js/src/ion/IonBuilder.cpp
@@ +38,3 @@
>      lazyArguments_(NULL)
>  {
> +    script_.init(info->script()),

Comma?  Otherwise, perfect.

@@ +145,5 @@
>  
>  JSFunction *
>  IonBuilder::getSingleCallTarget(uint32 argc, jsbytecode *pc)
>  {
> +    types::StackTypeSet *calleeTypes = oracle->getCallTarget(script(), argc, pc);

You used script_ below, so it's probably okay to use it directly here too?

::: js/src/jsinfer.cpp
@@ +6218,5 @@
>          js_delete(allocationSiteTable);
>  }
>  
>  /* static */ void
> +TypeScript::Sweep(FreeOp *fop, RawScript script)

Good catch!

Comment 22

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/978c67b9efa1

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/978c67b9efa1