Last Comment Bug 779897 - GC: Use arenaListsToSweep to queue arenas for background as well as foreground sweeping
: GC: Use arenaListsToSweep to queue arenas for background as well as foregroun...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: mozilla17
Assigned To: Jon Coppeard (:jonco)
:
:
Mentors:
Depends on:
Blocks: 782993
  Show dependency treegraph
 
Reported: 2012-08-02 09:55 PDT by Jon Coppeard (:jonco)
Modified: 2012-08-21 19:11 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed fix (12.87 KB, patch)
2012-08-02 10:00 PDT, Jon Coppeard (:jonco)
no flags Details | Diff | Splinter Review
Part 1 - put collecting compartments on a linked list (11.74 KB, patch)
2012-08-15 03:07 PDT, Jon Coppeard (:jonco)
no flags Details | Diff | Splinter Review
Part 2 - use areanaListsToSweep for background sweeping too (15.31 KB, patch)
2012-08-15 03:08 PDT, Jon Coppeard (:jonco)
no flags Details | Diff | Splinter Review
Part 1 - put collecting compartments on a linked list v2 (11.74 KB, patch)
2012-08-15 09:25 PDT, Jon Coppeard (:jonco)
no flags Details | Diff | Splinter Review
Part 2 - use areanaListsToSweep for background sweeping too v2 (16.00 KB, patch)
2012-08-15 09:26 PDT, Jon Coppeard (:jonco)
no flags Details | Diff | Splinter Review
Proposed fix v3 (16.99 KB, patch)
2012-08-17 08:04 PDT, Jon Coppeard (:jonco)
wmccloskey: review+
Details | Diff | Splinter Review

Description Jon Coppeard (:jonco) 2012-08-02 09:55:22 PDT
This is a tidyup that allows us to get rid of the GC helper thread's finalizeVector that is currently dynamically allocated during GC.  It uses the entries of areanaListsToSweep that are not used for queuing foreground things.
Comment 1 Jon Coppeard (:jonco) 2012-08-02 10:00:25 PDT
Created attachment 648372 [details] [diff] [review]
Proposed fix

I *think* the interactions between the threads are all safe.  I found I had to wait for the background thread to finish in NewCompartment however, which may be an issue.  If this is an problem, there may be another way around it.
Comment 2 Terrence Cole [:terrence] 2012-08-02 10:49:31 PDT
It looks like a nice cleanup to me, but I'm not that familiar with this code.

Brian and I have a plan to at some point replace the fiddly inter-thread state-keeping of our ever-growing number of threads with a work-stealing-queue (i.e. message-passing).  The idea is to make every background thread generic so that it could process any needed workload with just the information passed in the message.  (This would, of course, include a completion api to let us also synchronize foreground code against any specific message.)  The goal is to be able to seamlessly scale up or down the amount of background work we do based on CPU availability by just adding and removing threads.

For the specific case of the GC's background freeing, I would think that the message would consist of a range of addresses in the arenaListsToSweep.  I'd appreciate your input on how good/terrible an idea this is.
Comment 3 Bill McCloskey (:billm) 2012-08-02 15:48:01 PDT
Comment on attachment 648372 [details] [diff] [review]
Proposed fix

This is a nice cleanup, but the need to wait for the background thread in NewCompartment kind of kills it. We create compartments pretty often, and this could cause annoying pauses.

It might make sense for the background sweep thread to get its own copy of the compartments vector. That seems cleaner to me than what we have now.
Comment 4 Jon Coppeard (:jonco) 2012-08-15 03:07:35 PDT
Created attachment 652054 [details] [diff] [review]
Part 1 - put collecting compartments on a linked list
Comment 5 Jon Coppeard (:jonco) 2012-08-15 03:08:29 PDT
Created attachment 652055 [details] [diff] [review]
Part 2 - use areanaListsToSweep for background sweeping too
Comment 6 Jon Coppeard (:jonco) 2012-08-15 09:25:57 PDT
Created attachment 652122 [details] [diff] [review]
Part 1 - put collecting compartments on a linked list v2
Comment 7 Jon Coppeard (:jonco) 2012-08-15 09:26:44 PDT
Created attachment 652123 [details] [diff] [review]
Part 2 - use areanaListsToSweep for background sweeping too v2
Comment 8 Bill McCloskey (:billm) 2012-08-16 22:11:24 PDT
Comment on attachment 652122 [details] [diff] [review]
Part 1 - put collecting compartments on a linked list v2

