[JSC] Functor for WeakGCMap::ensureValue must not invoke GC
Created attachment 423997 [details] Patch
<rdar://problem/75619217>
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.
Created attachment 423998 [details] Patch
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.
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.
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.
(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.
(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?
(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?
(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.
(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.
(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.
(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.
Created attachment 424043 [details] Patch
Committed r274911 (235681@main): <https://commits.webkit.org/235681@main>
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.
(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.
(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.
(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.