Huge heap executing JavaScript (70x compared to V8)

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Stephen Adams, Unassigned)

Tracking

28 Branch
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment)

1.64 MB, application/javascript
Details
(Reporter)

Description

2 years ago
Created attachment 8553878 [details]
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.
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?
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.
Depends on: 1038238
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.
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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [MemShrink]

Comment 6

2 years ago
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.
(Reporter)

Comment 7

2 years ago
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?
Stephen, just filing them and ccing me and Luke should be enough.

Comment 9

2 years ago
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?
It looked to me like the Error objects here actually represented Dart exceptions....

Comment 11

2 years ago
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.
Yes, absolutely.  Lots of Errors with deep stacks == badtimes on large string allocations.
> 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?
Whiteboard: [MemShrink] → [MemShrink:P2]
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...
> 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?
Yeah, that seems fine.  Console API caps to 200 stackframes, fwiw.
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?
> 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...
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.