Track the amount of malloc memory precisely

NEW
Assigned to

Status

()

enhancement
P3
normal
2 years ago
2 days ago

People

(Reporter: jonco, Assigned: jonco, NeedInfo)

Tracking

(Blocks 1 bug, {leave-open})

55 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 affected)

Details

Attachments

(33 attachments)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Assignee

Description

2 years ago
At the moment we have counters for malloced memory that count down to zero from a set threshold when memory is allocated.  When they hit zero we trigger a GC.  We have one for the runtime and one per zone.

There are several problems with this:
 - it doesn't reflect freed memory so we may trigger a GC when there is nothing to collect
 - the threshold doesn't depend on the size of the heap so we may trigger GCs more frequently than necessary for large heaps
 - we lose information when we merge zones (bug 1341093)
 - they work differently from the GC memory counters which is confusing

We should make these counters track allocated size exactly, the same as we do for GC allocated memory.

This is a large project and not all of the details are clear.  In particular it's not clear whether we should make all our free methods take a size in bytes or rely on the malloc implementation (e.g. jemalloc) to get this information.
Priority: -- → P3
Assignee

Updated

3 months ago
Blocks: GCScheduling
Assignee

Updated

2 months ago
Depends on: 1536154
Assignee

Updated

Last month
Depends on: 1550009
Assignee

Updated

Last month
Keywords: leave-open
Assignee

Comment 1

Last month

I'm going to start posting patches and hope to land them incrementally.

The general strategy is to add APIs to tell the GC about malloc memory associated with a cell:

  void AddCellMemory(gc::Cell* cell, size_t nbytes, MemoryUse use);
  void RemoveCellMemory(gc::Cell* cell, size_t nbytes, MemoryUse use)

Calls to these must be balanced and this is checked in debug builds.

FreeOp is updated with methods to call RemoveCellMemory and free. The long term aim is to replace all use of the current FreeOp::free_/delete_ with these methods to ensure all memory is tracked.

To start with I'm going to add tracking for memory which can contribute significantly to the total, e.g. is very frequently used or has a size which is under user control.

Assignee

Comment 5

Last month

This adds memory tracking for string contents while leaving the current scheme in place for the time being.

Depends on D30516

Comment 6

Last month
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/772b3ec0102d
Add APIs to track internal memory assocated with GC things r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/95399bf8d949
Add FreeOp methods free memory and update memory accounting r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/54227b612212
Track malloc memory associated with array buffers r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/01140845cccc
Track malloc memory associated with strings r=jandem
Assignee

Comment 8

29 days ago

Since we now have precise memory accounting for externally allocated memory associated with GC things we should be able to remove use of the existing malloc counter here. This should help with cases where we trigger too many GCs because we think there is more memory associated than there really is.

Comment 9

28 days ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/441a4c70ac22
Remove existing malloc accounting for externally allocated memory r=sfink?

== Change summary for alert #20930 (as of Mon, 13 May 2019 11:46:58 GMT) ==

Improvements:

4% raptor-tp6-twitter-firefox loadtime windows7-32-shippable opt 257.33 -> 246.29

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=20930

Assignee

Comment 12

27 days ago

This code is debug-only, but it seems a shame to waste have this key structure take up two words when it will fix into one. This also moves the MemoryUse enum definition to gc/GCEnum.h.

Assignee

Comment 13

27 days ago

This adds memory tracking for object elements. I had to swap the association in JSObject::swap. I also moved NativeObject::shrinkCapacityToInitializedLength out of line so as not to have to pull Zone.h into NativeObject.h. I don't know how performance sensitive this is - if it is I could look at this again.

Depends on D32170

Comment 15

26 days ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/740009023b1b
Pack MemoryTracker hashtable keys into a single word on 64bit platforms r=sfink
Attachment #9066774 - Attachment description: Bug 1395509 - Track malloc memory associated with JSObject slots r=jandem? → Bug 1395509 - Track malloc memory associated with JSObject slots r=jandem

Comment 18

21 days ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/94dadeeee608
Track malloc memory associated with JSObject elements r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/92fc3159f152
Track malloc memory associated with JSObject slots r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/5585ac78378a
Track malloc memory associated with JSScripts r=tcampbell
Assignee

Comment 20

20 days ago

This changes ZoneAllocPolicy to use the new precise memory tracking rather than the old malloc counter. This works because the mozilla standard containers (HashMap, Vector, etc) already call the AllocyPolicy free_ method with the correct size. This patch tracks individual instances of ZoneAllocPolicy to check that the numbers balance.

Depends on D33000

Comment 21

19 days ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b37b6c015168
Add an alloc policy that tracks malloc memory associated with GC things r=sfink
Assignee

Comment 22

19 days ago

This refactors the malloc allocation tracking so that the MemoryTracker object is only present in debug builds and the count of malloc bytes is kept separately in the Zone. This makes things a little clearer I think and removes one level of inlining.

Assignee

Comment 23

19 days ago

This splits out the allocation functions and allocation tracking state into a new base class, ZoneAllocator, which lives in its own header file. We can include this for the common case of allocating malloc memory for GC things without dragging in the full complexity of Zone.h.

