Last Comment Bug 546477 - report JS engine VirtualAlloc usage (for the stack) to about:memory
: report JS engine VirtualAlloc usage (for the stack) to about:memory
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Linux
: -- normal with 1 vote (vote)
: ---
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on: 540706
Blocks: DarkMatter
  Show dependency treegraph
 
Reported: 2010-02-16 12:02 PST by Luke Wagner [:luke]
Modified: 2011-07-05 21:45 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (4.00 KB, patch)
2011-06-01 23:16 PDT, Nicholas Nethercote [:njn]
luke: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2010-02-16 12:02:59 PST
While asking Vlad about VirtualAlloc on Windows, he told me that we need to be reporting our memory usage that skips the system allocator to about:memory.  Bug 540706 adds a new use of VirtualAlloc, but there are other existing uses in the JS engine.
Comment 1 Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-04-07 16:03:06 PDT
This sounds like bug 532486, though that is a bug with two purposes (count, and switch to valloc).  Maybe we should narrow 532486 to the valloc case, and make this one for counting?
Comment 2 Igor Bukanov 2010-04-08 12:57:31 PDT
(In reply to comment #1)
> This sounds like bug 532486, though that is a bug with two purposes (count, and
> switch to valloc).  Maybe we should narrow 532486 to the valloc case, and make
> this one for counting?

The bug 557538 would allow to do all the necessary accounting outside the engine. And with jemalloc available together with the bug 553812 it would allow to have a common pool of chunks for jemalloc and the GC.
Comment 3 Nicholas Nethercote [:njn] 2011-04-10 22:51:16 PDT
Luke, are the VirtualAlloc calls in question the ones for code generated by the JITs?  If so, bug 633653 has this covered.
Comment 4 Luke Wagner [:luke] 2011-04-11 09:28:12 PDT
The VirtualAlloc calls I was referring to are in StackSpace::init and StackSpace::bumpCommit.
Comment 5 Nicholas Nethercote [:njn] 2011-05-13 00:27:07 PDT
> The VirtualAlloc calls I was referring to are in StackSpace::init and
> StackSpace::bumpCommit.

StackSpace::init allocates a 4MB chunk. Can you explain why Windows needs bumpCommit, and what it does?

The other place in the JS engine that calls mmap/VirtualAlloc is in jsgcchunk.cpp.  That only happens if MOZ_MEMORY is undefined and it's covered by the js/gc-heap reporter in xpcjsruntime.cpp anyway.
Comment 6 Nicholas Nethercote [:njn] 2011-05-13 00:38:25 PDT
Luke, is determining the StackSpace memory usage as simple as using ThreadDataIter to count how many ThreadData objects there are, and multiplying that number by StackSpace::CAPACITY_BYTES?
Comment 7 Luke Wagner [:luke] 2011-05-13 09:26:33 PDT
For Winodws, StackSpace::init reserves 4MB (via MEM_RESERVE) but initially commits (via MEM_COMMIT) only StackSpace::COMMIT_BYTES (128K) and grows as necessary (in bumpCommit).  (Note that this notion of "commit" is different than the kernel's.)  Thus, on windows, only the committed memory will be included in virtual memory usage.  This can be measured via (StackSpace::commitEnd_ - StackSpace::base_) * sizeof(Value).  Feel free to add a public memoryUsed() helper to return this.

On Linux/OSX, I initially wasn't able to find an equivalent of MEM_RESERVE/MEM_COMMIT, so we just mmap the whole thing eagerly.  However, I just learned (like yesterday) you can achieve the effect of MEM_RESERVE (without MEM_COMMIT) by mmap()ing with PROT_NONE and using mprotect() to later commit the memory.  I haven't verified this solution though.
Comment 8 Nicholas Nethercote [:njn] 2011-05-15 22:18:23 PDT
See also bug 656941, which proposed to extend the current Windows-only behaviour to all platforms.
Comment 9 Nicholas Nethercote [:njn] 2011-06-01 23:16:00 PDT
Created attachment 536826 [details] [diff] [review]
patch

This adds two reporters:  explicit/js/stack-space-committed and explicit/js/stack-space-uncommitted.  I don't have descriptions for them in yet, hence the "njn" comments.  Luke, does having two reporters seem reasonable to you?  The uncommitted one is currently always zero on non-Windows platforms.
Comment 10 Luke Wagner [:luke] 2011-06-02 15:29:54 PDT
Comment on attachment 536826 [details] [diff] [review]
patch

I don't understand the NS_MEMORY_REPORTER_IMPLEMENT macro, but it looks like uncommitted is going to be included in the "Explicit Allocations" sum.  Now, I know uncommitted memory still counts against us since it uses up VM address space which can crash us on 32-bit systems, but perhaps we could instead give "reserved but uncommitted" as a separate item under "Other Measurements".  (Will "vsize" include "reserved but uncommitted"?  If so, then it could be a subitem.)
Comment 11 Nicholas Nethercote [:njn] 2011-06-02 16:04:39 PDT
(In reply to comment #10)
> Comment on attachment 536826 [details] [diff] [review] [review]
> patch
> 
> I don't understand the NS_MEMORY_REPORTER_IMPLEMENT macro

http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsIMemoryReporter.idl#135

> but it looks like
> uncommitted is going to be included in the "Explicit Allocations" sum.  Now,
> I know uncommitted memory still counts against us since it uses up VM
> address space which can crash us on 32-bit systems, but perhaps we could
> instead give "reserved but uncommitted" as a separate item under "Other
> Measurements".  (Will "vsize" include "reserved but uncommitted"?  If so,
> then it could be a subitem.)

"vsize" will include reserved but uncommitted memory, but it's not present on Windows;  Windows has "private" instead.  And about:memory doesn't allow for "Other measurements" items to have sub-items, it's just a flat list.

Probably the best thing to do is to just remove stack-space-uncommitted, because it's (a) currently always 0 on Linux and Mac, and (b) on Windows that space costs us almost nothing (just address space) and so it's inherently uninteresting.  "explicit" isn't guaranteed to include absolutely everything anyway so I think this is the right trade-off.
Comment 12 Luke Wagner [:luke] 2011-06-02 16:07:26 PDT
Comment on attachment 536826 [details] [diff] [review]
patch

r+ sans uncommittedSize() and GetJSStackSpaceUncommitted, per previous comment.
Comment 13 Nicholas Nethercote [:njn] 2011-06-02 19:43:58 PDT
http://hg.mozilla.org/tracemonkey/rev/bf418b38bfe3
Comment 14 Chris Leary [:cdleary] (not checking bugmail) 2011-07-05 21:45:33 PDT
hg.mozilla.org/mozilla-central/rev/bf418b38bfe3

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