Last Comment Bug 726845 - GC: indirect Value marking
: GC: indirect Value marking
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Terrence Cole [:terrence]
:
Mentors:
Depends on: 721463
Blocks: 720522
  Show dependency treegraph
 
Reported: 2012-02-13 16:19 PST by Terrence Cole [:terrence]
Modified: 2012-02-17 05:25 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v0 (34.59 KB, patch)
2012-02-13 16:19 PST, Terrence Cole [:terrence]
wmccloskey: review+
Details | Diff | Splinter Review
final (33.29 KB, patch)
2012-02-16 10:55 PST, Terrence Cole [:terrence]
terrence: review+
Details | Diff | Splinter Review

Description Terrence Cole [:terrence] 2012-02-13 16:19:59 PST
Created attachment 596840 [details] [diff] [review]
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 (:billm) 2012-02-15 16:15:27 PST
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] 2012-02-16 10:55:31 PST
Created attachment 597897 [details] [diff] [review]
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] 2012-02-16 10:56:21 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f6f9de44583
Comment 4 Ed Morley [:emorley] 2012-02-17 05:25:16 PST
https://hg.mozilla.org/mozilla-central/rev/6f6f9de44583

Note You need to log in before you can comment on or make changes to this bug.