The default bug view has changed. See this FAQ.

GC: indirect ID marking

RESOLVED FIXED in mozilla13

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
Attachment #597073 - Flags: review?(wmccloskey)
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.
Attachment #597073 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 2

5 years ago
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?
Attachment #597073 - Attachment is obsolete: true
Attachment #598063 - Flags: review?(wmccloskey)
Attachment #598063 - Flags: review?(wmccloskey) → review+
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.
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ece583b83508
https://hg.mozilla.org/mozilla-central/rev/ece583b83508
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.