Bug 224110

Summary: Delete JS code and trigger garbage collection in worker threads on memory pressure
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Service WorkersAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, fpizlo, ggaren, mark.lam, sbarati, simon.fraser, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 224088    
Attachments:
Description Flags
Patch
none
Patch ews-feeder: commit-queue-

Description Chris Dumez 2021-04-02 08:29:26 PDT
Delete JS code and trigger garbage collection in worker threads on memory pressure.
Comment 1 Chris Dumez 2021-04-02 08:32:10 PDT
Created attachment 425025 [details]
Patch
Comment 2 Mark Lam 2021-04-02 09:43:24 PDT
Comment on attachment 425025 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        For the garbage collection logic, I tried to match what the GCController is going

/going/doing/

> Source/WebCore/workers/WorkerGlobalScope.cpp:63
> +static HashSet<ScriptExecutionContextIdentifier>& allWorkerGlobalScopeIdentifiers()

One trick we use to make this lock requirement more detectible is to make the function take a LockHolder& though we don't use the lock in the function i.e. you much have acquired the lock before calling the function.  Otherwise, you get a compile time error.  It doesn't enforce that you're holding the right lock, but at least you'll know if you forgot to grab a lock.

> Source/WebCore/workers/WorkerGlobalScope.cpp:90
> +        allWorkerGlobalScopeIdentifiers().add(contextIdentifier());

It would be better to enforce that we hold the lock when calling add and remove, instead of when we get the HashSet.  So, maybe wrap the whole thing in a class with its own add and remove that takes a LockHolder&?
Comment 3 Chris Dumez 2021-04-02 09:49:46 PDT
Created attachment 425030 [details]
Patch
Comment 4 Geoffrey Garen 2021-04-02 09:50:20 PDT
r=me

> Source/WebCore/workers/WorkerGlobalScope.cpp:63
> +static HashSet<ScriptExecutionContextIdentifier>& allWorkerGlobalScopeIdentifiers()

I like the design pattern where we pass a Locker& to a function that requires a lock. Helps to remind you at compile time. (It's not perfect, since the type doesn't guarantee you took the *right* lock. Still, I've found it to be nice enough in practice.)

> Source/WebCore/workers/WorkerGlobalScope.cpp:532
> +    vm().deleteAllCode(JSC::DeleteAllCodeIfNotCollecting);

It seems like our pre-existing idiom has been to use DeleteAllCodeIfNotCollecting, ever since we had a concurrent GC (http://trac.webkit.org/changeset/209570/webkit). So, I guess it's good to be consistent here.

But I think we want to follow up and consider always using PreventCollectionAndDeleteAllCode instead. Otherwise, if concurrent GC is in progress, we may miss opportunities to delete code, which is suboptimal, and might also cause flaky results in our memory benchmarks.

> Source/WebCore/workers/WorkerGlobalScope.cpp:552
> +#if PLATFORM(IOS_FAMILY)
> +    if (!vm().heap.isCurrentThreadBusy()) {
> +        vm().heap.collectNowFullIfNotDoneRecently(JSC::Async);
> +        return;
> +    }
> +#endif
> +#if USE(CF) || USE(GLIB)
> +    vm().heap.reportAbandonedObjectGraph();
> +#else
> +    vm().heap.collectNow(JSC::Async, JSC::CollectionScope::Full);
> +#endif
> +}

I guess this is how the existing main thread memory warning works too? I think this is also kinda sub-optimal, and the IOS behavior would be best for every platform.
Comment 5 Chris Dumez 2021-04-02 09:53:07 PDT
(In reply to Geoffrey Garen from comment #4)
> r=me
> 
> > Source/WebCore/workers/WorkerGlobalScope.cpp:63
> > +static HashSet<ScriptExecutionContextIdentifier>& allWorkerGlobalScopeIdentifiers()
> 
> I like the design pattern where we pass a Locker& to a function that
> requires a lock. Helps to remind you at compile time. (It's not perfect,
> since the type doesn't guarantee you took the *right* lock. Still, I've
> found it to be nice enough in practice.)

Yes, I made that change.

> 
> > Source/WebCore/workers/WorkerGlobalScope.cpp:532
> > +    vm().deleteAllCode(JSC::DeleteAllCodeIfNotCollecting);
> 
> It seems like our pre-existing idiom has been to use
> DeleteAllCodeIfNotCollecting, ever since we had a concurrent GC
> (http://trac.webkit.org/changeset/209570/webkit). So, I guess it's good to
> be consistent here.
> 
> But I think we want to follow up and consider always using
> PreventCollectionAndDeleteAllCode instead. Otherwise, if concurrent GC is in
> progress, we may miss opportunities to delete code, which is suboptimal, and
> might also cause flaky results in our memory benchmarks.
> 
> > Source/WebCore/workers/WorkerGlobalScope.cpp:552
> > +#if PLATFORM(IOS_FAMILY)
> > +    if (!vm().heap.isCurrentThreadBusy()) {
> > +        vm().heap.collectNowFullIfNotDoneRecently(JSC::Async);
> > +        return;
> > +    }
> > +#endif
> > +#if USE(CF) || USE(GLIB)
> > +    vm().heap.reportAbandonedObjectGraph();
> > +#else
> > +    vm().heap.collectNow(JSC::Async, JSC::CollectionScope::Full);
> > +#endif
> > +}
> 
> I guess this is how the existing main thread memory warning works too? I
> think this is also kinda sub-optimal, and the IOS behavior would be best for
> every platform.

Yes, I matched for we do in MemoryRelease / GCController. We can consider using the iOS behavior on all platforms but we should align the main thread behavior too then.
Comment 6 Radar WebKit Bug Importer 2021-04-02 10:28:51 PDT
<rdar://problem/76153592>
Comment 7 Chris Dumez 2021-04-02 11:40:13 PDT
Comment on attachment 425030 [details]
Patch

Clearing flags on attachment: 425030

Committed r275428 (236091@main): <https://commits.webkit.org/236091@main>
Comment 8 Chris Dumez 2021-04-02 11:40:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 EWS 2021-04-02 12:13:52 PDT
Found 1 new test failure: fullscreen/full-screen-placeholder.html