Closed Bug 1518193 Opened 2 years ago Closed 2 years ago

Free JIT LIFO data off thread


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




Tracking Status
firefox66 --- fixed


(Reporter: jonco, Assigned: jonco)



(4 files)

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.
Assignee: nobody → jcoppeard

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.

Attachment #9035264 - Flags: review?(sphink)

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

Attachment #9035265 - Flags: review?(sphink)

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.

Attachment #9035266 - Flags: review?(sphink)

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.

Attachment #9035269 - Flags: review?(sphink)
Attachment #9035264 - Flags: review?(sphink) → review+
Attachment #9035265 - Flags: review?(sphink) → review+
Attachment #9035266 - Flags: review?(sphink) → review+
Comment on attachment 9035269 [details] [diff] [review]

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.
Attachment #9035269 - Flags: review?(sphink) → review+
Pushed by
Move BackgroundSweepTask functionality into GCParallelTask base class r=sfink
Add a new parallel task for freeing LIFO data r=sfink
Use new free task to also free JIT LIFO data freed after minor GC r=sfink
Use new free task to also free nursery buffers r=sfink
You need to log in before you can comment on or make changes to this bug.