Depends on D33179

Comment 24

18 days ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50769742e295
Refactor malloc allocation tracking r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4daa44cdb9c
Split out zone memory allocation framework into separate base class r=sfink

Comment 27

15 days ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/351d19c8bdd3
Add APIs to handle memory accounting while initializing object pointers to malloc memory r=sfink?

Comment 29

14 days ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ab12b34ff823
Add APIs to handle memory accounting while initializing object pointers to malloc memory r=sfink?
Assignee

Comment 31

12 days ago

The change to split out ZoneAllocator messed up the MemoryTracker code that prints out what failed to be removed by making it run after the Zone destructor which will already assert in the case (but without printing useful information first).

Assignee

Comment 33

11 days ago

Previously I rolled the malloc byte count into a total byte count for each zone but this may adversely affect GC scheduling (e.g. by triggering more non-incremental GCs because allocation volumes appear higher with this change). So that we can land this machinery without disturbing benchmarks too much, this patch splits out the new malloc memory accounting into a separate counter and uses the maxMallocBytes setting as the threshold (default value is 128MB vs 30MB for the GC heap threshold). This should make the behaviour closer to the original behaviour for now. We can go back and adjust the parameters later to obtain the desired behaviour.

Depends on D34180

Assignee

Updated

8 days ago
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
Assignee

Comment 34

8 days ago

Use memory tracking APIs to track malloc memory associated with Map and Set objects.

Depends on D34181

Assignee

Comment 35

8 days ago

Use memory tracking APIs to track malloc memory associated with BigInts.

Depends on D34371

Assignee

Comment 36

8 days ago

Use memory tracking APIs to track malloc memory associated with Scopes.

Depends on D34373

Assignee

Comment 37

8 days ago

Use memory tracking APIs to track malloc memory associated with the different ctypes objects.

I ended up creating new public APIs because ctypes currently mostly uses our public APIs, but I actaully don't know why. I don't think it can be built standalone. Maybe this should use the internal APIs instead.

Depends on D34374

Assignee

Comment 38

8 days ago

Use memory tracking APIs to track malloc memory associated with weak maps.

Depends on D34375

Assignee

Comment 39

8 days ago

Use memory tracking APIs to track malloc memory associated with Shapes. I had to thread FreeOp and BaseShape through in various places so there's enough context.

Depends on D34376

Comment 40

7 days ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3e11ae5fb19
Fix assertions that all tracked memory is removed when a zone is collected r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/06ef8a540487
Move HeapSize class to gc/Scheduling.h where it belongs r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b3207f97971
Add a separate byte count for malloc allocations r=sfink
Assignee

Comment 41

7 days ago

Use memory tracking APIs to track malloc memory associated with module IndirectBindingMap. These are attached to ModuleObjects and ModuleNamespaceObjects.

Depends on D34377

Assignee

Comment 42

7 days ago

Use memory tracking APIs to track malloc memory associated with IonScripts and BaselineScripts. This does the memory accounting when they are attached/detached from the JSScript rather than on initialisation/finalization as normally. I had to record the allocation size in the objects themselves. Hopefully this isOK.

Depends on D34551

Assignee

Comment 43

7 days ago

Use memory tracking APIs to track malloc memory associated with ArgumentsObjects. This one is slightly more complicated because we can allocate these in the nursery and malloc memory is not tracked for nursery objects, so we have to do this if they get tenured.

Depends on D34552

Assignee

Comment 44

7 days ago

Use memory tracking APIs to track malloc memory associated with NativeIterator objects. I had to store the initial propery count as propertiesEnd can change during the lifetime of this object.

Depends on D34553

Assignee

Comment 46

7 days ago

I wanted to make VectorMatchPairs use ZoneAllocPolicy but this is also used in a bunch of places where it's not attached to a GC thing, so I left this as a todo.

Depends on D34555

Comment 48

6 days ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/059c1897eb8f
Track malloc memory used by JS Map and Set objects r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/db2d48f8abf1
Track malloc memory associated with BigInts r=pbone
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4f684303335
Track malloc memory associated with Scopes r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/0943e5feaa8d
Track malloc memory associated with ctypes objects r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a2985fdef25
Track malloc memory associated with WeakMap objects r=pbone
https://hg.mozilla.org/integration/mozilla-inbound/rev/82c5c40a8881
Track malloc memory used by shapes r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9c24c9e108c
Track malloc memory used by IndirectBindingMaps associated with modules r=pbone
Assignee

Comment 49

6 days ago

Some kinds of memory use (e.g. reg exp byte code) require multiple memory associations for a single cell. This patch adds machinery to let that happen for specific uses.

Depends on D34556

Assignee

Comment 50

6 days ago

I had to store the length as the first word of the bytecode. Hopefully this is not too onerous. I also shuffled RegExpShared fields around to make the class a little smaller.

Depends on D34723

Assignee

Comment 51

6 days ago

Note that we only track the for typed arrays that are not backed by an ArrayBuffer as the memory is tracked there from then on.

