Bug 225007 – Reduce memory footprint for background tabs

• Component: New Bugs
• Status: RESOLVED FIXED

[Ben Nham]

Reduce memory footprint for background tabs

[Ben Nham]

Created attachment 426961 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/77090852>

[Ben Nham]

Comment on attachment 426961 [details]
Patch

initial pass for testing

[Chris Dumez]

Comment on attachment 426961 [details]
Patch

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

> Source/WTF/ChangeLog:9
> +        similar just before WebContent suspends.

The major difference is that on iOS the process suspends and does not keep allocating memory after we simulate memory pressure.

> Source/WTF/wtf/PlatformEnableCocoa.h:393
> +#if !defined(ENABLE_NON_VISIBLE_WEBPROCESS_MEMORY_CLEANUP_TIMER)

Did you mean to enable this only on macOS?

> Source/WebKit/WebProcess/WebProcess.cpp:1590
> +#if ENABLE(NON_VISIBLE_WEBPROCESS_MEMORY_CLEANUP_TIMER)

This should probably cover the whole timer logic. Why start a timer to do nothing on iOS?

> Source/WebKit/WebProcess/WebProcess.cpp:1591
> +    WebCore::releaseMemory(WTF::Critical::Yes, WTF::Synchronous::No, WebCore::MaintainBackForwardCache::No, WebCore::MaintainMemoryCache::No);

One concern. Consider the case where we navigate cross-origin (Process-swap-on-navigation). To support back/forward cache, we keep the previous process around with its back/forward cache entry. When we swapped process, I am assuming that pageWillLeaveWindow() gets called in the previous process. If so, it would start your 1min timer and it would clear my back/forward cache entry. This means that the back/forward cache only works for 1min after a process-swap. If so, this seems bad.

[Ben Nham]

Created attachment 427084 [details]
Patch

[Ben Nham]

Comment on attachment 427084 [details]
Patch

Enable only for Mac and Catalyst and don't drop the back/forward cache.

[Chris Dumez]

Comment on attachment 427084 [details]
Patch

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

r- because this clears the back/forward cache and thus likely breaks the back/forward cache after 1 min, after navigating cross-origin (process-swap).

> Source/WebKit/WebProcess/WebProcess.cpp:1600
> +    WebCore::releaseMemory(WTF::Critical::Yes, WTF::Synchronous::No, WebCore::MaintainBackForwardCache::Yes, WebCore::MaintainMemoryCache::No);

You are still clearing the back/forward cache, which is the one I complained about.

WTF:: is unnecessary, so is WebCore::.

> Source/WebKit/WebProcess/WebProcess.cpp:1602
> +        page->releaseMemory(WTF::Critical::Yes);

WTF:: is unnecessary.

[Chris Dumez]

Comment on attachment 427084 [details]
Patch

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

r=me with nits.

>> Source/WebKit/WebProcess/WebProcess.cpp:1600
>> +    WebCore::releaseMemory(WTF::Critical::Yes, WTF::Synchronous::No, WebCore::MaintainBackForwardCache::Yes, WebCore::MaintainMemoryCache::No);
> 
> You are still clearing the back/forward cache, which is the one I complained about.
> 
> WTF:: is unnecessary, so is WebCore::.

Oh, I misread, this is keeping the back/forward cache. My bad.

[Ben Nham]

Created attachment 427088 [details]
patch for landing

[EWS]

Committed r276619 (237048@main): <https://commits.webkit.org/237048@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427088 [details].

[Ben Nham]

Reverted r276619 for reason:

Causes multiple regressions on perf bots.

Committed r276779 (237162@main): <https://commits.webkit.org/237162@main>

[Ben Nham]

Created attachment 427351 [details]
Patch

[Ben Nham]

*** Bug 225165 has been marked as a duplicate of this bug. ***

[EWS]

Committed r276788 (237166@main): <https://commits.webkit.org/237166@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427351 [details].