Closed Bug 841801 Opened 9 years ago Closed 9 years ago

GC: Allow object finalization on the background thread


(Core :: JavaScript Engine, enhancement)

Not set





(Reporter: jonco, Assigned: jonco)



(4 files, 2 obsolete files)

At the moment, any JS object with a finalizer is finalized non-incrementally in the first sweep slice for its compartment/zone.  Some finalizers need to run at this time, but some may be safely run on the background thread.  Doing this would reduce GC pause times.

We can add a flag to the Class structure that indicates that objects of that class can be finalized on the background thread.
Add a JSClass flag to indicate that objects of this class that have a finalizer support running of the finalizer on the background thread.
Attachment #715566 - Flags: review?(wmccloskey)
Attachment #715567 - Flags: review?(wmccloskey)
This is pretty awesome, but it's going to badly break the assumptions that generational GC is working on at the moment. Specifically, we plan to not call finalizers for things in the nursery so that minor collection time stays proportional to the number of live objects.

The problem is that we need to make the call on whether to allocate something in the Nursery or Tenured in NewGCThing where we don't currently have access to the Class. We do have infrastructure to allocate specific objects in the Tenured by passing a flag to NewGCThing, so it should be possible to solve this right now by just updating the creation sites. I don't really like this solution, however, since eventually we'd like to have a second nursery for things with finalizers.

I think perhaps we need to update the object creation paths to take something more abstract than the AllocKind so that we can encode this sort of knowledge from the creation site. We could also fold InitialHeap into this and get rid of that usually-unused parameter as well.

This is all pretty far outside the scope of this bug however, so if you want to just force these objects to start Tenured, that would be fine for now.
Comment on attachment 715566 [details] [diff] [review]
1 - Enable background finalization of objects

Review of attachment 715566 [details] [diff] [review]:

::: js/src/jsobjinlines.h
@@ +233,5 @@
>  JSObject::finalize(js::FreeOp *fop)
>  {
>      js::Probes::finalizeObject(this);
>      if (!IsBackgroundFinalized(getAllocKind())) {

This should go in an #ifdef DEBUG block to make sure it doesn't slow us down.

@@ +241,2 @@
> +    /* Finalize obj first, in case it needs map and slots. */

This comment is no longer relevant; please remove.
Attachment #715566 - Flags: review?(wmccloskey) → review+
Comment on attachment 715567 [details] [diff] [review]
2 - Turn on background finalization for a few classes

Review of attachment 715567 [details] [diff] [review]:

One scary thought that occurred to me here is that some of these finalizers might invoke write barriers. That would be bad because we decide whether to invoke a barrier by looking at the arenaheader, and that might have been freed already. This isn't a problem during normal finalization because we don't free any chunks until the end of the GC.

For example, I'm pretty sure that the map and set objects will invoke write barriers on any values that were present in the table. Did the measurements you did allow you to quantify how important each of these is? In that case we could audit the important ones more carefully for write barrier invocations and leave the rest along.
Attachment #715567 - Flags: review?(wmccloskey)
Also, we should address the problem with generational GC. Basically, you need to ensure that any objects with finalizers end up in the tenured heap. I don't think this should be too hard.
Is this something like what you meant?

I made InitialHeapForNewKind take a class (and renamed it) to calculate the heap to use, and added assertions to JSObject::create JSObject::createArray.
Attachment #716667 - Flags: review?(wmccloskey)
Green try build of this lot here:
Attachment #716667 - Flags: review?(wmccloskey) → review+
Attachment #715566 - Attachment is obsolete: true
Attachment #720742 - Flags: review+
Attachment #715567 - Attachment is obsolete: true
Attachment #720743 - Flags: review?(wmccloskey)
Allow proxies to be finalized in the background where possible.  Some proxies required finalization on the main thread.  We also need to make sure cross compartment wrappers need to have the same alloc kind as any other object they may be transplanted with.
Attachment #720746 - Flags: review?(wmccloskey)
Comment on attachment 720742 [details] [diff] [review]
1 - Enable background finalization of objects

You reviewed this already.  This version has comments addressed and has been rebased after a catchup.
Attachment #720742 - Flags: review+ → review?(wmccloskey)
Some rough stats from testing this:

Proportion of objects that have finalizers: 11% - 17%
Of those, proportion now finalized in background with this patch: 25 - 33%
Attachment #720742 - Flags: review?(wmccloskey) → review+
Attachment #720743 - Flags: review?(wmccloskey) → review+
Attachment #720746 - Flags: review?(wmccloskey) → review+
Depends on: 852370
No longer depends on: 852370
You need to log in before you can comment on or make changes to this bug.