Last Comment Bug 718570 - GC: Mark stack refactoring
: GC: Mark stack refactoring
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Bill McCloskey (:billm)
:
Mentors:
Depends on:
Blocks: IncrementalGC
  Show dependency treegraph
 
Reported: 2012-01-17 00:02 PST by Bill McCloskey (:billm)
Modified: 2012-02-23 09:50 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (53.20 KB, patch)
2012-01-17 00:02 PST, Bill McCloskey (:billm)
igor: review+
Details | Diff | Splinter Review

Description Bill McCloskey (:billm) 2012-01-17 00:02:18 PST
Created attachment 589117 [details] [diff] [review]
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 2012-01-18 02:27:08 PST
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 (:billm) 2012-02-23 09:50:40 PST
This landed along with incremental GC.

Note You need to log in before you can comment on or make changes to this bug.