Delete JS code and trigger garbage collection in worker threads on memory pressure

• Component: Service Workers
• Status: RESOLVED FIXED

[Chris Dumez]

Delete JS code and trigger garbage collection in worker threads on memory pressure.

[Chris Dumez]

Created attachment 425025 [details]
Patch

[Mark Lam]

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&?

[Chris Dumez]

Created attachment 425030 [details]
Patch

[Geoffrey Garen]

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.

[Chris Dumez]

(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.

[Radar WebKit Bug Importer]

<rdar://problem/76153592>

[Chris Dumez]

Comment on attachment 425030 [details]
Patch

Clearing flags on attachment: 425030

Committed r275428 (236091@main): <https://commits.webkit.org/236091@main>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[EWS]

Found 1 new test failure: fullscreen/full-screen-placeholder.html