921224 - Minimize GCMark buffer when incremental GC is disabled.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp — off until 29-09]]

Currently, on an almost empty worker (~1MB), the gcmarker buffer use ~130 KB.

After discussing with both Terrence and Bill, we cannot move this to a fully decommited memory as we want to avoid OOM when we react to a near OOM warning reported by the kernel.

On the other hand, this was made large because we need a large buffer for barrier of an incremental GC (in-between slices), which means that we can reduce this value in case of non-incremental GCs.

This matters because workers are still working with non-incremental GCs.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Use a smaller gc-marker stack for worker runtimes.

This straw-man patch adds a boolean |isWorker| argument to GCMarker::init(),
and uses that to determine the marking stack size.  That change necessitated
bubbling |isWorker| arguments out through various constructors and
initialization functions, all the way to WorkerJSRuntime's constructor.

It works.  Compare the main runtime:

│   ├───7,042,680 B (06.69%) -- runtime
...
│   │   ├────262,144 B (00.25%) ── gc-marker

with a worker runtime:

│   ├───3,936,208 B (03.74%) --
worker(resource:///modules/sessionstore/SessionWorker.js, 0x7f1f2ea24000)
│   │   ├──1,199,840 B (01.14%) -- runtime
...
│   │   │  ├─────32,768 B (00.03%) ── gc-marker

(This is a 64-bit build, which is why the main runtime's stack is 256 KiB
rather than 128 KiB.)

But I won't be surprised if people don't like this approach.  For example, it
would be nicer if the stack size was derived automatically depending on whether
or not incremental GC is enabled for the runtime.  But I couldn't see what code
caused a worker runtime to not use incremental GC.  (I see
DisableIncrementalGC(), but it's never called.  Do workers somehow hit the
|gcIncrementalEnabled=false| path in jsobj.cpp?)

I also don't know if reducing the stack size by 8x is a reasonable number.

Comment 2

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

Thanks for doing the work on this, but I would like a different approach. Incremental GC is enabled when we set rt->gcMode to JSGC_MODE_INCREMENTAL in JS_SetGCParameter. This gets called from SetMemoryGCModePrefChangedCallback in nsJSEnvironment.cpp based on a pref.

I think it would be better if we start with a small ballast for the mark stack and then change it if the pref changes. This would require:

- Writing a resizeBallast function in the MarkStack class. This shouldn't be too hard. Basically, we just need to realloc |ballast|, change |ballastLimit|, and possibly change |stack|, |tos|, and |limit|, depending on whether |stack == ballast|. The code for |enlarge| may be helpful in understanding the data structure invariants.

- Change JS_SetGCParameter so that it informs the GC if the mode changes. Maybe it could call a function you could write in jsgc.cpp. That function would just call resizeBallast if we're changing modes between incremental and non-incremental.

I have no idea how big a reasonable size is for the mark stack for workers. I guess 16/32KB seems reasonable. I would guess that most worker GCs happen when everything has been torn down, in which case we should mark very little. Maybe :bent would know more.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Worker GCs also happen after a small idle timeout (5s maybe?) or whenever they get paused.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Use a smaller mark stack when incremental GC is disabled.

Comment 5

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

Comment on attachment 830631 [details] [diff] [review]
Use a smaller mark stack when incremental GC is disabled.

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

This looks nice. Just the one issue below.

::: js/src/jsgc.h
@@ +1039,5 @@
> +            return;
> +
> +        T *oldBallast = ballast;
> +        T *newBallast = (T *)js_realloc(ballast, sizeof(T) * ballastcap);
> +        if (!newBallast)

Can you please write it as:
if (!newBallast) {
    /* Just ignore... */
    return;
}

@@ +1047,5 @@
> +        ballast = newBallast;
> +        ballastLimit = ballast + ballastcap;
> +
> +        if (stack == oldBallast) {
> +            size_t tosIndex = tos - stack;

Thanks for getting back to this. I'm sorry to have to push back again, but I think there's a problem here. If stack==oldBallast and ballastcap < tosIndex, then this code won't work.

Perhaps, if oldBallast == stack and if you're making the ballast smaller, then you could keep the old ballast around as the current stack. The old ballast would eventually be freed at the end of the next GC. This won't typically happen unless the user switches to non-incremental GC via the pref, so we don't really care about its performance.

Comment 6

[Nicholas Nethercote [inactive]]

Why do we even need the ballast?  AIUI:

- ballast is used as the stack to begin with.

- If we push() enough times that it overflows we call enlarge(), and a new bigger stack is allocated, and ballast is held onto.

- Finally, when we call reset(), any non-ballast stack is freed and the ballast is made into the stack again.

I guess the key point is that you want reset() to be infallible?  But if we didn't have ballast, then reset() would either (a) do nothing, or (b) realloc stack to be smaller than it currently is.  (b) is unlikely to fail, and if it did, we could just ignore the failure and keep the overly-large stack in place, similar to what I did in resizeBallast().

Comment 7

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

Yeah, that makes sense.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Don't use ballast in MarkStack.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Use a smaller mark stack when incremental GC is disabled.

Note that we now pass the JSGCMode down to MarkStack when it's created and let
it decide how to size itself.

Comment 10

[Nicholas Nethercote [inactive]]

BTW, I did some ad hoc profiling to see how often the mark stack is enlarged.  During a few minutes of browsing (TechCrunch, gmail, plus v8 and Sunspider benchmarks), I got this:

( 1)      5 (50.0%, 50.0%): ++ enlrg(0x7fa46c493430), cap = 32768 -> 65536
( 2)      3 (30.0%, 80.0%): ++ enlrg(0x7fa46c493430), cap = 65536 -> 131072
( 3)      1 (10.0%, 90.0%): ++ enlrg(0x7fa46c493430), cap = 131072 -> 262144
( 4)      1 (10.0%,100.0%): ++ enlrg(0x7fa46c493430), cap = 262144 -> 524288

The main runtime mark stack, which does incremental GC, increased a few times.  The worker runtime mark stacks never increased.  So having 4096 entries for non-incremental GC mark stacks seems ok.

Comment 11

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

Comment on attachment 831988 [details] [diff] [review]
(part 1) - Don't use ballast in MarkStack.

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

Thanks.

::: js/src/jsgc.h
@@ +906,5 @@
>  template<class T>
>  struct MarkStack {
> +    T *stack_;
> +    T *tos_;
> +    T *limit_;

A better name for this might be end_.

@@ +911,2 @@
>  
> +    size_t baseCap_;        // The capacity we start with and reset() to.

Please put the comment above the field. Also, please spell out capacity instead of cap in all the field and method names.

@@ +931,4 @@
>  
> +    void setStack(T *stack, size_t tosIndex, size_t cap) {
> +        stack_ = stack;
> +        tos_   = stack + tosIndex;

Please don't line these up like this.

@@ +936,5 @@
> +    }
> +
> +    bool init(size_t baseCap) {
> +        baseCap_ = baseCap;
> +        if (baseCap_ < minCap_) baseCap_ = minCap_;

These should go on separate lines. Same for the next statement.

@@ +949,4 @@
>          return true;
>      }
>  
> +    void setSizeLimit(size_t newCap) {

This should probably be setMaxCapacity given all the other uses of capacity.

@@ +953,2 @@
>          JS_ASSERT(isEmpty());
> +        maxCap_ = newCap;

You should also adjust baseCap_ to be less than maxCap_. Otherwise reset() won't actually resize the stack.

@@ +991,5 @@
>  
>      void reset() {
> +        T *newStack;
> +        if (cap() == baseCap_) {
> +            newStack = stack_;      // No size change; keep the current stack.

Comment should go above this line. Also, it seems easier to return here. Then the rest of the function could be indented less.

@@ +997,5 @@
> +            newStack = (T *)js_realloc(stack_, sizeof(T) * baseCap_);
> +            if (!newStack) {
> +                // If the realloc fails, just keep using the existing stack;
> +                // it's not ideal but better than failing.
> +                newStack = stack_;

I think it would be simpler just to return.

Comment 12

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

Comment on attachment 831989 [details] [diff] [review]
(part 2) - Use a smaller mark stack when incremental GC is disabled.

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

I'm not sure that the switching logic for incremental/non-incremental should go in the mark stack code itself. However, this patch has been through enough iterations. Please do what seems best to you.

::: js/src/jit-test/tests/basic/bug656261.js
@@ +4,5 @@
>  }
>  
>  function test()
>  {
> +    var N = internalConst("NON_INCREMENTAL_MARK_STACK_BASE_CAPACITY") + 2;

Shell tests run with the mode set to JSGC_MODE_INCREMENTAL, so this isn't the right number to use. Why not just have some max overall capacity that we have an internal const for?

::: js/src/jsgc.h
@@ +936,5 @@
>          tos_   = stack + tosIndex;
>          limit_ = stack + cap;
>      }
>  
> +    void setBaseCap(JSGCMode mode) {

setBaseCapacity.

Comment 13

[Nicholas Nethercote [inactive]]

> @@ +953,2 @@
> >          JS_ASSERT(isEmpty());
> > +        maxCap_ = newCap;
> 
> You should also adjust baseCap_ to be less than maxCap_. Otherwise reset()
> won't actually resize the stack.

You mean like this?

  if (baseCap_ > maxCap_)
    baseCap_ == maxCap_;

That handles the case where we're shrinking the stack size below the previous base capacity, which I think is what you're worried about?  I guess we also need to handle the case where setSizeLimit() tries to set maxCap below minCap?

Comment 14

[Nicholas Nethercote [inactive]]

> @@ +991,5 @@
> >  
> >      void reset() {
> > +        T *newStack;
> > +        if (cap() == baseCap_) {
> > +            newStack = stack_;      // No size change; keep the current stack.
> 
> Comment should go above this line. Also, it seems easier to return here.
> Then the rest of the function could be indented less.
> 
> @@ +997,5 @@
> > +            newStack = (T *)js_realloc(stack_, sizeof(T) * baseCap_);
> > +            if (!newStack) {
> > +                // If the realloc fails, just keep using the existing stack;
> > +                // it's not ideal but better than failing.
> > +                newStack = stack_;
> 
> I think it would be simpler just to return.

In both of these cases tos_ needs to be reset, and in the second case end_ needs to be reset.  The setStack() call at the end of reset() achieves both of these, and I like having a single function that resets stack_, tos_ and end_ consistently rather than resetting them piecemeal.

Comment 15

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

(In reply to Nicholas Nethercote [:njn] from comment #13)
> > @@ +953,2 @@
> > >          JS_ASSERT(isEmpty());
> > > +        maxCap_ = newCap;
> > 
> > You should also adjust baseCap_ to be less than maxCap_. Otherwise reset()
> > won't actually resize the stack.
> 
> You mean like this?
> 
>   if (baseCap_ > maxCap_)
>     baseCap_ == maxCap_;

Yes (but without the double-equal).

> That handles the case where we're shrinking the stack size below the
> previous base capacity, which I think is what you're worried about?  I guess
> we also need to handle the case where setSizeLimit() tries to set maxCap
> below minCap?

I don't really care about minCap.

> In both of these cases tos_ needs to be reset, and in the second case end_ needs to be reset. 
> The setStack() call at the end of reset() achieves both of these, and I like having a single
> function that resets stack_, tos_ and end_ consistently rather than resetting them piecemeal.

OK, I forgot about that.

Comment 16

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad3b4011a11d
https://hg.mozilla.org/integration/mozilla-inbound/rev/49c0a2303a8b

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ad3b4011a11d
https://hg.mozilla.org/mozilla-central/rev/49c0a2303a8b