1352430 - Sweep foreground objects incrementally

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[Jon Coppeard (:jonco)]

Currently all foreground objects are finalized non-incrementally in the first sweep slice for their sweep group.  This can cause pauses if there are a large number of objects to finalize.

The reason this happens because we rely on finalizers to remove references to these objects from the wrapper cache and various XPConnect tables (and probably other places too).  If we add the appropriate barriers we should be able to finalize these incrementally.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: 1-barrier-wrapper-cache WIP

Add a barrier to the wrapper cache to check for dying objects.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: 2-incrementally-finalize-objects WIP

Finalize foreground objects incrementally.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: 3-gc-finalize-callback WIP

Add another phase to the GC finalize callback.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: 4-xpconnect-sweeping WIP

Sweep XPConnect maps for dying objects.

Comment 5

[Jon Coppeard (:jonco)]

There are a bunch of places where we effectively do ExposeObjectToActive(wrapper->GetWrapperPresevingColor()) which need fixing, but otherwise this is looking promising:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=783b8a7d6e8f37357439c098dc3fbe0b4f9284d0&group_state=expanded&selectedJob=87918267

Comment 6

[Emanuel Hoogeveen [:ehoogeveen]]

I'm a bit confused by this bug. Is this about *all* foreground objects, and does that mean foregroundFinalize() is currently never called with a limited budget? I guess I never looked at the whole system when I added incrementalSweptArenas, but I assumed the budget was there for a reason.

Comment 7

[Jon Coppeard (:jonco)]

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #6)
This is specifically for foreground swept JSObjects.  Other kinds of GC thing can already be swept incrementally on the main thread.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: 1-incrementally-finalize-objects

Patch to finalize foreground JSObjects incrementally on the main thread.  This  removes the immediate finalization code and extends the incremental arena finalization code to work with JSObjects.  Order of finalization is not changed.

Debugger objects are store on a linked list and rather than put a read barrier on this I made the sweep code delete the object immediately (so these are not finalized incrementally).

Comment 9

[Jon Coppeard (:jonco)]

Attachment: 2-gc-finalize-callback

Update the finalize callback to inform clients of the stages of incremental finalization of sweep groups.  This now has three states instead of two:

  JSFINALIZE_GROUP_PREPARE -> JSFINALIZE_GROUP_START -> JSFINALIZE_GROUP_END

'prepare' to 'start' runs non-incrementally, 'start' to 'end' runs incrementally.

Comment 10

[Jon Coppeard (:jonco)]

Attachment: 3-xpconnect-sweeping

Patch to update XPConnect to handle incremental object finalization.

Dying scopes are deleted when we reach the JSFINALIZE_GROUP_END state.

XPCWrappedNative and XPCWrappedNativeProto objects are swept from the maps on the scope rather than adding read barriers.  This was done for simplicity and could potentially be improved.

I changed most of scope sweeping to only happen if we're interested in the global's zone, and added assertions that the things swept are in the same compartment as the global.

Comment 11

[Jon Coppeard (:jonco)]

Comment on attachment 8860367 [details] [diff] [review]
3-xpconnect-sweeping

Requesting review for minor js/src changes.

Comment 12

[Jon Coppeard (:jonco)]

Attachment: 5-barrier-cpows

Add a read barrier to the JavaScriptShared::cpows_ table to check for dying objects as these are currently removed by the CPOW finalizer.

Comment 13

[Jon Coppeard (:jonco)]

Attachment: 6-sweeping-zeal-mode

Add a new GC zeal mode to test incremental sweeping.  This does everything up to the start of incremental sweeping, then yields, then finishes the GC.

Comment 14

[Jon Coppeard (:jonco)]

Attachment: 4-barrier-wrapper-cache

Add a read barrier to the wrapper cache to check whether the cache contains a dying JSObject before handing it out.  I added GetWrapperMaybeDead to just get the pointer regardless, as that is needed for finalizers.

We need to be careful during finalization as the wrapper may contain the original pointer, may contain null if cleared by the read barrier and may contain a new pointer if cleared and then set to a new wrapper.  I added a version of ClearWrapper that takes a JSObject* argument and only clears the wrapper if it's still set to the original value.

