Last Comment Bug 1125259 - Huge heap executing JavaScript (70x compared to V8)
: Huge heap executing JavaScript (70x compared to V8)
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 28 Branch
: x86_64 Linux
-- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 1038238
Blocks:
  Show dependency treegraph
 
Reported: 2015-01-23 11:00 PST by Stephen Adams
Modified: 2015-03-31 11:15 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
bug.js (1.64 MB, application/javascript)
2015-01-23 11:00 PST, Stephen Adams
no flags Details

Description User image Stephen Adams 2015-01-23 11:00:07 PST
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.
Comment 1 User image Boris Zbarsky [:bz] (still a bit busy) 2015-01-23 11:17:18 PST
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 User image Boris Zbarsky [:bz] (still a bit busy) 2015-01-23 11:20:09 PST
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 User image Boris Zbarsky [:bz] (still a bit busy) 2015-01-23 11:28:49 PST
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 User image Boris Zbarsky [:bz] (still a bit busy) 2015-01-23 11:33:18 PST
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 User image Boris Zbarsky [:bz] (still a bit busy) 2015-01-23 11:46:03 PST
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 User image Luke Wagner [:luke] 2015-01-23 17:01:17 PST
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 User image Stephen Adams 2015-01-23 17:37:00 PST
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 User image Boris Zbarsky [:bz] (still a bit busy) 2015-01-23 20:27:35 PST
Stephen, just filing them and ccing me and Luke should be enough.
Comment 9 User image Luke Wagner [:luke] 2015-01-26 07:33:22 PST
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 User image Boris Zbarsky [:bz] (still a bit busy) 2015-01-26 08:07:22 PST
It looked to me like the Error objects here actually represented Dart exceptions....
Comment 11 User image Luke Wagner [:luke] 2015-01-26 10:24:49 PST
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 User image Boris Zbarsky [:bz] (still a bit busy) 2015-01-26 10:37:34 PST
Yes, absolutely.  Lots of Errors with deep stacks == badtimes on large string allocations.
Comment 13 User image Nicholas Nethercote [:njn] 2015-02-03 14:13:51 PST
> 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 User image Boris Zbarsky [:bz] (still a bit busy) 2015-02-03 17:29:43 PST
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 User image Nicholas Nethercote [:njn] 2015-02-03 18:44:17 PST
> 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 User image Boris Zbarsky [:bz] (still a bit busy) 2015-02-03 19:15:19 PST
Yeah, that seems fine.  Console API caps to 200 stackframes, fwiw.
Comment 17 User image Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] 2015-03-31 11:09:28 PDT
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 User image Boris Zbarsky [:bz] (still a bit busy) 2015-03-31 11:15:05 PDT
> 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...

Note You need to log in before you can comment on or make changes to this bug.