GC: add type safety for Shape

RESOLVED FIXED in mozilla20

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
The patch that adds the Unrooted type handles BaseShape and UnownedBaseShape.  Shape is quite a bit harder because the existing division of the code does not follow GC boundaries well at all.  I'll be attaching a patch as soon as I get a green try run that handles about 80% of the un-typed Shape*.  Unfortunately, the last 20% is going to take a more significant reworking of jsscope and jspropertyfoo, so is outside the purview of this specific bug.
(Assignee)

Comment 1

6 years ago
Created attachment 688404 [details] [diff] [review]
v0

This is very long, but it's almost entirely made of trivial changes.  With this patch applied, we are down to 77 Shape*, the vast majority of which are property tree and search's use of Shape** and lastProperty's return, which flows into hundreds of lines of changes in the methodjit.
Attachment #688404 - Flags: review?(sphink)
(Assignee)

Updated

6 years ago
Depends on: 817091
Comment on attachment 688404 [details] [diff] [review]
v0

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

::: js/src/ion/shared/Assembler-shared.h
@@ +98,5 @@
>  
>      explicit ImmGCPtr(const gc::Cell *ptr) : value(reinterpret_cast<uintptr_t>(ptr))
>      { }
> +
> +    // ImmGCPtr is rooted so we can convert safely directly from Unrooted<T>.

It gives me the creepy-crawlies, but ok.

