1125259 - Huge heap executing JavaScript (70x compared to V8)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Stephen Adams]

Attachment: bug.js

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2171.95 Safari/537.36

Steps to reproduce:

jsshell -f bug.js



Actual results:

The program prints some lines, mostly beginning "Mirrors read error: ..." and ends by throwing an exception.  This is expected behaviour.

What is wrong is this program takes 10GB to run on jsshell or in FireFox.
On d8 (Chrome's V8 equivalent to jsshell) the heap is never bigger than 150MB.

jsshell --help reports: Version: JavaScript-C28.0




Expected results:

I would expect the two JavaScript engines execute in roughly the same amount of memory, say a factor of 2x-3x.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I see us get to about 1.2GB on Mac in a current opt shell.  d8 gets to about 300MB.

Looking at what we're doing during the run, we're allocating a ton of stack strings from the new Error call in wrapException.  If I just turn off stack capture for new Error, our memory usage never goes over about 120MB.  Bug 1038238 would help.

OK, so that leaves an important question: is the script holding on to all those error objects, or are we just failing to GC expeditiously enough for some reason?

Comment 2

[Boris Zbarsky [:bzbarsky]]

So I tried modifying wrapException by adding this line right after the |new Error| line:

    var stack = wrapper.stack;

and then run in d8, memory usage goes to approximately 600MB before the program terminates.

Comment 3

[Boris Zbarsky [:bzbarsky]]

And a bit of testing shows there are 106k calls to wrapException during the run.  

None of this says what the lifetime of those Errors _should_ be.

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK, so in a debugger I do see a bunch of TOO_MUCH_MALLOC gcs as we go.  So presumably the script is keeping the exceptions alive and they're not being collected, even though GC is triggering.

Comment 5

[Boris Zbarsky [:bzbarsky]]

One other important difference that probably explains the lower V8 memory usage.  V8's stacks stored on Error objects are only 10 frames deep as far as I can tell.  We'll report the full stack as long as the stack string is less than 2^20 characters.  So for deep enough stacks and exceptions kept alive, we'll presumably use more memory than V8 will.

Comment 6

[Luke Wagner [:luke]]

This code appears to be compiled from Dart.  Does anyone know if this behavior is particular the the Dart program being compiled or inherent to all compiled Dart code?  If it was the latter, that would certainly raise the priority.

Comment 7

[Stephen Adams]

Yes, this is compiled from Dart.  I don't think the huge heap is a general problem.
This program is one of our tests that started failing on a test bot that was configured with 8GB RAM.

We do have some benchmarks programs that are much slower on jsshell than d8.
It would be great of we could get an understanding of that.
I will file those separately.  Is there some way I should mark that bug report?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Stephen, just filing them and ccing me and Luke should be enough.

Comment 9

[Luke Wagner [:luke]]

ooc, is the source Dart program explicitly creating and holding onto a lot of error objects or is the Error object an impl detail of some non-error Dart thing?

Comment 10

[Boris Zbarsky [:bzbarsky]]

It looked to me like the Error objects here actually represented Dart exceptions....

Comment 11

[Luke Wagner [:luke]]

Ah, then I guess the problem isn't Dart specific; the same issue would arise if a JS app was creating and saving a ton of Errors.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Yes, absolutely.  Lots of Errors with deep stacks == badtimes on large string allocations.

Comment 13

[Nicholas Nethercote [inactive]]

> We'll report the full stack as long as the stack string is less than 2^20 characters.

Where is that 2^20 number in the code? That sounds like absurdly large limit. Could we just make it a lot smaller?

Comment 14

[Boris Zbarsky [:bzbarsky]]

It's the super-poorly named "MaxReportedStackDepth" in js::ComputeStackString in jsexn.cpp.

We could certainly make it a lot smaller, but note that V8 is broken in the other extreme, imo: 10 stackframes is not enough to tell what the heck is going on with most sites...

Comment 15

[Nicholas Nethercote [inactive]]

> We could certainly make it a lot smaller, but note that V8 is broken in the
> other extreme, imo: 10 stackframes is not enough to tell what the heck is
> going on with most sites...

Sure. We could do something like limit it to 100 stack frames or 2^15 bytes, whichever is smaller?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Yeah, that seems fine.  Console API caps to 200 stackframes, fwiw.

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I just tested again now that bug 1038238 landed. Peak memory usage was ~167MB.

Do we want to resolve this bug now? Or are we still investigating why the Error objects are retained?

Comment 18

[Boris Zbarsky [:bzbarsky]]

> I just tested again now that bug 1038238 landed. Peak memory usage was ~167MB.

Fixed!  ;)

Afaict the Error objects are retained by the JS code itself...