Closed
Bug 841801
Opened 12 years ago
Closed 12 years ago
GC: Allow object finalization on the background thread
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: jonco, Assigned: jonco)
Details
Attachments
(4 files, 2 obsolete files)
(deleted),
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
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)
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #715567 -
Flags: review?(wmccloskey)
Comment 3•12 years ago
|
||
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.
Assignee | ||
Comment 7•12 years ago
|
||
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)
Assignee | ||
Comment 8•12 years ago
|
||
Green try build of this lot here: https://tbpl.mozilla.org/?tree=Try&rev=7c8aa1b5c8ff
Attachment #716667 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #715566 -
Attachment is obsolete: true
Attachment #720742 -
Flags: review+
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #715567 -
Attachment is obsolete: true
Attachment #720743 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 11•12 years ago
|
||
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)
Assignee | ||
Comment 12•12 years ago
|
||
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)
Assignee | ||
Comment 13•12 years ago
|
||
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+
Assignee | ||
Comment 14•12 years ago
|
||
Comment 15•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8ac6cab88cff
https://hg.mozilla.org/mozilla-central/rev/e15520ae6a3e
https://hg.mozilla.org/mozilla-central/rev/23619e5ea880
https://hg.mozilla.org/mozilla-central/rev/8ab6bb7ecd1b
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in
before you can comment on or make changes to this bug.
Description
•