965745 - operation callback not always run when running Ion code, leading to high memory usage

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

function *gen() {
  for(;;)
    yield gen;
}
var f = gen;
while (f) {
  f = gen();
}

When running this code for a few minutes, it fills up memory. nbp and wingo also think that it might be a memory leak.

Comment 1

[Andy Wingo [:wingo]]

Simpler case:

function *gen() { while (1) yield gen }
while (1) gen()

Also occurs with legacy generators:

function gen() { while (1) yield gen }
while (1) gen()

Comment 2

[Andy Wingo [:wingo]]

WTF?  This is pretty basic:

function *gen() { yield 1 }
while (1) gen()

Comment 3

[Jason Orendorff [:jorendorff]]

dumpHeap() says that GC is collecting the objects, and valgrind memcheck does not detect extra leaks when I allocate more generators rather than less. I must be doing something wrong.

Comment 4

[Andy Wingo [:wingo]]

Steps to reproduce:

 1. Build a fresh release-mode shell.
 2. Open a system memory monitor (e.g. top, sorted by RSS).
 3. Run the script.

Expectations: The js process should consume a constant small amount of memory (up to a few dozen MB).

Actual results: The RSS of the JS process continues to rise.  After 5 seconds on my machine it takes 5.5GB of private dirty memory.

Note that if you limit the heap size using the relevant function whose name I don't recall right now, the RSS doesn't rise.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

function *gen() { yield 1 }
var count = 0;
while (1) {
    gen()
    if (count++ == 1000) {
        gc();
        count = 0;
    }
}

With this test case, the memory doesn't increase at all, except for the few MB needed for the runtime. I am not really aware of the conditions under which a GC occurs, but h4writer tells me that a GC should naturally occur after X allocations, which is not the case in the initial test case.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: patch-backedges

After adding some tracing, it seems that we are triggering the operation callback (due to memory allocation), but it is never running.

Aparrently we check the interrupt flag at script entry and loop backedges, although we use mprotect and/or patch loop backedges when running Ion-generated code.

I tried with --no-ion and --ion-eager flags and the problem seems to be Ion specific.

Looking at the JitRuntime::ensureIonCodeAccessible() it seems we only patch the backedges if the code was previously protected, which is not always the case.

I don't know this code particularly, but it seems like we would always want to patch the backedges if the interrupt flag is set.

How does this look for a fix?  I moved some of the comments around as well to make the make more sense.

Also, do we ever re-patch the backedges to not check the interrupt flag?  I couldn't see where this was done.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8369372 [details] [diff] [review]
patch-backedges

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

Looks good, nice catch!

(In reply to Jon Coppeard (:jonco) from comment #6)
> Looking at the JitRuntime::ensureIonCodeAccessible() it seems we only patch
> the backedges if the code was previously protected, which is not always the
> case.

Yeah, the assumption is that ensureIonCodeAccessible is only called from the signal handler but that's not true.

> Also, do we ever re-patch the backedges to not check the interrupt flag?  I
> couldn't see where this was done.

This is done in InterruptCheck in jit/VMFunctions.cpp:

    cx->runtime()->jitRuntime()->patchIonBackedges(cx->runtime(),
                                                   JitRuntime::BackedgeLoopHeader);

Comment 8

[Andrew McCreight [:mccr8]]

Not knowing much about this, this sounds kind of bad.  Is this only a problem for generators?  Should it be backported?  "After 5 seconds on my machine it takes 5.5GB of private dirty memory" does not sound good. ;)

Comment 9

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/357895a6aaac

Comment 10

[Andrew McCreight [:mccr8]]

Jon, do you have any idea about the questions in comment 8?  Thanks.

Comment 11

[Jon Coppeard (:jonco)]

It's not only a problem for generators.  I'm not really sure how often it will occur in practice - I guess you would have to only run Ion code otherwise the interpreter would end up invoking the callback.  But it's a simple fix, so assuming no problems show up in the next few days I think we should backport.

Comment 12

[Nicholas Nethercote [inactive]]

> But it's a
> simple fix, so assuming no problems show up in the next few days I think we
> should backport.

Yes. FWIW, the highest-rated comment on the Hacker News discussion of Firefox 27's release was about how it's the first major browser release to support generators: https://news.ycombinator.com/item?id=7177607

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/357895a6aaac

Comment 14

[Till Schneidereit [:till]]

Doesn't seem like any problems showed up, right?

Comment 15

[Jon Coppeard (:jonco)]

Comment on attachment 8369372 [details] [diff] [review]
patch-backedges

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 864220 - IonMonkey: Implement loop interrupt checks using mprotect
User impact if declined: Possibility of high memory usage
Testing completed (on m-c, etc.): On m-c since 2014-02-05
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5093186ca570
https://hg.mozilla.org/releases/mozilla-beta/rev/9ea0906ca4fe

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/9ea0906ca4fe

Comment 18

[Andrew McCreight [:mccr8]]

Is the operation callback where we are doing our stack overflow checks in Ion code by any chance?  There are some top crashes that seem to involve JITted code (bug 856670, bug 969441) that could be related to this, if so.  I guess we can see if crash volume for those signatures goes down on non-release branches.

Comment 19

[Jan de Mooij [:jandem]]

(In reply to Andrew McCreight [:mccr8] from comment #18)
> Is the operation callback where we are doing our stack overflow checks in
> Ion code by any chance?

No it's the other way around: when we trigger the operation callback, we modify the stack limit for JIT code so that the check always fails and we call into the VM. There we handle the operation callback and reset the stack limit.

(This only applies to script entry; loops rely on mprotect/signal handlers or an explicit check if that's disabled.)

> There are some top crashes that seem to involve
> JITted code (bug 856670, bug 969441) that could be related to this, if so. 
> I guess we can see if crash volume for those signatures goes down on
> non-release branches.

I'll take a look at these bugs.

Comment 20

[u279076]

Benjamin, can you please confirm this is fixed for you now in Firefox 28, 29, and 30? Should this get an in-testsuite testcase?

Comment 21

[Benjamin Bouvier [:bbouvier] (inactive)]

This is fixed in the latest nightly, couldn't say for the other releases.

I think it cannot get in an automatically reproducible testcase without being run in valgrind, since as far as i can tell, the shell cannot say whether it leaks memory or not (and if it can, I would love to know how to do that).