report JS engine VirtualAlloc usage (for the stack) to about:memory

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: luke, Assigned: njn)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
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.
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

8 years ago
(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.

Updated

7 years ago
Blocks: 563700
(Assignee)

Comment 3

6 years ago
Luke, are the VirtualAlloc calls in question the ones for code generated by the JITs?  If so, bug 633653 has this covered.
(Reporter)

Comment 4

6 years ago
The VirtualAlloc calls I was referring to are in StackSpace::init and StackSpace::bumpCommit.
(Assignee)

Comment 5

6 years ago
> 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.
(Assignee)

Comment 6

6 years ago
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?
(Reporter)

Comment 7

6 years ago
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.
(Assignee)

Comment 8

6 years ago
See also bug 656941, which proposed to extend the current Windows-only behaviour to all platforms.
(Assignee)

Comment 9

6 years ago
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.
Assignee: general → nnethercote
Attachment #536826 - Flags: review?(luke)
(Reporter)

Comment 10

6 years ago
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.)
(Assignee)

Comment 11

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

Comment 12

6 years ago
Comment on attachment 536826 [details] [diff] [review]
patch

r+ sans uncommittedSize() and GetJSStackSpaceUncommitted, per previous comment.
Attachment #536826 - Flags: review?(luke) → review+
(Assignee)

Comment 13

6 years ago
http://hg.mozilla.org/tracemonkey/rev/bf418b38bfe3
Whiteboard: fixed-in-tracemonkey
(Assignee)

Updated

6 years ago
Summary: report JS engine VirtualAlloc usage to about:memory → report JS engine VirtualAlloc usage (for the stack) to about:memory
hg.mozilla.org/mozilla-central/rev/bf418b38bfe3
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.