1133140 - Move heap limit check up to CheckAllocatorState

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: 0_split_out_gcifneeded-v0.diff

One subtlety here is that so far all of the triggers we have moved to CheckAllocatorState have been main-thread only. This works fine because CheckAllocatorState is a NOP off the main thread. This first moves the GC checks up above the early return and then moves up the heap limit check up.

A second subtlety is that we currently do the heap size check under a method that gets retried 4 times with various LastDitchGC and waiting on the sweeping thread. With this restructure, we don't get that for free. Once I've duped that down, testGCOutOfMemory passes. One test, jit-test/tests/gc/bug-1109913.js, did not stop failing. After walking through it, I think it must have been succeeding because of some different bug, because it certainly should be raising OOM. I've annotated this test as OOMing to make it pass.

Comment 1

[Terrence Cole [:terrence]]

Attachment: 1_move_up_gcmaxbytes_check-v0.diff

And the second half.

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8564421 [details] [diff] [review]
0_split_out_gcifneeded-v0.diff

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

::: js/src/jsgcinlines.h
@@ +449,5 @@
>  CheckAllocatorState(ExclusiveContext *cx, AllocKind kind)
>  {
> +    if (allowGC && !GCIfNeeded(cx))
> +        return false;
> +

It's not immediately obvious whether the return value of GCIfNeeded is a standard failure return, or an indication of whether we GC'd. So I would prefer if this were spelled

  if (allowGC) {
    if (!GCIfNeeded(cx))
      return false;
  }

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8564422 [details] [diff] [review]
1_move_up_gcmaxbytes_check-v0.diff

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

My confidence is low that I am properly predicting the full effects of this change, but it looks sane enough to try.

::: js/src/jsgc.cpp
@@ -1039,5 @@
>  
> -    // Fail the allocation if we are over our heap size limits.
> -    if (!isHeapMinorCollecting() &&
> -        !isHeapCompacting() &&
> -        usage.gcBytes() >= tunables.gcMaxBytes())

Is this dominated by CheckAllocatorState? Should this be an assertion now?

  MOZ_ASSERT(usage.gcBytes() < tunables.gcMaxBytes());

Perhaps that's extra noise if it isn't strictly necessary for correctness.

Comment 4

[Terrence Cole [:terrence]]

(In reply to Steve Fink [:sfink, :s:] from comment #3)
> Comment on attachment 8564422 [details] [diff] [review]
> 1_move_up_gcmaxbytes_check-v0.diff
> 
> Review of attachment 8564422 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> My confidence is low that I am properly predicting the full effects of this
> change, but it looks sane enough to try.
> 
> ::: js/src/jsgc.cpp
> @@ -1039,5 @@
> >  
> > -    // Fail the allocation if we are over our heap size limits.
> > -    if (!isHeapMinorCollecting() &&
> > -        !isHeapCompacting() &&
> > -        usage.gcBytes() >= tunables.gcMaxBytes())
> 
> Is this dominated by CheckAllocatorState? Should this be an assertion now?

Oh my, no, that would be far too sane! We use a different path when allocating for the GC when tenuring or moving.

>   MOZ_ASSERT(usage.gcBytes() < tunables.gcMaxBytes());

And indeed, gcBytes can go over gcMaxBytes. We basically just have to delay until the next allowGC allocation happens and we can take the normal paths. 

> Perhaps that's extra noise if it isn't strictly necessary for correctness.

Comment 5

[Terrence Cole [:terrence]]

We unfortunately seem to depend rather heavily on our current last-ditch gc position in several tests, so moving that up is proving to be challenging. I'm landing the trivial part 1 change first to make rebasing easier as I work on different aspects: 
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0b21535d0a2

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d0b21535d0a2

Comment 7

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0844d972d08
https://treeherder.mozilla.org/#/jobs?repo=try&revision=73c6c968bbb3

The ggc jobs in the try run are orange because I missed an allocation. Should be fixed in the m-i push above.

Comment 8

[Terrence Cole [:terrence]]

Seems we're getting some OOMs, maybe, in jstests.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2e6a109fca8

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a0844d972d08

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jonco, maybe it's time to close this bug?

Comment 11

[Jon Coppeard (:jonco)]

Looks like both patches landed.