772820 - Don't GC when analysis/inference/compilation is active

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Splitting this off from bug 767223, which also makes this change; having to account for potential GCs while analyzing separate scripts in parallel would require all sorts of complicated, poorly tested logic.  Supersnappy also needs this.

Very few GC things are ever allocated during analysis --- template objects for objects created inline, very rarely initializing some standard class --- and being able to do GCs here doesn't add much value.  It's possible we could OOM from not being able to do a last ditch GC from one of these allocations, but that doesn't seem compelling.  There is tons of LifoAlloc and heap allocation done in these phases, and if any of those fail we will OOM without trying to GC anyways.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch (0f87c11f27b0)

Suppress GC activity during analysis/inference/compilation.  Incremental sweeping needs this I think and the lack of this is getting increasingly annoying when dealing with rooting around analysis functions.

Comment 2

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

Comment on attachment 703306 [details] [diff] [review]
patch (0f87c11f27b0)

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

This seems a little unpleasant to me, but maybe I'm being irrational. Telemetry shows that there are very few last-ditch GCs. It might be worth exploring whether we could entirely eliminate last-ditch GCs. Then the only source of GCs would be API calls and the operation callback.

::: js/src/jsgc.cpp
@@ +4518,5 @@
>  #ifdef JS_GC_ZEAL
>      JSRuntime *rt = cx->runtime;
>      PrepareForDebugGC(cx->runtime);
>  
> +    if (rt->mainThread.suppressGC)

Could you move this before PrepareForDebugGC? Otherwise it will schedule compartments for GC, and those flags will be used whenever we GC next, which seems undesirable.

::: js/src/jsinferinlines.h
@@ +346,5 @@
>   * Pins inference results so that intermediate type information, TypeObjects
>   * and JSScripts won't be collected during GC. Does additional sanity checking
>   * that inference is not reentrant and that recompilations occur properly.
>   */
>  struct AutoEnterTypeInference

It's kinda weird that the object is called AutoEnterTypeInference but the flag is called activeAnalysis. Were there just more uses of AutoEnterTypeInference?

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Bill McCloskey (:billm) from comment #2)
> This seems a little unpleasant to me, but maybe I'm being irrational.
> Telemetry shows that there are very few last-ditch GCs. It might be worth
> exploring whether we could entirely eliminate last-ditch GCs. Then the only
> source of GCs would be API calls and the operation callback.

Hmm, interesting idea, do you think this will change with a generational GC?  I would imagine we will do a lot more nursery collections at random spots in the VM there, and refusing to do collections would mean we have to allocate from the major heap if the nursery fills up, which seems undesirable.  I'm also not sure how much this would improve things in the end, as almost all hot operations which create roots can also reenter script and hit the operation callback.  See bug 831886 for some numbers about that.  (This is fixable, but the code needs refactoring whether we allow NewGCThing to GC or not.)

It's fine to avoid GCs during analysis because analysis cannot reenter script and because the number of GC things created is at most linear in size to the amount of analyzed code, and almost always puny.

> It's kinda weird that the object is called AutoEnterTypeInference but the
> flag is called activeAnalysis. Were there just more uses of
> AutoEnterTypeInference?

Yeah, this is me being lazy.  I'll s/AutoEnterTypeInference/AutoEnterAnalysis/g

Comment 4

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b531a62b114

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/7b531a62b114

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/7b531a62b114