Review of attachment 652122 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good to me overall. I just wrote a small linked list class that I think would be useful here. The patch is in bug 783465. Could you base these patches on top of it and use it? I'm hoping that will make some of the list manipulation simpler.

::: js/src/jsgcinlines.h
@@ +396,4 @@
>  
>    public:
>      GCCompartmentsIter(JSRuntime *rt) {
> +        it = rt->gcCollectingCompartments;

Could you assert that we only call this during garbage collections?
Comment 9 Bill McCloskey (:billm) 2012-08-16 22:14:05 PDT
Comment on attachment 652123 [details] [diff] [review]
Part 2 - use areanaListsToSweep for background sweeping too v2

Review of attachment 652123 [details] [diff] [review]:
-----------------------------------------------------------------

It seems like this patch is pretty tightly tied to the other one. When you post the new patch, please just fold them together.

One suggestion: It's a little unfortunate how you have to remember to call FinishCollection at various times. I think it might be cleaner to tear down and then rebuild the linked list of compartments to collect in BeginMarkPhase.

::: js/src/jsgc.cpp
@@ +2829,5 @@
> +    for (CompartmentsIter c(rt); !c.done(); c.next()) {
> +        JS_ASSERT(!c->isCollecting());
> +        JS_ASSERT(!c->wasGCStarted());
> +        JS_ASSERT(!c->gcNextCompartment);
> +        for (unsigned i = 0 ; i < FINALIZE_LIMIT ; ++i) {

No brace needed here.

@@ +3090,5 @@
>           * We must finalize in the insert order, see comments in
>           * finalizeObjects.
>           */
>          FreeOp fop(rt, false, true);
> +        for (int phase = 0 ; phase < BackgroundPhaseCount ; ++phase) {

No spaces before the semicolons.

@@ +3092,5 @@
>           */
>          FreeOp fop(rt, false, true);
> +        for (int phase = 0 ; phase < BackgroundPhaseCount ; ++phase) {
> +            for (GCCompartmentsIter c(rt); !c.done(); c.next()) {
> +                for (int index = 0 ; index < BackgroundPhaseLength[phase] ; ++index) {

No spaces before the semicolons.

@@ +3095,5 @@
> +            for (GCCompartmentsIter c(rt); !c.done(); c.next()) {
> +                for (int index = 0 ; index < BackgroundPhaseLength[phase] ; ++index) {
> +                    AllocKind kind = BackgroundPhases[phase][index];
> +                    ArenaHeader *arenas = c->arenas.arenaListsToSweep[kind];
> +                    if (arenas) {

No braces needed. Also, you can do:

if (ArenaHeader *arenas = c->arenas.arenaListsToSweep[kind])
    ArenaLists::backgroundFinalize(&fop, arenas);

::: js/src/jsgc.h
@@ +145,5 @@
>      return map[kind];
>  }
>  
> +static inline bool
> +IsBackgroundFinalized(AllocKind kind)

We have IsBackgroundAllocKind for this.
Comment 10 Bill McCloskey (:billm) 2012-08-16 22:43:31 PDT
Given the discussion in bug 783465, it sounds like it would be better to use the existing linked list class in mfbt. You can include it as "mozilla/LinkedList.h".
Comment 11 Jon Coppeard (:jonco) 2012-08-17 08:03:43 PDT
Thanks for the comments!

I had a go at using the mozilla list class, but this requires the list element class to publicly inherit from LinkedListElement, which adds the list manipulation methods to its public interface, e.g. JSCompartment gets a remove() method.  I agree with the idea, but this felt wrong, so I stuck to the ad-hoc singly linked list but only used it for passing the compartment list to the background thread.  This is much simpler than in the original patch.

> > +IsBackgroundFinalized(AllocKind kind)
> We have IsBackgroundAllocKind for this.

IsBackgroundAllocKind only seem to work for objects, not other GC things.

Other comments addressed in the new patch.
Comment 12 Jon Coppeard (:jonco) 2012-08-17 08:04:49 PDT
Created attachment 652770 [details] [diff] [review]
Proposed fix v3
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-08-21 19:11:10 PDT
https://hg.mozilla.org/mozilla-central/rev/afd29b8ab521

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