1488780 - decommit can be slow while holding the allocator lock

Status: ASSIGNED

(Core :: Memory Allocator, enhancement, P1)

Description

[Ted Campbell [:tcampbell]]

In profile from https://bugzilla.mozilla.org/show_bug.cgi?id=1488435

(In reply to Randell Jesup [:jesup] from comment #3)
> https://docs.google.com/document/d/
> 1CwPnZ7NnzzIt_UjqhaESbgWyI8VRDDzUQA_le9qaeG0/edit?usp=sharing

https://perf-html.io/public/af86f00f11f7c7719ed6018a24dab7a86ecc507b/calltree/?globalTrackOrder=0-1-2-3-4&hiddenGlobalTracks=0-1-2-3&hiddenLocalTracksByPid=2573-0&invertCallstack&localTrackOrderByPid=2573-0~&range=3.3981_8.3281~4.3286_7.4642&thread=5&v=3

Here, 10% of time is spent spinning a lock. This seems to be contention do to off-thread parse and Ion.

Comment 1

[Jon Coppeard (:jonco)]

We could experiment with using separate jemalloc arenas for LIFO allocations on helper threads.

Comment 2

[Ted Campbell [:tcampbell]]

(In reply to Jon Coppeard (:jonco) from comment #1)
> We could experiment with using separate jemalloc arenas for LIFO allocations
> on helper threads.

This seems like a good idea. Helper threads sharing an arena is probably fine since they are by definition background work.

Comment 3

[Jon Coppeard (:jonco)]

Another idea is to use multiple malloc heaps on the main thread and cycle between them so that when we free things on the background thread we don't content on the heap lock.  Background free happens separately for malloc allocations associated with nursery and tenured objects.

Comment 4

[Jon Coppeard (:jonco)]

A further idea would be to make jemalloc use more finegrained locking.  At the moment I think it's pretty coarse.

Comment 5

[Ted Campbell [:tcampbell]]

Jon, on this profile it seems pretty severe. I see 10% of mainthread time unless I'm reading this wrong. Pushing back to qf:67 is fine if we think the fix is more experimental. Thoughts?

Comment 6

[Jon Coppeard (:jonco)]

I wasn't able to reproduce that degree of contention (I only saw ~1%), but it would be great to improve the situation around malloc locking.

I can't commit to working on this right now though.

Comment 7

[Steven DeTar [:sdetar]]

Based on recent discussion with Jon and Ted, we decided to move the QF target to qf:p1:f67.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

FYI, I still see 10% in google docs:

We still experience (at least on my dual-xeon machine) ~10% jemalloc lock contention. See https://perfht.ml/2N0lzNn - during keypresses, it's even higher - https://perfht.ml/2ztMEVS with several long spins of up to 60ms while one of the JS Helpers does a LifAlloc::freeAll() (mostly in the madvise()).  Perhaps we should bump the priority of this, unless it's specific to high-core-count machines.

Note this profile is a "fast" profile where keypress time is <100ms.

Comment 9

[Nathan Froyd [:froydnj]]

Can we just use the same thread-local arena stuff that we wound up using for Stylo threads?  Or at least see if that makes any difference?

Comment 10

[Nicolas B. Pierron [:nbp]]

(In reply to Randell Jesup [:jesup] from comment #8)

We still experience (at least on my dual-xeon machine) ~10% jemalloc lock
contention. See https://perfht.ml/2N0lzNn - during keypresses, it's even
higher - https://perfht.ml/2ztMEVS with several long spins of up to 60ms
while one of the JS Helpers does a LifAlloc::freeAll() (mostly in the
madvise()).

Can we re-evaluate this observation since Bug 1489572 landed and should have highly reduced the number of these allocations.

Comment 11

[Jon Coppeard (:jonco)]

I was looking into this this week and found that when you free memory jemalloc may decide to decommit/madvise all the dirty pages for an arena in one go, with the lock held. This can be slow.

Using a separate arena for JIT data doesn't stop this happening unfortunately because the initial allocations (IonBuilder) are done on the main thread. I guess it might help as at least it wouldn't conflict with other kinds of allocation.

Ideally this decommit process would be incremental and would release the lock while calling into the OS.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Jon Coppeard (:jonco) from comment #11)

I was looking into this this week and found that when you free memory jemalloc may decide to decommit/madvise all the dirty pages for an arena in one go, with the lock held. This can be slow.

Using a separate arena for JIT data doesn't stop this happening unfortunately because the initial allocations (IonBuilder) are done on the main thread. I guess it might help as at least it wouldn't conflict with other kinds of allocation.

Ideally this decommit process would be incremental and would release the lock while calling into the OS.

glandium - any thoughts on if jemalloc could background the decommits or at least move them out from under the lock, so it would only block the thread and not other uses of jemalloc on other threads?

The profile from above (https://perfht.ml/2TH2ovO focused on an madvise() in the top js helper thread) has LifoAlloc::freeAll() -> arena_dalloc() -> Purge() (likely some others inlined there and thus missing, like DallocSmall()), -> madvise(). Perhaps Purge could move segments to a to-be-freed list, and then process them at the end of arena_dalloc() after dropping the lock? Or even fancier, a separate thread to free segments afterwards - but that's probably overkill, and might have some negative ramifications in load situations.

Comment 13

[Mike Hommey [:glandium]]

Can you try removing https://dxr.mozilla.org/mozilla-central/source/memory/build/mozjemalloc.cpp#211-215 and replace #ifdef XP_DARWIN with #if defined(XP_DARWIN) || defined(XP_LINUX) on https://dxr.mozilla.org/mozilla-central/source/memory/build/mozjemalloc.cpp#170 and get a new profile?

Comment 14

[Jason Orendorff [:jorendorff]]

I don't think this is a JS engine bug anymore. I think that aspect of it was fixed by bug 1489572.

The new idea in comment 11 should probably be a new bug, but for now I'm just moving this one -> Core :: Memory Allocator.

Comment 15

[Eric Rahm [:erahm]]

Jesup, can you try out comment 13?

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

Paul - you might want to take a look at this, and see if it's still an issue or still applies. See comment 13

Comment 17

[Paul Bone [:pbone]]

(In reply to Randell Jesup [:jesup] (needinfo me) from comment #16)

Paul - you might want to take a look at this, and see if it's still an issue or still applies. See comment 13

Thanks, I worked on some things that reduce contention in jemalloc already (Bug 1809610),
but without very significant changes decommit has to be done with the lock held. However Bug 1814815, Bug 1898646 and Bug 1843997 could all help reduce the impact.

Comment 18

[Paul Bone [:pbone]]

I've been working on some patches to perform the system calls outside of the arena lock.

Comment 19

[Paul Bone [:pbone]]

Attachment: Bug 1488780 - pt 1. Refactoring in arena_t::Purge r=glandium

Comment 20

[Paul Bone [:pbone]]

Attachment: Bug 1488780 - pt 2. Make Purge() a one-shot operation r=glandium

Comment 21

[Paul Bone [:pbone]]

Attachment: Bug 1488780 - pt 3. Move Purge() out of the arena lock r=glandium

Purge() will now take the lock itself. It must not be called with the lock
held and therefore this patch moves calls to Purge until after the lock is
released.

Comment 22

[Paul Bone [:pbone]]

Attachment: Bug 1488780 - pt 4. Split Purge() into three phases r=glandium

A future patch will lock only the first and third phases. Until then this
change will make the next few patches easier to read.

Comment 23

[Paul Bone [:pbone]]

Attachment: Bug 1488780 - pt 5. Refactor DeallocChunk r=glandium

• Rename DeallocChunk to PrepareRemoveChunk,
• Move chunk removral code into a new function RemoveChunk

Comment 24

[Paul Bone [:pbone]]

Attachment: Bug 1488780 - pt 6. Don't insert then remove an empty chunk's run r=glandium

Comment 25

[Paul Bone [:pbone]]

Attachment: Bug 1488780 - pt 7. Make Purge() and RemoveChunk() safe to run concurrently r=glandium

• Add a mDying flag to Chunks
• After Purge(), if a chunk is dying don't add it to structures such as
  mChunksMadvised
• Add a mHasBusyPages flag to Chunks
• Don't delete busy chunks
• Instead in Purge() if a chunk has a mDying set then Purge is responsible
  for finishing the deletion.

Comment 26

[Paul Bone [:pbone]]

Attachment: Bug 1488780 - pt 8. Remove busy chunks from dirty list during purge r=glandium

The easiest way to avoid multiple threads from trying to purge the same
memory is to remove the chunk being purged from the list of dirty chunks.
It can be re-added later if it has any dirty pages remaining.

Although this may prevent another thread from purging some memory, at least
immediately, it will make following patches easier to implement.

This also means that Purge() may not find a chunk to purge memory from even
if there is dirty memory.

Comment 27

[Paul Bone [:pbone]]

Attachment: Bug 1488780 - pt 9. Mark pages busy during the purge operation r=glandium

Comment 28

[Paul Bone [:pbone]]

Attachment: Bug 1488780 - pt 10. Don't allocate runs with busy pages r=glandium

• Mark runs as busy by marking their first and last page

• Remove busy runs from mAvailRuns during purge

Comment 29

[Paul Bone [:pbone]]

Attachment: Bug 1488780 - pt 11. Don't merge busy runs r=glandium

When a run of pages is freed don't merge it with adjacent busy runs

Comment 30

[Paul Bone [:pbone]]

Attachment: Bug 1488780 - pt 12. Move Coaleascing code out of DallocRun r=glandium

Comment 31

[Paul Bone [:pbone]]

Attachment: Bug 1488780 - pt 13. Coalesce runs and delete chunks in Purge() r=glandium

A chunk may have been busy and unable to be deleted or a run may have been
unable to be merged due to neighouring busy runs. So when Purge() finishes
it must check if it needs to merge runs or delete the chunk. Note that
merging a run may result in one large empty run which means the chunk must
then become the spare chunk and purge will delete the previous spare chunk -
if any.

Comment 32

[Paul Bone [:pbone]]

Attachment: Bug 1488780 - pt 14. Don't realloc into busy memory r=glandium

Comment 33

[Paul Bone [:pbone]]

Attachment: Bug 1488780 - pt 15. Only allocate into the spare chunk if it's not busy r=glandium

Comment 34

[Paul Bone [:pbone]]

Attachment: Bug 1488780 - pt 16. Unlock for decommit r=glandium

Comment 35

[Paul Bone [:pbone]]

I'm waiting for test results, when they're done they'll appear here:

Here's the correct URL: https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=e9a39c5993dc5d6deb267d05a376498102bd59ed&newProject=try&newRevision=d04985d0c83e9f10d6f4a606eaeb28dca3c5e55c&framework=13&page=1&filter=speedometer

The windows11 subtests show the biggest wins: https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=d04985d0c83e9f10d6f4a606eaeb28dca3c5e55c&originalSignature=5131097&newSignature=5131097&framework=13&application=firefox&originalRevision=e9a39c5993dc5d6deb267d05a376498102bd59ed&page=1&showOnlyConfident=1

Edit: Added correct URLs.

Comment 36

[Paul Bone [:pbone]]

Attachment: Bug 1488780 - pt 17. Release chunks outside the arena lock r=glandium

Comment 37

[Paul Bone [:pbone]]

Attachment: Bug 1488780 - pt 18. Add a hint for dirty page scanning r=glandium