Bug 223629 – [JSC] Functor for WeakGCMap::ensureValue must not invoke GC

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Functor for WeakGCMap::ensureValue must not invoke GC

[Yusuke Suzuki]

Created attachment 423997 [details]
Patch

[Yusuke Suzuki]

<rdar://problem/75619217>

[Yusuke Suzuki]

Comment on attachment 423997 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423997&action=review

> Source/JavaScriptCore/runtime/WeakGCMap.h:73
> +        // If functor invokes GC, GC can prune WeakGCMap, and manipulate HashMap while we are touching it in ensure function.
> +        // The functor must not invoke GC.
> +        DisallowGC disallowGC;

I do not want to put DeferGC here since we would like to be aware about the scope of DeferGC explicitly. Deferring GC is not generally good. We would like to invoke GC responsively.
If we use DeferGC, we should make DeferGC scope small, and we should be aware of that.

[Yusuke Suzuki]

Created attachment 423998 [details]
Patch

[Mark Lam]

Comment on attachment 423998 [details]
Patch

r=me.  Please make sure that the EWS failures are not real.  FYI, I've started a re-run.

[Darin Adler]

Instead of deferring GC we can switch to a double hashing algorithm (separate get and add), or we can make code that conditionally rehashes if the hash table is modified during the ensure functor.

[Sam Weinig]

It's too bad we can't reuse any of the logic in WeakMapImpl for this, which already should have to deal with this issue.

[Sam Weinig]

