1993212 - Investigate allocating WeakMap memory with the buffer allocator

Status: RESOLVED FIXED

(Core :: JavaScript: GC, task, P2)

Description

[Jon Coppeard (:jonco)]

This should make things more efficient.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Bug 1993212 - Part 1: Rename reportAllocationOverflow methods to reportAllocOverflow r?jandem

According to mfbt/AllocPolicy.h, allocation policies are supposed to implement
a 'reportAllocOverflow' method. This is used both by the implmementation of the
alloc policy itself and also by containers such as mfbt/HashTable.h when
growing its table.

Our MallocProvider mixin class implements most of the AllocPolicy methods for
a client class, but requires clients to provide a 'reportAllocationOverflow'
method.

Thus clients must provide both methods if they want to be used by containers.
Currently only TrackedAllocPolicy (a.k.a. ZoneAllocPolicy) does this, by having
reportAllocOverflow call reportAllocationOverflow.

The current situation is confusing. We can simplify this by making
MallocProvider use the reportAllocOverflow name and renaming to this
everywhere.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1993212 - Part 2: Refactor JS alloc policy header to provide a base class for alloc policies r?jandem

This refactoring gives us a place to put default method definitions without
adding them to every policy.

The patch also renames some alloc policy classes to make it clear they use malloc allocation.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Bug 1993212 - Part 3: Wait for background GC activity to finish if buffer alloc fails r?jandem

This makes handling OOM simpler in the next patch because we don't need to
retry at a higher level.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1993212 - Part 4: Add a way to trace memory allocations owned by standard containers r=jandem!,glandium!

To support using GC memory in containers we need a way to trace that memory.
This patch adds a 'getExternalAllocPtr' method to vector and hash table containers
that can be used to get the pointer to trace.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Bug 1993212 - Part 5: Add BufferAllocPolicy to allow containers to allocate their memory with the buffer allocator r?jandem

The patch adds BufferAllocPolicy which allocates memory using the buffer
allocator and provides a traceExternalAlloc method to trace the allocations, as
well as updating its knowledge of whether the owning cell is tenured.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Bug 1993212 - Part 6: Allow multi-threaded buffer allocator use while holding a lock r?jandem

Previously calls to the buffer allocator to allocate/free memory were always
done from the main thread and so no locking was required. GC sweeping can be
split between more than one helper thread so we need a way to make this thread
safe.

The patch adds a way to set up debug build assertions in the buffer allocator
by passing a mutex that the caller must hold. The caller is responsible for
actually taking/releasing the lock.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: Bug 1993212 - Part 7: Take lock when resizing weak map hash table r?jandem

This sets up multi-threaded use of the buffer allocator during sweep slices
using an existing mutex. While sweeping weak maps the mutex is only held when
destroying the weak map iterator as this is the part that may resize the table
and hence call into the allocator. This is also the part required by existing
use of the lock for weak map sweeping.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: Bug 1993212 - Part 8: Rename store buffer lock to sweeping lock r?jandem

Since this mutex is now used for more than synchronizing access to the store
buffer it is renamed to the sweeping lock.

Comment 9

[Jon Coppeard (:jonco)]

Attachment: Bug 1993212 - Part 9: Change WeakMap constructor to either take zone or context and owning JS object r?jandem

Currently the memOf parameter is optional for both constructors. This patch
changes it so that for a weak map with an owning JSObject you need to pass
context and owning object, and for non-owned ones you only pass the zone.

Comment 10

[Jon Coppeard (:jonco)]

Attachment: Bug 1993212 - Part 10: Use the buffer allocator for JS WeakMap/Set objects r?jandem

Finally we can switch over to using the buffer allocator for weak maps, both
the WeakMap allocation and the hash table memory via BufferAllocPolicy.

WeakMap objects remain tenured. I haven't thought about whether it's worth
trying to nursery allocate these.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/250fe02414bf
https://hg.mozilla.org/integration/autoland/rev/2e3b8c140e2d
Part 1: Rename reportAllocationOverflow methods to reportAllocOverflow r=jandem
https://github.com/mozilla-firefox/firefox/commit/2a1da2346de4
https://hg.mozilla.org/integration/autoland/rev/e3d7bffe9f70
Part 2: Refactor JS alloc policy header to provide a base class for alloc policies r=jandem
https://github.com/mozilla-firefox/firefox/commit/c2825af16cda
https://hg.mozilla.org/integration/autoland/rev/5af2812721e3
Part 3: Wait for background GC activity to finish if buffer alloc fails r=jandem
https://github.com/mozilla-firefox/firefox/commit/d6b644ec4c0e
https://hg.mozilla.org/integration/autoland/rev/faad163a7761
Part 4: Add a way to trace memory allocations owned by standard containers r=jandem,glandium
https://github.com/mozilla-firefox/firefox/commit/5a084af149b1
https://hg.mozilla.org/integration/autoland/rev/6a5acd5de294
Part 5: Add BufferAllocPolicy to allow containers to allocate their memory with the buffer allocator r=jandem
https://github.com/mozilla-firefox/firefox/commit/c26184fd278d
https://hg.mozilla.org/integration/autoland/rev/c567d6ede8bd
Part 6: Allow multi-threaded buffer allocator use while holding a lock r=jandem
https://github.com/mozilla-firefox/firefox/commit/3213a523220a
https://hg.mozilla.org/integration/autoland/rev/dd65634bd8c6
Part 7: Take lock when resizing weak map hash table r=jandem
https://github.com/mozilla-firefox/firefox/commit/1ca6b92089c7
https://hg.mozilla.org/integration/autoland/rev/fa30081c9d70
Part 8: Rename store buffer lock to sweeping lock r=jandem
https://github.com/mozilla-firefox/firefox/commit/0f30cbbbbffc
https://hg.mozilla.org/integration/autoland/rev/7e5d588bf5c2
Part 9: Change WeakMap constructor to either take zone or context and owning JS object r=jandem
https://github.com/mozilla-firefox/firefox/commit/bb10751894f1
https://hg.mozilla.org/integration/autoland/rev/b532f766ba6f
Part 10: Use the buffer allocator for JS WeakMap/Set objects r=jandem

Comment 12

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/2e3b8c140e2d
https://hg.mozilla.org/mozilla-central/rev/e3d7bffe9f70
https://hg.mozilla.org/mozilla-central/rev/5af2812721e3
https://hg.mozilla.org/mozilla-central/rev/faad163a7761
https://hg.mozilla.org/mozilla-central/rev/6a5acd5de294
https://hg.mozilla.org/mozilla-central/rev/c567d6ede8bd
https://hg.mozilla.org/mozilla-central/rev/dd65634bd8c6
https://hg.mozilla.org/mozilla-central/rev/fa30081c9d70
https://hg.mozilla.org/mozilla-central/rev/7e5d588bf5c2
https://hg.mozilla.org/mozilla-central/rev/b532f766ba6f

Comment 13

[Mayank Bansal]

0.3% improvement on Base content Js on OSX

Comment 14

[Acasandrei Beatrice (needinfo me)]

Perfherder has detected a awsy performance change from push b532f766ba6fe70f6b28353aa4d34ef732fa34e6.

No action is required from the author; this comment is provided for informational purposes only.

Improvement	Test	Platform	Options	Absolute values [old vs new]
0.27%	Base Content JS (doc)	macosx1470-64-shippable	fission	1,516,248.00 bytes -> 1,512,184.00 bytes

Need Help or Information?

If you have any questions, please reach out to [e-mail]. Alternatively, you can find help on Slack by joining #perf-help, and on Matrix you can find help by joining #perftest.

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.