1518193 - Free JIT LIFO data off thread

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P3)

Description

[Jon Coppeard (:jonco)]

The GC keeps track of some JIT LIFO data that is only safe to free after collecting the nursery (GCRuntime::blocksToFreeAfterMinorGC).  Currently this is freed on the main thread, but that shows up in some profiles.

We should be able to free this off thread, possibly in the same way as we do for GCRuntime::blocksToFreeAfterSweeping.

Also, we might want to try freeing this data at the end of the first GC slice since I think discarding JIT code will add blocks here.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1518193-refactor-parallel-tasks

Refactor GCParallelTask to move a couple of methods from BackgroundSweepTask here, so that can be used by other parallel tasks. This is in preparation for adding a similar background free task.

This removes BackgroundSweepTask::done and instead adds an extra optional Finishing state to GCParallelTask.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1518193-add-free-task

Patch to add BackgroundFreeTask and move freeing of LIFO data there. I also renamed a few things and did some minor refactoring.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1518193-free-jit-data-off-thread

Patch to merege blocks to be freed after minor GC into the main LIFO blocks to free structure after minor GC.

Removed references to 'afterSweeping' for the other LIFO blocks because there's no requirement to free them after sweeping and they are all unused at the point they are queued.

Also kicked off a free task after discarding JIT code at the start of GC since this will queue data to be freed. We are also 'after minor GC' at this point because we always do one at the start of GC.

I moved setting the sweepOnBackgroundThread into the initial GC state because I think it makes more sense there, although it didn't turn out to be relevant for this patch.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1518193-free-nursery-buffers

We can also use this mechanism to free nursery buffers so that we don't kick off two parallel tasks that will both content on the jemalloc lock after every minor GC.

Comment 5

[Steve Fink [:sfink] [:s:]]

Comment on attachment 9035269 [details] [diff] [review]
bug1518193-free-nursery-buffers

Review of attachment 9035269 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! All the pieces more or less made sense to me individually. Hopefully TSAN will say if I missed anything major.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86f8236cd20f
Move BackgroundSweepTask functionality into GCParallelTask base class r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bdbec17ea7c
Add a new parallel task for freeing LIFO data r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d9e12dcfe7f
Use new free task to also free JIT LIFO data freed after minor GC r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2a84a3dcae0
Use new free task to also free nursery buffers r=sfink

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/86f8236cd20f
https://hg.mozilla.org/mozilla-central/rev/1bdbec17ea7c
https://hg.mozilla.org/mozilla-central/rev/7d9e12dcfe7f
https://hg.mozilla.org/mozilla-central/rev/d2a84a3dcae0