726845 - GC: indirect Value marking

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: v0

This patch reworks the Value marking interfaces to take Value* and HeapValue*.

This appears to basically work and not make anything dramatically more ugly.  The only question I have is what to do about IncrementalValueBarrier.  For now I've updated it to take a pointer as well.

Comment 1

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

Comment on attachment 596840 [details] [diff] [review]
v0

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

Looks good.

::: js/src/builtin/MapObject.cpp
@@ +185,5 @@
>      MapObject *mapobj = static_cast<MapObject *>(obj);
>      if (ValueMap *map = mapobj->getData()) {
>          for (ValueMap::Range r = map->all(); !r.empty(); r.popFront()) {
> +            const HeapValue &key = r.front().key;
> +            HeapValue orig(key);

Why do you need orig here? Can't you just use key? Same question below.

::: js/src/gc/Barrier-inl.h
@@ +114,5 @@
>      post(comp);
>  }
>  
>  inline void
> +HeapValue::writeBarrierPre(Value *value)

Why change this? We don't ever expect an incremental barrier to move anything.

::: js/src/jscntxt.h
@@ +1132,5 @@
>          JS_ASSERT(link);
>          return reinterpret_cast<JSContext *>(uintptr_t(link) - offsetof(JSContext, link));
>      }
>  
> +    void mark(JSTracer *trc);

There should be an empty line before private:.

::: js/src/jsexn.cpp
@@ +432,5 @@
>              vcount += elem->argc;
>          }
>          vp = GetStackTraceValueBuffer(priv);
>          for (i = 0; i != vcount; ++i, ++vp) {
> +            MarkValue(trc, vp, "stack trace argument");

Please remove the braces while you're here.

::: js/src/jsfriendapi.cpp
@@ +577,5 @@
>          JS_NOT_REACHED("invalid trace kind");
>  }
>  
>  extern JS_FRIEND_API(void)
> +IncrementalValueBarrier(Value *v)

Again, I don't think we need to change this.

Comment 2

[Terrence Cole [:terrence]]

Attachment: final

(In reply to Bill McCloskey (:billm) from comment #1)
> ::: js/src/builtin/MapObject.cpp
> @@ +185,5 @@
> >      MapObject *mapobj = static_cast<MapObject *>(obj);
> >      if (ValueMap *map = mapobj->getData()) {
> >          for (ValueMap::Range r = map->all(); !r.empty(); r.popFront()) {
> > +            const HeapValue &key = r.front().key;
> > +            HeapValue orig(key);
> 
> Why do you need orig here? Can't you just use key? Same question below.

You are quite right.  It took me quite awhile to figure out how to get to the HeapValue out of the HashableValue at all: I must have forgotten to remove the intermediary I was using.

> ::: js/src/gc/Barrier-inl.h
> @@ +114,5 @@
> >      post(comp);
> >  }
> >  
> >  inline void
> > +HeapValue::writeBarrierPre(Value *value)
> 
> Why change this? We don't ever expect an incremental barrier to move
> anything.

I'll definitely undo these.  My thinking was that it might be useful in the nebulous future if we want to have an incremental moving collector.  Ignoring the fact that this is bonkers on its face, it turns out that it also makes for an annoyingly large amount of code motion for the Unbarriered markers.  This will fit better in a different bug anyway, if we do want to do it later.

Comment 3

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f6f9de44583

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6f6f9de44583