Comment 15

[Jon Coppeard (:jonco)]

Comment on attachment 8860460 [details] [diff] [review]
4-barrier-wrapper-cache

Requesting additional review for js/src changes.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8860460 [details] [diff] [review]
4-barrier-wrapper-cache

>@@ -1723,17 +1723,17 @@ nsGlobalWindow::~nsGlobalWindow()
>+    JSObject *proxy = GetWrapperMaybeDead();
>     if (proxy) {
>       js::SetProxyExtra(proxy, 0, js::PrivateValue(nullptr));

but the comments on GetWrapperMaybeDead say:

> Don't store the result anywhere or pass it into JS API functions.

How is js::SetProxyExtra not a JS API function?

> +++ b/dom/base/nsWrapperCacheInlines.h
>+  if (obj && js::gc::EdgeNeedsSweepUnbarriered(&obj)) {

Correct me if I'm wrong, but EdgeNeedsSweepUnbarriered involves multiple out of line function calls and various branching, pointer-chasing, etc, right.

Have you done any performance measurement here?  I would love to see the results for this microbenchmark, with and without this patch, and with the "[Pure]" annotation for firstChild commented out in Node.webidl so the JIT doesn't optimize things away:

  <script>
  var count = 10000000;
  var node = document.documentElement;
  var start = new Date;
  var x;
  for (var i = 0; i < count; ++i) x = node.firstChild;
  var end = new Date;
  alert((end - start)/count * 1e6);
  </script>

>+    const_cast<nsWrapperCache*>(this)->ClearWrapper();

I'm not very happy with this const_cast, but don't have better ideas, short of making all the wrapper-getting bits non-const.  :(  

That said, why is this call here anyway?  Is it needed for correctness, or is it just a performance optimization?  In the latter case, is it a useful one that justifies the const_cast smell?  Needs comments at least.

>+++ b/dom/bindings/BindingUtils.h
>+  JS::AutoAssertGCCallback inCallback(obj);

This involves an extra out of line function call.  It might be ok, given we're already calling a finalizer, but the fact that we have to do a function call for debug-only code is rather annoying.  Can you please file a followup to make AutoAssertGCCallback compile out entirely in opt builds?

>+++ b/dom/bindings/Codegen.py
>+def finalizeHook(descriptor, hookName, freeOp, obj):

Care to change the descriptor.isGlobal() case to use the new arg too, instead of hardcoding "obj" as the name of the JSObject*?

Why do the ClearWrapper callsites in SandboxPrivate::ForgetGlobalObject (called from the finalizer, note) and SimpleGlobalObject::~SimpleGlobalObject not need to use the new ClearWrapper signature?  Could use at least documentation.

Comment 17

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

Comment on attachment 8860326 [details] [diff] [review]
1-incrementally-finalize-objects

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

Scary, but it makes sense so far.

Comment 18

[Andrew McCreight [:mccr8]]

Comment on attachment 8860330 [details] [diff] [review]
2-gc-finalize-callback

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +824,5 @@
> +
> +            break;
> +        }
> +        case JSFINALIZE_GROUP_START:
> +        {

For completeness, please add this here:
  MOZ_ASSERT(self->mDoingFinalization, "bad state");

Comment 19

[Andrew McCreight [:mccr8]]

Comment on attachment 8860367 [details] [diff] [review]
3-xpconnect-sweeping

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

Cool stuff. I could imagine doing something along these lines to make CC unlinking incremental. (Right now we don't do it because C++ objects can get resurrected via some of these tables.)

Is mExpandoSet already protected by MovableCellHasher stuff?

What about mXrayExpandos? I see that it gets traced so maybe that fixes it, but other fields you do explicitly update are also traced so I don't understand.

::: js/src/jsfriendapi.cpp
@@ +625,5 @@
>  
> +JS_FRIEND_API(bool)
> +js::ObjectZoneIsSweepingOrCompacting(JSObject* obj)
> +{
> +    return MaybeForwarded(obj)->zone()->isGCSweepingOrCompacting();

Does this handle null |obj| correctly? Based on the checks in the other code it looks like it might get them.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +833,5 @@
>          }
>          case JSFINALIZE_GROUP_END:
>          {
> +            // Sweep scopes needing cleanup
> +            XPCWrappedNativeScope::KillDyingScopes();

In theory, could this be done incrementally now? eg because it is okay to allow arbitrary mutator activity before we run it.

::: js/xpconnect/src/XPCWrappedNativeScope.cpp
@@ +530,5 @@
>      }
>  }
>  
> +static inline void
> +CheckSameCompartment(DebugOnly<JSCompartment*>& comp, JSObject* obj)

Maybe call this AssertSameCompartment? I think that implies DEBUG-only more strongly.

@@ +540,5 @@
> +    JSCompartment* objComp = js::GetObjectCompartment(obj);
> +    if (comp)
> +        MOZ_ASSERT(objComp == comp);
> +    else
> +        comp = objComp;

It seems overly complex to have side effects like this. Can't you just init comp to the compartment of the global and be done with it?

@@ +572,5 @@
>          // Sweep waivers.
>          if (cur->mWaiverWrapperMap)
>              cur->mWaiverWrapperMap->Sweep();
>  
> +        if (js::ObjectZoneIsSweepingOrCompacting(cur->mGlobalJSObject.unbarrieredGet())) {

I wonder how much of a speedup you get just from this check. ;)

@@ +575,5 @@
>  
> +        if (js::ObjectZoneIsSweepingOrCompacting(cur->mGlobalJSObject.unbarrieredGet())) {
> +            // Update our pointer to the global object in case it was moved or
> +            // finalized.
> +            cur->mGlobalJSObject.updateWeakPointerAfterGC();

Does the above check mean you don't need to null check mGlobalJSObject?

Comment 20

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

Comment on attachment 8860367 [details] [diff] [review]
3-xpconnect-sweeping

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

I think I've more or less convinced myself that this is all ok.

::: js/xpconnect/src/XPCWrappedNativeScope.cpp
@@ +595,5 @@
> +                auto entry = static_cast<Native2WrappedNativeMap::Entry*>(iter.Get());
> +                XPCWrappedNative* wrapper = entry->value;
> +                JSObject* obj = wrapper->GetFlatJSObjectPreserveColor();
> +                JS_UpdateWeakPointerAfterGCUnbarriered(&obj);
> +                MOZ_ASSERT(!obj || obj == wrapper->GetFlatJSObjectPreserveColor());

Wait, why? Doesn't this get invoked for a compacting GC as well?

@@ +607,5 @@
> +                auto entry = static_cast<ClassInfo2WrappedNativeProtoMap::Entry*>(i.Get());
> +                JSObject* obj = entry->value->GetJSProtoObjectPreserveColor();
> +                JS_UpdateWeakPointerAfterGCUnbarriered(&obj);
> +                CheckSameCompartment(comp, obj);
> +                MOZ_ASSERT(!obj || obj == entry->value->GetJSProtoObjectPreserveColor());

same question

@@ +613,5 @@
> +                    i.Remove();
> +            }
> +
> +            // Check for finalization of the global object and move this scope from
> +            // the live list to the dying list if necessary.

Wouldn't the logic of this function be simpler if you put the if (!cur->mGlobalJSObject) block immediately after cur->mGlobalJSObject.updateWeakPointerAfterGC(), to make it clear that that's how it can become null?

Except then you couldn't set cur to nullptr to signal that prev should not be updated. But you could just re-test if (cur->mGlobalJSObject) for that.

Or if you don't mind pointers to pointers, the full logic could be

  for (Scope** link = &gScopes; *link; ) {
      cur = *link;
      if (!ObjectZoneIsSweepingOrCompacting)
          continue;
      updateWeak();
      if (mGlobalJSObject)
          link = &cur->mNext; // Point to the next link.
      else
          *link = cur->mNext; // Update the current link to point to the next element.
      ...all the logic dealing with cur...
  }

...except that doesn't enqueue onto gDyingScopes, so the advancement logic would actually have to be

      if (mGlobalJSObject) {
          link = &cur->mNext;
      } else {
          *link = cur->mNext;
          cur->mNext = gDyingScopes;
          gDyingScopes->mNext = cur;
      }

It localizes the link-manipulation logic, it eliminates one bookkeeping variable (prev/cur/next -> link/cur), and it allows the early-continue. But it's a funky extra layer of indirection, and it's only relevant to this patch in that you had to wrap almost the whole body in an if.

(Well, and I never get linked list manipulation right on the first try, so there's probably some glaring error here.)

Comment 21

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

Comment on attachment 8860393 [details] [diff] [review]
6-sweeping-zeal-mode

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

This state management seems better.

Comment 22

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

Comment on attachment 8860460 [details] [diff] [review]
4-barrier-wrapper-cache

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

In terms of correctness, this looks ok to me. Which is surprising, I thought it'd be hairier than this.

::: dom/base/nsWrapperCache.h
@@ +106,5 @@
> +   * This getter does not check whether the wrapper is dead and in the process
> +   * of being finalized.
> +   *
> +   * This should only be called if you really need to see dead objects as part
> +   * of finalization. Don't store the result anywhere or pass it into JS API

*JSAPI. And maybe clarify what JSAPI functions are ok for finalization?

::: dom/base/nsWrapperCacheInlines.h
@@ +14,5 @@
>  inline JSObject*
> +nsWrapperCache::GetWrapperPreserveColor() const
> +{
> +  JSObject* obj = mWrapper;
> +  if (obj && js::gc::EdgeNeedsSweepUnbarriered(&obj)) {

This line makes me nervous. How fast is EdgeNeedsSweepUnbarriered? Maybe ask jmaher or something how to do a spot-check on try for talos regressions? (There's some sort of standard number of before/after runs you should do.)

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8860387 [details] [diff] [review]
5-barrier-cpows

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

::: js/ipc/WrapperOwner.cpp
@@ +918,5 @@
>  
>  void
>  WrapperOwner::drop(JSObject* obj)
>  {
> +    // The assoication may have already been swept from the table but if it's

typo

Comment 24

[Jon Coppeard (:jonco)]

Attachment: 3-xpconnect-sweeping v2

(In reply to Andrew McCreight [:mccr8] from comment #19)
Thanks for the comments.

> Is mExpandoSet already protected by MovableCellHasher stuff?

This is traced via XPCJSRuntime::TraceAdditionalNativeGrayRoots / XPCWrappedNativeScope::TraceWrappedNativesInAllScopes so doesn't need sweeping, and the MovableCellHasher stuff handles the updates yes.

> What about mXrayExpandos? I see that it gets traced so maybe that fixes it,
> but other fields you do explicitly update are also traced so I don't
> understand.

Yes, tracing should fix it.  Your comment made me realise we're unnecessarily updating mContentXBLScope and mAddonScopes so I removed this.

> In theory, could this be done incrementally now? eg because it is okay to
> allow arbitrary mutator activity before we run it.

I think they could be finalized incrementally after this point.

> It seems overly complex to have side effects like this. Can't you just init
> comp to the compartment of the global and be done with it?

Yes, good idea.

> Does the above check mean you don't need to null check mGlobalJSObject?

Yes AFAICT mGlobalJSObject can't be null for a live scope.  If this is possible then I do need to check.

I ended up refactoring this considerably following comments from you and Steve so flagging for re-review.

Comment 25

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #20)
> > +                MOZ_ASSERT(!obj || obj == wrapper->GetFlatJSObjectPreserveColor());
> 
> Wait, why? Doesn't this get invoked for a compacting GC as well?

It does but moving is handled by the objectMoved hook in this case.  I've added a comment.

I updated the patch to simplify it as suggested and use pointer-to-pointer manipulation for the list traversal.

Comment 26

[Jon Coppeard (:jonco)]

Attachment: 4-barrier-wrapper-cache v2

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #16)
> >       js::SetProxyExtra(proxy, 0, js::PrivateValue(nullptr));
> 
> but the comments on GetWrapperMaybeDead say:
> 
> > Don't store the result anywhere or pass it into JS API functions.

I've updated the comments to make this clearer.

> Correct me if I'm wrong, but EdgeNeedsSweepUnbarriered involves multiple out
> of line function calls and various branching, pointer-chasing, etc, right.

This is true.  I've changed EdgeNeedsSweepUnbarriered to do the state check on the zone inline.  This should mitigate the performance impact unless the cache contains a wrapper that is in a zone that is currently being swept (during incremenal GC).

Results for your benchmark (average of 4 runs):

  Without patch:       18.0775
  With original patch: 21.0
  With update patch:   18.425

> That said, why is this call here anyway?  Is it needed for correctness, or
> is it just a performance optimization?  In the latter case, is it a useful
> one that justifies the const_cast smell?  Needs comments at least.

It is an optimisation as the cache will eventually be cleared by the wrapper's finalizer.  It does stop us needing to check whether the object is dead after the first time if it is found to be so.  I added a comment.

Other issues addressed.

Comment 27

[Jon Coppeard (:jonco)]

Comment on attachment 8861535 [details] [diff] [review]
4-barrier-wrapper-cache v2

I moved the Zone's GC state into shadow::Zone so that we can inline the state check.

Comment 28

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

Comment on attachment 8861535 [details] [diff] [review]
4-barrier-wrapper-cache v2

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

::: dom/base/nsWrapperCacheInlines.h
@@ +18,5 @@
> +  if (obj && js::gc::EdgeNeedsSweepUnbarriered(&obj)) {
> +    // The object has been found to be dead and is in the process of being
> +    // finalized, so don't let the caller see it. As an optimisation, remove it
> +    // from the cache so we don't have to do this check in future.
> +    const_cast<nsWrapperCache*>(this)->ClearWrapper();

Just out of curiosity, have you explored if it would be better to make mWrapper mutable? It would sort of go to the other extreme, since really you want "mutable but only in this one situation". But it would require thinking less about the potential for undefined behavior.

Comment 29

[Boris Zbarsky [:bzbarsky]]

>  Without patch:       18.0775
>  With original patch: 21.0
>  With update patch:   18.425

Huh.  That's a much smaller difference than I expected, actually, and the numbers are in the right ballpark so I'm pretty sure you're measuring what I think you're measuring.  Great.  :)

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8861535 [details] [diff] [review]
4-barrier-wrapper-cache v2

r=me on the DOM bits

Comment 31

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #28)
> Just out of curiosity, have you explored if it would be better to make
> mWrapper mutable? It would sort of go to the other extreme, since really you
> want "mutable but only in this one situation". But it would require thinking
> less about the potential for undefined behavior.

What's the undefined behaviour here - is it modifying a |const WrapperCache|?  If so I think this is possible if the member is mutable too.  I don't know whether that counts as UB though.

Anyway I think having const_cast in one place is probably better than making the member mutable because it's localised, as long as there's no undefined behaviour.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b56c9e5ce7fe
Finalize foreground objects incrementally r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/42dcd77916bf
Update the GC finalize callback to communicate the new state r=sfink r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/09be4ae7bbf0
Update XPConnect sweeping to handle incrementally finalized objects r=mccr8 r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/0203cc1f2d2f
Add barrier to wrapper cache to clear dying objects that have not yet been finalized r=bz r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/8de58bbbb392
Add barrier to CPOWs table to remove dying objects r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c1eb5975702
Add zeal mode to exercise incremental foreground finalization r=sfink

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b56c9e5ce7fe
https://hg.mozilla.org/mozilla-central/rev/42dcd77916bf
https://hg.mozilla.org/mozilla-central/rev/09be4ae7bbf0
https://hg.mozilla.org/mozilla-central/rev/0203cc1f2d2f
https://hg.mozilla.org/mozilla-central/rev/8de58bbbb392
https://hg.mozilla.org/mozilla-central/rev/3c1eb5975702