763800 - Eliminate most uses of JS_THREADSAFE from the JS GC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

The GC uses JS_THREADSAFE to decide whether to do background finalization. It's really frustrating that we have two separate paths in all these cases. In most cases, I think we can just use a single code path.

There are several reasons why we use JS_THREADSAFE:

1. Some variable declarations are for types (like PRThread and PRLock) that are defined only in JS_THREADSAFE builds. However, it would be easy to create dummy declarations for these in jslock.h if !JS_THREADSAFE.

2. Sometimes code depends on variables that are only defined if JS_THREADSAFE, and so that code must also be JS_THREADSAFE. Once we fix the variables, we can remove the #ifdefs around the code.

3. The GC does sweeping on a background thread, and we have lots of code to decide whether to do background sweeping or not based on JS_THREADSAFE. However, I would like us to change the code so that it looks like we always do background sweeping: at the end of a GC, if JS_THREADSAFE is not defined, we would just run the background thread's code in the foreground.

Eventually, it would be nice if background sweeping were a flag that could be changed at runtime rather than a compile-time switch. That would make the code cleaner and more flexible. However, it would be fine if the only result of this bug is to clean things up.

Till, I realize that this bug isn't very well specified, but I think that the results could be really nice. Please ask lots of questions, and I'll help as much as I can.

Comment 1

[Luke Wagner [:luke]]

This sounds great; will it implicitly fix bug 760739?

Comment 2

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

Yes it will.

Comment 4

[Till Schneidereit [:till]]

Attachment: wip

Thanks for the great task - exactly what I was hoping for!

The attached patch moves almost all of the GC-related differences between threadsafe and non-threadsafe configs into GCHelperThread itself.

I hope my approach isn't entirely off base - at least it passes all tests and jit-tests lets me run the browser without issues.

Apart from whether the whole approach is ok or not, I have mainly two remaining questions:

- Do I understand correctly that the FINALIZE_OBJECT*_BACKGROUND kinds are now pretty much redundant? As far as I can tell, right now they aren't used anymore (or, more correctly, their non-_BACKGROUND counterparts aren't used). If that's correct, the entire machinery for switching to _BACKGROUND kinds could be removed, too, right?

- I'm not sure what to do about the ifdefs in Chunk::releaseArena.
Should the first one maybe be replaced with a call to rt->gcHelperThread.waitBackgroundSweepEnd()?
As for the second one: I'm at a complete loss about that, I'm afraid.

Comment 5

[Till Schneidereit [:till]]

Attachment: now with some documentation

I've added support for detecting if a sweep is actively performed by GCHelperThread and removed the two ifdefs from Chunk::releaseArena.

I'm really not wild about the names I came up with (sweepInProgress and, much worse, inDoSweep), so suggestions are very much welcome.

I've also added a short comment about the fact that GCHelperThread will run on the main thread in non-threadsafe builds. Makes me wonder if the class shouldn't lose the "Thread" suffix, though.

Comment 6

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

Comment on attachment 632435 [details] [diff] [review]
now with some documentation

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

This looks good to me. I do have one general question. As far as I can tell, if !JS_THREADSAFE, the GCHelperThread::state field will never be anything except IDLE (and maybe SHUTDOWN). It can never be any of the ALLOCATING states because backgroundAllocation is always false. And it will never be SWEEPING because GCHelperThread::prepareForBackgroundSweep always returns false. Please correct me if I'm wrong.

If this is true, we should take advantage of it. None of the !JS_THREADSAFE code needs to deal with the case where the state is SWEEPING or ALLOCATING. I think this means that you can remove some code that this patch adds (like in GCHelperThread::waitBackgroundSweepEnd and GCHelperThread::finish).

Additionally, I don't think that the extra inDoSweep flag is needed on GCHelperThread. Instead, we can just check if state == SWEEPING, like we do in the current code.

::: js/src/jsgc.cpp
@@ +2676,5 @@
>          PR_DestroyCondVar(done);
> +#else
> +    /*
> +     * We cannot be in the ALLOCATING or CANCEL_ALLOCATION states as
> +     * the allocations should have been stopped during the last GC.

If !JS_THREADSAFE, then we should never be in either of these states, right? Because the backgroundAllocation flag is false?

@@ +2686,5 @@
>  }
>  
>  /* static */
>  void
>  GCHelperThread::threadMain(void *arg)

