727135 - GC: indirect ID marking

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: v0: No surprises here.

This makes the marking interfaces for jsids and HeapIds take pointers to jsids and HeapIds.

Comment 1

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

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]]

Attachment: 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 [inactive unless it's an emergency] (:billm)]

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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ece583b83508

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ece583b83508