718570 - GC: Mark stack refactoring

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: patch

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.

Comment 1

[Igor Bukanov]

Comment on attachment 589117 [details] [diff] [review]
patch

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().

Comment 2

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

This landed along with incremental GC.