(In reply to Sam Weinig from comment #7)
> It's too bad we can't reuse any of the logic in WeakMapImpl for this, which
> already should have to deal with this issue.

There is very similar code to this in

JSDOMGlobalObject::createCrossOriginFunction/JSDOMGlobalObject::createCrossOriginGetterSetter. 

We should make sure that case is also fixed and potentially add a test if possible.

[Sam Weinig]

(In reply to Yusuke Suzuki from comment #3)
> Comment on attachment 423997 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423997&action=review
> 
> > Source/JavaScriptCore/runtime/WeakGCMap.h:73
> > +        // If functor invokes GC, GC can prune WeakGCMap, and manipulate HashMap while we are touching it in ensure function.
> > +        // The functor must not invoke GC.
> > +        DisallowGC disallowGC;
> 
> I do not want to put DeferGC here since we would like to be aware about the
> scope of DeferGC explicitly. Deferring GC is not generally good. We would
> like to invoke GC responsively.
> If we use DeferGC, we should make DeferGC scope small, and we should be
> aware of that.

Would it work to instead of using DeferGC for this, we added a bit to WeakGCMap that makes pruneStaleEntries() a no-op while it is set? And then a function like ensureValue would set that bit while it was running?

[Mark Lam]

(In reply to Sam Weinig from comment #9)
> (In reply to Yusuke Suzuki from comment #3)
> > Comment on attachment 423997 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=423997&action=review
> > 
> > > Source/JavaScriptCore/runtime/WeakGCMap.h:73
> > > +        // If functor invokes GC, GC can prune WeakGCMap, and manipulate HashMap while we are touching it in ensure function.
> > > +        // The functor must not invoke GC.
> > > +        DisallowGC disallowGC;
> > 
> > I do not want to put DeferGC here since we would like to be aware about the
> > scope of DeferGC explicitly. Deferring GC is not generally good. We would
> > like to invoke GC responsively.
> > If we use DeferGC, we should make DeferGC scope small, and we should be
> > aware of that.
> 
> Would it work to instead of using DeferGC for this, we added a bit to
> WeakGCMap that makes pruneStaleEntries() a no-op while it is set? And then a
> function like ensureValue would set that bit while it was running?

if GC can run, and the setter is not reachable by GC, and we don't prune the pointer, wouldn't simply skipping pruning means we'll get a dangling pointer because GC can sweep that setter away later?

[Mark Lam]

(In reply to Mark Lam from comment #10)
> if GC can run, and the setter is not reachable by GC, and we don't prune the
> pointer, wouldn't simply skipping pruning means we'll get a dangling pointer
> because GC can sweep that setter away later?

Nevermind, I misunderstood pruning.  The pointer is already null.

[Yusuke Suzuki]

(In reply to Darin Adler from comment #6)
> Instead of deferring GC we can switch to a double hashing algorithm
> (separate get and add), or we can make code that conditionally rehashes if
> the hash table is modified during the ensure functor.

I think the current DeferGC thing is the best way since,

1. DeferGC does not defer GC forever. When destroying DeferGC scope, we invoke GC if GC request happened during DeferGC scope. So this DeferGC only defers one function allocation.
2. While we can just defer GC for one allocation, we can avoid double hashing, and we can avoid making hash table more complicated.

[Yusuke Suzuki]

(In reply to Sam Weinig from comment #9)
> (In reply to Yusuke Suzuki from comment #3)
> > Comment on attachment 423997 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=423997&action=review
> > 
> > > Source/JavaScriptCore/runtime/WeakGCMap.h:73
> > > +        // If functor invokes GC, GC can prune WeakGCMap, and manipulate HashMap while we are touching it in ensure function.
> > > +        // The functor must not invoke GC.
> > > +        DisallowGC disallowGC;
> > 
> > I do not want to put DeferGC here since we would like to be aware about the
> > scope of DeferGC explicitly. Deferring GC is not generally good. We would
> > like to invoke GC responsively.
> > If we use DeferGC, we should make DeferGC scope small, and we should be
> > aware of that.
> 
> Would it work to instead of using DeferGC for this, we added a bit to
> WeakGCMap that makes pruneStaleEntries() a no-op while it is set? And then a
> function like ensureValue would set that bit while it was running?

I think DeferGC is better since this makes collecting dead Weak handles & shrinking later.
When comparing (1) not collecting dead Weak handles until the next Full GC and (2) defer GC for one function allocation, (2) is better.

[Yusuke Suzuki]

(In reply to Sam Weinig from comment #8)
> (In reply to Sam Weinig from comment #7)
> > It's too bad we can't reuse any of the logic in WeakMapImpl for this, which
> > already should have to deal with this issue.
> 
> There is very similar code to this in
> 
> JSDOMGlobalObject::createCrossOriginFunction/JSDOMGlobalObject::
> createCrossOriginGetterSetter. 
> 
> We should make sure that case is also fixed and potentially add a test if
> possible.

I think the existing test is failing because of DisallowGC, and this ensures that we are not invoking GC which incurs WeakGCMap modification. So crashing tests are guaranteeing this.

[Yusuke Suzuki]

Created attachment 424043 [details]
Patch

[Yusuke Suzuki]

Committed r274911 (235681@main): <https://commits.webkit.org/235681@main>

[Darin Adler]

Why are we deferring *both* at all call sites of ensureValue *and* inside ensureValue? It seems like we are holding up our pants using both belts and suspenders. There is probably a good reason, but I can’t figure it out from reading the comments.

[Yusuke Suzuki]

(In reply to Darin Adler from comment #17)
> Why are we deferring *both* at all call sites of ensureValue *and* inside
> ensureValue? It seems like we are holding up our pants using both belts and
> suspenders. There is probably a good reason, but I can’t figure it out from
> reading the comments.

No, inside ensureValue we are using DisallowGC, not DeferGC.

[Darin Adler]

(In reply to Yusuke Suzuki from comment #18)
> No, inside ensureValue we are using DisallowGC, not DeferGC.

I see. Let me check my understanding: callers must not GC-allocate in the function they pass to ensureValue, unless they discover they must, in which case they can relax the restriction by doing DeferGC.

[Yusuke Suzuki]

(In reply to Darin Adler from comment #19)
> (In reply to Yusuke Suzuki from comment #18)
> > No, inside ensureValue we are using DisallowGC, not DeferGC.
> 
> I see. Let me check my understanding: callers must not GC-allocate in the
> function they pass to ensureValue, unless they discover they must, in which
> case they can relax the restriction by doing DeferGC.

Right. This enforces the caller to be aware that they are deferring GC while running this code.