Last Comment Bug 727135 - GC: indirect ID marking
: GC: indirect ID 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-14 10:12 PST by Terrence Cole [:terrence]
Modified: 2012-02-18 09:56 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v0: No surprises here. (10.19 KB, patch)
2012-02-14 10:12 PST, Terrence Cole [:terrence]
wmccloskey: review+
Details | Diff | Review
v1: Updated with review feedback. (18.43 KB, patch)
2012-02-16 16:51 PST, Terrence Cole [:terrence]
wmccloskey: review+
Details | Diff | Review

Description Terrence Cole [:terrence] 2012-02-14 10:12:54 PST
Created attachment 597073 [details] [diff] [review]
v0: No surprises here.

This makes the marking interfaces for jsids and HeapIds take pointers to jsids and HeapIds.
Comment 1 Bill McCloskey (:billm) 2012-02-15 16:28:12 PST
Comment on attachment 597073 [details] [diff] [review]
v0: No surprises here.

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

::: js/src/jsgcmark.cpp
@@ +257,5 @@
>  {
> +    if (JSID_IS_STRING(*id)) {
> +        JSString *str = JSID_TO_STRING(*id);
> +        MarkInternal(trc, str);
> +        JS_ASSERT(str == JSID_TO_STRING(*id));

This seems unnecessary, since MarkInternal already asserts this. Why not prepare for the future and say *id = INTERNED_STRING_TO_JSID(str) ? Same below.

::: js/src/jsscope.h
@@ +771,5 @@
>      }
>  
>      jsid propid() const { JS_ASSERT(!isEmptyShape()); return maybePropid(); }
>      const HeapId &maybePropid() const { JS_ASSERT(!JSID_IS_VOID(propid_)); return propid_; }
> +    HeapId &propidRef() { return propid_; }

Can we just kill maybePropid? It doesn't appear to have many users outside the GC.
Comment 2 Terrence Cole [:terrence] 2012-02-16 16:51:26 PST
Created attachment 598063 [details] [diff] [review]
v1: Updated with review feedback.

(In reply to Bill McCloskey (:billm) from comment #1)
> ::: js/src/jsscope.h
> @@ +771,5 @@
> >      }
> >  
> >      jsid propid() const { JS_ASSERT(!isEmptyShape()); return maybePropid(); }
> >      const HeapId &maybePropid() const { JS_ASSERT(!JSID_IS_VOID(propid_)); return propid_; }
> > +    HeapId &propidRef() { return propid_; }
> 
> Can we just kill maybePropid? It doesn't appear to have many users outside
> the GC.

I actually tried that when making the first patch :-).  I didn't pursue it far though because it looked fairly complex.  I've tried harder now though, and I think it worked out quite well.  However, it roughly doubled the size of the code changes, so I'm asking for re-review.

First, I made the HeapPtr<const Shape> to HeapPtrShape -- this makes it easier to use the non-const propidRef in several places, and lets us remove const from all the shape marking stuff.

Second, I used one awkward const_cast in the StackShape constructor.  Removing the const looked like it would propagate back quite a ways and this seems like an appropriate (if weird) use for it.  I don't think it's safe to use the propid() const version because of the !isEmptyShape assertion (which I think we do want on most propid() uses).

Also, why is HeapId non-copyable?
Comment 3 Bill McCloskey (:billm) 2012-02-17 15:41:41 PST
Oh, I forgot the question. You can make HeapId copyable if you need it. For a while I was worried about write barriers being invoked too often, and I tried to avoid that by removing copy constructors. But the fear was never borne out, and I eventually wrote a copy constructor for HeapPtr (I think). HeapId never got one, but that shouldn't stop you.
Comment 4 Terrence Cole [:terrence] 2012-02-17 18:22:05 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/ece583b83508
Comment 5 Ed Morley [:emorley] 2012-02-18 09:56:42 PST
https://hg.mozilla.org/mozilla-central/rev/ece583b83508

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