Can you put this entire function inside the same #ifdef JS_THREADSAFE block as threadLoop?

@@ +2753,4 @@
>      size_t maxArenaLists = MAX_BACKGROUND_FINALIZE_KINDS * rt->compartments.length();
>      return finalizeVector.reserve(maxArenaLists);
> +#else
> +    return false;

Great idea!

@@ +2823,5 @@
>      while (state == SWEEPING || state == CANCEL_ALLOCATION)
>          PR_WaitCondVar(done, PR_INTERVAL_NO_TIMEOUT);
> +#else
> +    if (state == ALLOCATING)
> +        state = IDLE;

How will the state ever be ALLOCATING? Can we assert that it's not?

::: js/src/jsgc.h
@@ +578,3 @@
>          backgroundAllocation(true)
> +#else
> +        backgroundAllocation(false)

I don't think it matters if we set this to true or false. We always change it in GCHelperThread::init anyway.

Comment 7

[Till Schneidereit [:till]]

Attachment: review comments incorporated

(In reply to Bill McCloskey (:billm) from comment #6)
> This looks good to me. I do have one general question. As far as I can tell,
> if !JS_THREADSAFE, the GCHelperThread::state field will never be anything
> except IDLE (and maybe SHUTDOWN). It can never be any of the ALLOCATING
> states because backgroundAllocation is always false. And it will never be
> SWEEPING because GCHelperThread::prepareForBackgroundSweep always returns
> false. Please correct me if I'm wrong.

Right, I hadn't considered the implications of that - great find.

I have removed the thus unnecessary code, put lots more code into #ifdef JS_THREADSAFE and added asserts about the state and JS_NOT_REACHED where valid.

Pushed to try with the resulting patch: https://tbpl.mozilla.org/?tree=Try&rev=1799ada476da

Comment 8

[Gregor Wagner [:gwagner]]

I tried to make the background finalize types not depending on JS_THREADSAFE when introducing them but I couldn't do it without regressing microbenchmarks. Please also make sure that allocating a ton of objects in a shell-benchmark for example doesn't regress too much.

Comment 9

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

Comment on attachment 632618 [details] [diff] [review]
review comments incorporated

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

Great, thanks.

I'd be surprised if browser performance regressed from this patch. Shell performance might regress, but I'm not concerned about that. We're basically just bringing browser and shell performance closer.

::: js/src/jsgc.h
@@ +594,5 @@
>          return backgroundAllocation;
>      }
>  
> +    bool isSweeping() const {
> +        return state == SWEEPING;

We already have this, named "sweeping".

Comment 10

[Gregor Wagner [:gwagner]]

(In reply to Bill McCloskey (:billm) from comment #9)
> Comment on attachment 632618 [details] [diff] [review]
> review comments incorporated
> 
> Review of attachment 632618 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great, thanks.
> 
> I'd be surprised if browser performance regressed from this patch. Shell
> performance might regress, but I'm not concerned about that. We're basically
> just bringing browser and shell performance closer.

This patch makes a lot of sense but I still had people coming after me when I regressed the single-threaded shell :)

Comment 11

[Till Schneidereit [:till]]

Attachment: redundant isSweeping removed

D'oh, thanks for pointing out GCHelperThread::sweeping. Removed isSweeping.

(In reply to Gregor Wagner [:gwagner] from comment #8)
> I tried to make the background finalize types not depending on JS_THREADSAFE
> when introducing them but I couldn't do it without regressing
> microbenchmarks. Please also make sure that allocating a ton of objects in a
> shell-benchmark for example doesn't regress too much.

I tested just allocating 100m objects of different types in a loop, with inconclusive results:
let date = new Date
and
let arr = []
regressed by about 1.9%, whereas
let str = 'aaa'
let num = i
let obj = {}
all improved by about 2.1%.

I don't know where the improvement comes from, but it is very consistent.

Comment 12

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4530efc8e2ec

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4530efc8e2ec