Closed Bug 718570 Opened 10 years ago Closed 9 years ago

GC: Mark stack refactoring


(Core :: JavaScript Engine, defect)

Not set





(Reporter: billm, Assigned: billm)




(1 file)

Attached patch patchSplinter Review
This patch makes some changes to the mark stack to prepare for incremental GC. Namely:
- It adds support for gray root buffering
- It stores the mark stack persistently in the JSRuntime
- It handles resetting the mark stack, in case an incremental GC fails

I also took the opportunity to make some additional changes:
- Removed the conservative root dumping code. If we ever want to study this, we can just add printfs in the right places. It's silly to carry around all that complexity for something that's never used.
- Made more mark stack methods private.
- Made PushValueArray be a method of GCMarker

This is basically the patch originally posted in bug 641025. It incorporates the review comments, including the one to move gray marking in GCMarker. I didn't use the suggestion to make the mark stack be a Vector. That's a fairly significant change, and I would prefer to do it after incremental GC.
Attachment #589117 - Flags: review?(igor)
Comment on attachment 589117 [details] [diff] [review]

Review of attachment 589117 [details] [diff] [review]:

Thanks for removing the conservative root dumping! That code at one point was useful to debug the conservative scanner, but now it just unnecessary complicates things.

::: js/src/jsgc.h
@@ +1782,3 @@
> +    void start(JSRuntime *rt, JSContext *cx);
> +    void finish();

"finish" as a name is typically used in SM to denote cleanup code or explicit destructors. I suppose "stop" can be used here.

@@ +1821,5 @@
>      bool hasDelayedChildren() const {
>          return !!unmarkedArenaStackTop;
>      }
> +    bool isFinished() {

This name is not good even if "finished" would be OK as between start and "finish" the marker can transition into "isFinished" state multiple times due to weak tables etc. IMO a function with reverse semantics like hasThingsToMark would be better even if in most cases it will be used as !hasThingsToMark() as a reader can immediately get its meaning. But if that is too long, then use something like isDrained().
Attachment #589117 - Flags: review?(igor) → review+
This landed along with incremental GC.
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.