Closed
Bug 727135
Opened 12 years ago
Closed 12 years ago
GC: indirect ID marking
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: terrence, Assigned: terrence)
References
Details
Attachments
(1 file, 1 obsolete file)
18.43 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
(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•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ece583b83508
Comment 5•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ece583b83508
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in
before you can comment on or make changes to this bug.
Description
•