Closed Bug 1337070 Opened 3 years ago Closed 3 years ago

Tolerate multiple zone groups and cooperating contexts in the GC

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bhackett, Assigned: bhackett)

References

Details

Attachments

(4 files)

Attached patch patchSplinter Review
There are quite a few places where the GC assumes there is a single zone group and main thread JSContext in a runtime.  Fixing this for a cooperatively scheduled multithreaded runtime mainly involves simple mechanical changes, since threads can look at the state of other suspended threads if they need to GC.
Attached patch nursery changesSplinter Review
Some changes for nurseries and minor GCs, mainly for moving the minor GC request information to the per-zone-group nursery from the per-runtime GCRuntime, since when we request a minor GC we always know which zone group we want to GC.
Attachment #8834079 - Flags: review?(jcoppeard)
The list of callbacks to invoke after a minor GC is on the zone group.  This means that when we delete an object that needs to be delayed until after a minor GC, we need to know which zone group to associate the delete callback with.  This changes users of GCManagedDeletePolicy to have a zone() method the policy can call.  It also removes a confusing bit in a jsapi-test that deletes a heap allocated GCPtr, when the comment on GCPtr specifies it must have a GC lifetime.
Attachment #8834081 - Flags: review?(jcoppeard)
Changes for tracing and major GCs.  These are mainly for iterating over all zone groups when we need to do something to each of them, and likewise for iterating over all cooperative contexts when necessary for checking stack roots and so forth.
Attachment #8834084 - Flags: review?(jcoppeard)
Attachment #8834079 - Flags: review?(jcoppeard) → review+
Comment on attachment 8834081 [details] [diff] [review]
delete policy changes

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

::: js/src/jsapi-tests/testGCHeapPostBarriers.cpp
@@ -77,5 @@
>  bool
>  TestHeapPostBarriersForType()
>  {
>      CHECK((TestHeapPostBarriersForWrapper<T, JS::Heap<T*>>()));
> -    CHECK((TestHeapPostBarriersForWrapper<T, js::GCPtr<T*>>()));

Please don't delete this test.  Can you add a comment instead that says we're bending the rules for testing purposes?
Attachment #8834081 - Flags: review?(jcoppeard) → review+
(In reply to Jon Coppeard (:jonco) from comment #4)
> Comment on attachment 8834081 [details] [diff] [review]
> delete policy changes
> 
> Review of attachment 8834081 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsapi-tests/testGCHeapPostBarriers.cpp
> @@ -77,5 @@
> >  bool
> >  TestHeapPostBarriersForType()
> >  {
> >      CHECK((TestHeapPostBarriersForWrapper<T, JS::Heap<T*>>()));
> > -    CHECK((TestHeapPostBarriersForWrapper<T, js::GCPtr<T*>>()));
> 
> Please don't delete this test.  Can you add a comment instead that says
> we're bending the rules for testing purposes?

The problem I ran into is that GCPtr has a managed delete policy, which now means it needs to have a zone() method.  Is there a good way to get the zone for a generic thing?  I could add one that works well enough to handle the types used in this test, but I deleted this because it seemed like a lot of bother to handle something that according to the comment isn't supported by the API.
Comment on attachment 8834084 [details] [diff] [review]
tracing/major GC changes

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

Looks good!

::: js/src/jsgc.cpp
@@ +6195,5 @@
>  
>  } /* anonymous namespace */
>  
> +static void
> +EvictAllNurseries(JSRuntime* rt, JS::gcreason::Reason reason)

It looks like this could be defined in a header file as it's used in a few places.
Attachment #8834084 - Flags: review?(jcoppeard) → review+
(In reply to Brian Hackett (:bhackett) from comment #5)
> The problem I ran into is that GCPtr has a managed delete policy, which now
> means it needs to have a zone() method.

Oh, right, that's annoying.  In that case, can you comment the test out and add a TODO, and I'll fix it?
Depends on: 1337365
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f4074b2c8e3
Tolerate multiple zone groups and cooperating contexts in the GC, r=jonco.
https://hg.mozilla.org/mozilla-central/rev/6f4074b2c8e3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.