::: js/src/jsapi.cpp
@@ +4552,5 @@
>  /*
>   * XXX reverse iterator for properties, unreverse and meld with jsinterp.c's
>   *     prop_iterator_class somehow...
>   * + preserve the obj->enumerate API while optimizing the native object case
> + * + native case here uses a RawShape, but that iterates in reverse!

Hm. I suppose this could be argued, but I think the original was probably better because it doesn't imply anything about the rootednes. (Yes, I know this is probably just search-and-replace.)

::: js/src/jsdbgapi.cpp
@@ +900,5 @@
>          if (!js_AddRoot(cx, &pd[i].id, NULL))
>              goto bad;
>          if (!js_AddRoot(cx, &pd[i].value, NULL))
>              goto bad;
> +        RootedShape shape(cx, const_cast<Shape *>(&r.front()));

The scary mean rooting progress script is going to see this...

::: js/src/jsinfer.cpp
@@ +814,3 @@
>      if (!obj->isNative())
> +        return UnrootedShape(NULL);
> +    UnrootedShape shape = DropUnrooted(obj)->nativeLookup(cx, DropUnrooted(id));

Wow, that's funky. I wonder if there's another name for DropUnrooted; I had to look it up to figure out what it does. EndNoGCScope(obj)?

@@ +3197,5 @@
>  
>  static inline void
> +UpdatePropertyType(JSContext *cx, TypeSet *types, RawObject obj, UnrootedShape shape, bool force)
> +{
> +    AutoAssertNoGC nogc;

Isn't the AutoAssertNoGC redundant with the Unrooted parameter?

@@ +5829,5 @@
>  
>  TypeObject *
>  JSCompartment::getNewType(JSContext *cx, TaggedProto proto_, JSFunction *fun_, bool isDOM)
>  {
> +    AssertCanGC();

Fairly obvious, but I guess it's fine.

::: js/src/jsinterpinlines.h
@@ +181,5 @@
>          /* Fast path for Object instance properties. */
>          JS_ASSERT(shape->hasSlot());
>          vp.set(pobj->nativeGetSlot(shape->slot()));
>      } else {
> +        RootedShape shape_(cx, shape);

Haven't we been using RootedShape shape(cx, shapeArg)?

::: js/src/jsiter.cpp
@@ +655,1 @@
>                  RawObject pobj = obj;

Couldn't these 2 lines be |UnrootedObject pobj(obj);|?

::: js/src/jsobj.cpp
@@ +1908,3 @@
>                  return false;
> +
> +            last = next;

Why is the extra variable needed? Couldn't you directly assign into last?

@@ +2569,5 @@
>          if ((attrs & JSPROP_GETTER) && !cx->compartment->wrap(cx, &getter))
>              return false;
>          if ((attrs & JSPROP_SETTER) && !cx->compartment->wrap(cx, &setter))
>              return false;
> +        v = shape->hasSlot() ? obj->getSlot(shape->slot()) : UndefinedValue();

Ouch. Much better.

@@ +2967,5 @@
>          JS_ASSERT(obj->isGlobal());
>          JS_ASSERT(obj->isNative());
>  
> +        bool hasProp = bool(obj->nativeLookup(cx, id));
> +        if (!hasProp) {

I'd be fine with |if (!obj->nativeLookup(cx, id))|, but whatever.

@@ +3778,5 @@
>          /* Lookups will not be cached through non-native protos. */
>          if (!obj->isNative())
>              break;
>  
> +        RootedShape shape(cx, obj->nativeLookup(cx, id));

Hoist the root above the loop

@@ +4212,5 @@
>      }
>  
>      Rooted<Shape*> shapeRoot(cx, shape);
>      RootedObject pobjRoot(cx, pobj);
> +    RootedValue nvp(cx, vp);

I'm missing something here... why are any of these 3 needed?

@@ +5304,5 @@
>  {
>      jsid id = shape.propid();
>      uint8_t attrs = shape.attributes();
>  
> +    fprintf(stderr, "    ((RawShape) %p) ", (void *) &shape);

Hm... that's kind of ugly. I'd rather add that to the progress script, alongside the "within comments" exception.

::: js/src/jsobj.h
@@ +757,5 @@
> +    static js::UnrootedShape addPropertyInternal(JSContext *cx,
> +                                                 JS::HandleObject obj, JS::HandleId id,
> +                                                 JSPropertyOp getter, JSStrictPropertyOp setter,
> +                                                 uint32_t slot, unsigned attrs,
> +                                                 unsigned flags, int shortid, js::Shape **spp,

Other places you've switched Shape** to RawShape*.

@@ +773,5 @@
>  
>    public:
>      /* Add a property whose id is not yet in this scope. */
> +    static js::UnrootedShape addProperty(JSContext *cx,
> +                                         JS::HandleObject, JS::HandleId id,

nit: Put JSContext* and the 'self' HandleObject on the same line.

@@ +782,3 @@
>  
>      /* Add a data property whose id is not yet in this scope. */
> +    js::UnrootedShape addDataProperty(JSContext *cx, jsid id_, uint32_t slot, unsigned attrs) {

Seems odd to static-ify addProperty but not addDataProperty right next to it.

::: js/src/jspropertycacheinlines.h
@@ +65,5 @@
>                                PropertyCacheEntry **entryp, JSObject **obj2p, PropertyName **namep)
>  {
>      JS_ASSERT(this == &cx->propertyCache());
>  
> +    UnrootedShape kshape = obj->lastProperty();

nit: ::test and ::testForSet are inconsistent -- either use both AutoAssertNoGC + Unrooted or just Unrooted for both.

::: js/src/jspropertytree.cpp
@@ +37,3 @@
>      if (!shape) {
>          JS_ReportOutOfMemory(cx);
> +        return UnrootedShape(NULL);

would this look cleaner as

  if (!shape)
    JS_ReportOutOfMemory(cx);
  return shape;

?

::: js/src/jspropertytree.h
@@ +83,5 @@
>      {
>      }
>  
> +    UnrootedShape newShape(JSContext *cx);
> +    UnrootedShape getChild(JSContext *cx, Shape *parent, uint32_t nfixed, const StackShape &child);

Is it painful to make |parent| a Handle?

::: js/src/jsscope.cpp
@@ +539,4 @@
>  
>          if (table) {
>              /* Store the tree node pointer in the table entry for id. */
> +            SHAPE_STORE_PRESERVING_COLLISION(spp, static_cast<RawShape>(shape));

I keep thinking that there's something more clever we should be doing for these tables, but I don't want to think about it yet.

::: js/src/vm/ObjectImpl.cpp
@@ +262,4 @@
>  {
>      MOZ_ASSERT(isNative());
>      Shape **spp;
> +    RootedId id(cx, id_);

s/id_/idArg/
Attachment #688404 - Flags: review?(sphink) → review+
(Assignee)

Comment 3

6 years ago
(In reply to Steve Fink [:sfink] from comment #2)
> Comment on attachment 688404 [details] [diff] [review]
> ::: js/src/ion/shared/Assembler-shared.h
> @@ +98,5 @@
> >  
> >      explicit ImmGCPtr(const gc::Cell *ptr) : value(reinterpret_cast<uintptr_t>(ptr))
> >      { }
> > +
> > +    // ImmGCPtr is rooted so we can convert safely directly from Unrooted<T>.
> 
> It gives me the creepy-crawlies, but ok.

Justifiably so! ImmGCPtrs get shoved into the middle of the assembly buffer, which we subsequently send to the processor to execute. Marking of these pointers has enough magic in it already that I don't feel too bad about casting aside this assertion.
 
> ::: js/src/jsdbgapi.cpp
> @@ +900,5 @@
> >          if (!js_AddRoot(cx, &pd[i].id, NULL))
> >              goto bad;
> >          if (!js_AddRoot(cx, &pd[i].value, NULL))
> >              goto bad;
> > +        RootedShape shape(cx, const_cast<Shape *>(&r.front()));
> 
> The scary mean rooting progress script is going to see this...

I specifically made this a Shape* so we would remember to revisit it, given that I didn't fix any of the underlying rooting issues.
 
> ::: js/src/jsinfer.cpp
> @@ +814,3 @@
> >      if (!obj->isNative())
> > +        return UnrootedShape(NULL);
> > +    UnrootedShape shape = DropUnrooted(obj)->nativeLookup(cx, DropUnrooted(id));
> 
> Wow, that's funky. I wonder if there's another name for DropUnrooted; I had
> to look it up to figure out what it does. EndNoGCScope(obj)?

Remind me to ask for suggestions in Monday's mailing.
 
> @@ +3197,5 @@
> >  
> >  static inline void
> > +UpdatePropertyType(JSContext *cx, TypeSet *types, RawObject obj, UnrootedShape shape, bool force)
> > +{
> > +    AutoAssertNoGC nogc;
> 
> Isn't the AutoAssertNoGC redundant with the Unrooted parameter?

Yes indeed! I tried to remove the ones that were staying around exclusively to support Return::get(nogc), but I don't doubt I missed a few of them.

> ::: js/src/jsinterpinlines.h
> @@ +181,5 @@
> >          /* Fast path for Object instance properties. */
> >          JS_ASSERT(shape->hasSlot());
> >          vp.set(pobj->nativeGetSlot(shape->slot()));
> >      } else {
> > +        RootedShape shape_(cx, shape);
> 
> Haven't we been using RootedShape shape(cx, shapeArg)?

Yeah, this is one of the annoying functions that needs to stay Raw on one path.  I'll fix the names at least.
 
> ::: js/src/jsiter.cpp
> @@ +655,1 @@
> >                  RawObject pobj = obj;
> 
> Couldn't these 2 lines be |UnrootedObject pobj(obj);|?

I'd need to ForwardDeclareJS(Object) somewhere and I didn't want to take the time and bother to figure out where best to do that.

> ::: js/src/jsobj.cpp
> @@ +1908,3 @@
> >                  return false;
> > +
> > +            last = next;
> 
> Why is the extra variable needed? Couldn't you directly assign into last?

/me scratches head

> @@ +2967,5 @@
> >          JS_ASSERT(obj->isGlobal());
> >          JS_ASSERT(obj->isNative());
> >  
> > +        bool hasProp = bool(obj->nativeLookup(cx, id));
> > +        if (!hasProp) {
> 
> I'd be fine with |if (!obj->nativeLookup(cx, id))|, but whatever.

Nah, I should have just written it like that.
 
> @@ +4212,5 @@
> >      }
> >  
> >      Rooted<Shape*> shapeRoot(cx, shape);
> >      RootedObject pobjRoot(cx, pobj);
> > +    RootedValue nvp(cx, vp);
> 
> I'm missing something here... why are any of these 3 needed?

|shape| and |pobj| need to stay alive across the possible GC in |->get(...)|. |nvp| was added because I changed it to use a MutableHandleValue to protect against the same GC.

> @@ +5304,5 @@
> >  {
> >      jsid id = shape.propid();
> >      uint8_t attrs = shape.attributes();
> >  
> > +    fprintf(stderr, "    ((RawShape) %p) ", (void *) &shape);
> 
> Hm... that's kind of ugly. I'd rather add that to the progress script,
> alongside the "within comments" exception.

Hmmm, fair enough.
 
> ::: js/src/jsobj.h
> @@ +757,5 @@
> > +    static js::UnrootedShape addPropertyInternal(JSContext *cx,
> > +                                                 JS::HandleObject obj, JS::HandleId id,
> > +                                                 JSPropertyOp getter, JSStrictPropertyOp setter,
> > +                                                 uint32_t slot, unsigned attrs,
> > +                                                 unsigned flags, int shortid, js::Shape **spp,
> 
> Other places you've switched Shape** to RawShape*.

Hmm... /me goes to grep.  Most all of the Shape** are abstractly a "reference to a place in the property tree or property cache".  These should get split up into at least 2 distinct types when we rewrite the Shape stuff.  It looks like I /did/ convert jsiter's shape_array to RawShape*.  On reflection, this should probably also get a distinct type, so I'll back it out for now.
 
> @@ +782,3 @@
> >  
> >      /* Add a data property whose id is not yet in this scope. */
> > +    js::UnrootedShape addDataProperty(JSContext *cx, jsid id_, uint32_t slot, unsigned attrs) {
> 
> Seems odd to static-ify addProperty but not addDataProperty right next to it.

addDataProperty is /way/ more difficult because of its extensive use to set up properties on our globals: e.g. most of the |id| args are pulled out of the atoms list with NameToId.  In this case I think it actually makes more sense to re-root internally.

> ::: js/src/jspropertytree.h
> @@ +83,5 @@
> >      {
> >      }
> >  
> > +    UnrootedShape newShape(JSContext *cx);
> > +    UnrootedShape getChild(JSContext *cx, Shape *parent, uint32_t nfixed, const StackShape &child);
> 
> Is it painful to make |parent| a Handle?

Partially that, but mostly just a performance concern: |parent| is always a |shape->parent| (a HeapPtr) and the property cache usually works, so we usually end up not needing to root parent at all.  I left it as a Shape* so we would remember to revisit this later.
 
> ::: js/src/jsscope.cpp
> @@ +539,4 @@
> >  
> >          if (table) {
> >              /* Store the tree node pointer in the table entry for id. */
> > +            SHAPE_STORE_PRESERVING_COLLISION(spp, static_cast<RawShape>(shape));
> 
> I keep thinking that there's something more clever we should be doing for
> these tables, but I don't want to think about it yet.

10' pole.  We'll need to revisit these eventually, but for now trust me on this one: 10' pole.
 
> ::: js/src/vm/ObjectImpl.cpp
> @@ +262,4 @@
> >  {
> >      MOZ_ASSERT(isNative());
> >      Shape **spp;
> > +    RootedId id(cx, id_);
> 
> s/id_/idArg/

Fixed.  I didn't make it static because it has the same problem as addDataProperty.
(In reply to Terrence Cole [:terrence] from comment #3)
> (In reply to Steve Fink [:sfink] from comment #2)
> > @@ +4212,5 @@
> > >      }
> > >  
> > >      Rooted<Shape*> shapeRoot(cx, shape);
> > >      RootedObject pobjRoot(cx, pobj);
> > > +    RootedValue nvp(cx, vp);
> > 
> > I'm missing something here... why are any of these 3 needed?
> 
> |shape| and |pobj| need to stay alive across the possible GC in
> |->get(...)|. |nvp| was added because I changed it to use a
> MutableHandleValue to protect against the same GC.

shape and pobj are both Handles already. nvp is a MutableHandleValue already, so why do you need to create a new RootedValue and copy it back?
(Assignee)

Comment 5

6 years ago
(In reply to Steve Fink [:sfink] from comment #4)
> (In reply to Terrence Cole [:terrence] from comment #3)
> > (In reply to Steve Fink [:sfink] from comment #2)
> > > @@ +4212,5 @@
> > > >      }
> > > >  
> > > >      Rooted<Shape*> shapeRoot(cx, shape);
> > > >      RootedObject pobjRoot(cx, pobj);
> > > > +    RootedValue nvp(cx, vp);
> > > 
> > > I'm missing something here... why are any of these 3 needed?
> > 
> > |shape| and |pobj| need to stay alive across the possible GC in
> > |->get(...)|. |nvp| was added because I changed it to use a
> > MutableHandleValue to protect against the same GC.
> 
> shape and pobj are both Handles already. nvp is a MutableHandleValue
> already, so why do you need to create a new RootedValue and copy it back?

*blink*
https://hg.mozilla.org/mozilla-central/rev/a41d57f01020
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.