Depends on D34726

Assignee

Comment 52

6 days ago

I'm not sure what these are, but track them anyway.

Depends on D34729

Assignee

Comment 53

6 days ago

This changes the format of the trace list from using -1 as a delimter to storing the list lengths up front so that we have length information.

Depends on D34730

Comment 55

4 days ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa841547378a
Track malloc memory used by JIT scrips r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/10cde525f0c8
Track malloc memory used by Arguments objects r=sfink

Comment 56

4 days ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fca3e8b8e5e
Track malloc memory used by FileObject objects r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/37f4b005af23
Track malloc memory used by RegExpStatics objects r=pbone
https://hg.mozilla.org/integration/mozilla-inbound/rev/d37bdcb35091
Allow multiple associations for some memory uses r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6850f7bb6b6
Track malloc memory used by RegExp bytecode r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d720afe50ea
Track malloc memory used by non-inline TypedArray elements r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7492a7c1b7d
Track malloc memory used by PerfMesaurement objects r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/9de6fe9f087e
Track malloc memory used by TypedObject trace lists r=sfink

Comment 57

4 days ago
Backout by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8a83a5f0fb9
Backed out 7 changesets for SM build failures on a CLOSED TREE.

Backed out 7 changesets (bug 1395509) for SM build failures on a CLOSED TREE.

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/d8a83a5f0fb9165f97d8000976bb2241d44cda34

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&revision=9de6fe9f087eb386f829300206084da2a0861ea3&selectedJob=251889998

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=251889998&repo=mozilla-inbound&lineNumber=2659

Log snippet:

[task 2019-06-14T13:48:17.817Z] File "/usr/lib/python2.7/subprocess.py", line 540, in check_call
[task 2019-06-14T13:48:17.817Z] raise CalledProcessError(retcode, cmd)
[task 2019-06-14T13:48:17.817Z] subprocess.CalledProcessError: Command '['/builds/worker/workspace/obj-haz-shell/dist/bin/js', '/builds/worker/checkouts/gecko/js/src/devtools/rootAnalysis/computeGCTypes.js', 'gcTypes.tmp', 'typeInfo.tmp']' returned non-zero exit status -11
[task 2019-06-14T13:48:17.861Z] Loaded /builds/worker/checkouts/gecko/js/src/devtools/rootAnalysis/t/out/suppression/defaults.py
[task 2019-06-14T13:48:17.861Z] Running gcTypes to generate ('gcTypes.txt', 'typeInfo.txt')
[task 2019-06-14T13:48:17.861Z] PATH="/builds/worker/workspace/sixgill/usr/bin:${PATH}" SOURCE='/builds/worker/checkouts/gecko/js/src/devtools/rootAnalysis/t/suppression' ANALYZED_OBJDIR='/builds/worker/checkouts/gecko/js/src/devtools/rootAnalysis/t/out/suppression' LD_LIBRARY_PATH="${LD_LIBRARY_PATH}:/builds/worker/workspace/obj-haz-shell/dist/bin" XDB='/builds/worker/workspace/sixgill/usr/bin/xdb.so' /builds/worker/workspace/obj-haz-shell/dist/bin/js /builds/worker/checkouts/gecko/js/src/devtools/rootAnalysis/computeGCTypes.js gcTypes.txt typeInfo.txt
[task 2019-06-14T13:48:17.887Z] Traceback (most recent call last):
[task 2019-06-14T13:48:17.887Z] File "/builds/worker/checkouts/gecko/js/src/devtools/rootAnalysis/analyze.py", line 339, in <module>
[task 2019-06-14T13:48:17.887Z] run_job(step, data)
[task 2019-06-14T13:48:17.887Z] File "/builds/worker/checkouts/gecko/js/src/devtools/rootAnalysis/analyze.py", line 209, in run_job
[task 2019-06-14T13:48:17.887Z] subprocess.check_call(command, env=env(config))
[task 2019-06-14T13:48:17.887Z] File "/usr/lib/python2.7/subprocess.py", line 540, in check_call
[task 2019-06-14T13:48:17.887Z] raise CalledProcessError(retcode, cmd)

Flags: needinfo?(jcoppeard)
Assignee

Comment 59

4 days ago

(In reply to Raul Gurzau (:RaulGurzau) from comment #58)
Steve do you know what happened here? It seems my patches broke the hazard analysis task somehow. I wasn't able to see anything useful in the logs. I'm currently trying to bisect this on try.

Flags: needinfo?(jcoppeard) → needinfo?(sphink)

Comment 60

3 days ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3436ffbb75c
Track malloc memory used by RegExpStatics objects r=pbone
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc1b32cdf4ca
Allow multiple associations for some memory uses r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/54817eb3c564
Track malloc memory used by RegExp bytecode r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/848bf60827e4
Track malloc memory used by non-inline TypedArray elements r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/219909ea3e4f
Track malloc memory used by PerfMesaurement objects r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/19f38c9cdd67
Track malloc memory used by TypedObject trace lists r=sfink
You need to log in before you can comment on or make changes to this bug.