Summary: | Reduce memory footprint for background tabs | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ben Nham <nham> | ||||||||||
Component: | New Bugs | Assignee: | Ben Nham <nham> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, ews-watchlist, nham, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Ben Nham
2021-04-23 16:06:04 PDT
Created attachment 426961 [details]
Patch
Comment on attachment 426961 [details]
Patch
initial pass for testing
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. Created attachment 427084 [details]
Patch
Comment on attachment 427084 [details]
Patch
Enable only for Mac and Catalyst and don't drop the back/forward cache.
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. 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. Created attachment 427088 [details]
patch for landing
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]. Reverted r276619 for reason: Causes multiple regressions on perf bots. Committed r276779 (237162@main): <https://commits.webkit.org/237162@main> Created attachment 427351 [details]
Patch
*** Bug 225165 has been marked as a duplicate of this bug. *** 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]. |