I'm working on a mechanism that periodically monitors the memory pressure of WebContent processes from within the process and responds accordingly. This will allow us to be more strict about what constitutes pressure, while also allows multiple tiers of pressure instead of one big spooky lever.
Created attachment 299918 [details] Work in progress This is where I'm currently at. Every 30 seconds, WebContent processes check their own memory footprint (as reported by vm_info.phys_footprint) and choose one of four pressure levels: * Unrestricted (below 1GB) * Conservative (1GB+) * Strict (2GB+) * Panic (4GB+) Unrestricted and Conservative are currently the same. When reaching Strict, we invoke the non-critical memory pressure handler, and start returning true for MemoryPressureHandler::isUnderMemoryPressure(). When reaching Panic, we synchronously invoke the critical memory pressure handler, and if that fails to bring us below the Panic threshold, we crash the process.
Attachment 299918 [details] did not pass style-queue: ERROR: Source/WebCore/platform/MemoryPressureHandler.h:61: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/platform/MemoryPressureHandler.h:62: One space before end of line comments [whitespace/comments] [5] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 299920 [details] Work in progress
Created attachment 299925 [details] Work in progress All the mac EWS'es not having RELEASE_LOG is pretty sad :|
(In reply to comment #1) > Created attachment 299918 [details] > Work in progress > > This is where I'm currently at. Every 30 seconds, WebContent processes check > their own memory footprint (as reported by vm_info.phys_footprint) and > choose one of four pressure levels: > Isn't this a concern on mobile devices regarding battery usage? If the user has a browser with N tabs, and each tab is firing a timer each 30 seconds .....
Yep, I don't intend to enable this on any mobile operating systems :)
Created attachment 300098 [details] Getting closer
<rdar://problem/30116072>
Created attachment 300172 [details] Basically a patch
Created attachment 300178 [details] Basically a patch that also builds
Created attachment 300185 [details] Patch Without unrelated Tools changes this time.
Comment on attachment 300185 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300185&action=review r=me > Source/WebCore/platform/MemoryPressureHandler.cpp:55 > + if (use) > + m_measurementTimer.startRepeating(30); > + else > + m_measurementTimer.stop(); Never ending timers are not super nice. But I suppose this is the best we can do for now. > Source/WebCore/platform/MemoryPressureHandler.h:62 > + Unrestricted, // Allocate as much as you want > + Conservative, // Maybe you don't cache every single thing > + Strict, // Time to start pinching pennies for real > + Panic, // OH GOD WE'RE SINKING, THROW EVERYTHING OVERBOARD Missing periods at the end of the comments. Also, they could be neatly aligned. > Source/WebCore/platform/MemoryPressureHandler.h:152 > + MemoryUsagePolicy memoryUsagePolicy() const { return m_memoryUsagePolicy; } It is confusing that this is only gives sensible values on Mac. Maybe it should be private until it is widely supported? Maybe naming should reflect the availability or it should be #if'd out when not available?
Comment on attachment 300185 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300185&action=review > Source/WebCore/platform/MemoryPressureHandler.cpp:99 > +static MemoryUsagePolicy policyForFootprint(size_t footprint) > +{ > + if (footprint >= thresholdForPolicy(MemoryUsagePolicy::Panic)) > + return MemoryUsagePolicy::Panic; > + if (footprint >= thresholdForPolicy(MemoryUsagePolicy::Strict)) > + return MemoryUsagePolicy::Strict; > + if (footprint >= thresholdForPolicy(MemoryUsagePolicy::Conservative)) > + return MemoryUsagePolicy::Conservative; > + return MemoryUsagePolicy::Unrestricted; > +} Should we also consider overall system memory pressure? > Source/WebCore/platform/MemoryPressureHandler.cpp:104 > +NEVER_INLINE void MemoryPressureHandler::didExceedMemoryLimitAndFailedToRecover() > +{ > + CRASH(); > +} Can we limit this to background tabs only? Shooting down foreground is more risky and annoying to user.
Created attachment 300407 [details] Patch With suggestions from Antti and Geoff slightly incorporated.
Comment on attachment 300407 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300407&action=review > Source/WebCore/platform/MemoryPressureHandler.cpp:70 > +static size_t thresholdForPolicy(MemoryUsagePolicy policy) constexpr? > Source/WebCore/platform/MemoryPressureHandler.h:81 > + void setDidExceedMemoryLimitAndFailedToRecoverCallback(WTF::Function<void()> function) { m_didExceedMemoryLimitAndFailedToRecoverCallback = WTFMove(function); } > + void setProcessIsEligibleForMemoryKillCallback(WTF::Function<bool()> function) { m_processIsEligibleForMemoryKillCallback = WTFMove(function); } These are not consistent with each other. How about calling the first one "MemoryKillCallback"?
Created attachment 300417 [details] Patch for landing
Comment on attachment 300417 [details] Patch for landing Clearing flags on attachment: 300417 Committed r211571: <http://trac.webkit.org/changeset/211571>
All reviewed patches have been landed. Closing bug.
Follow-up Windows / GTK build fix at <http://trac.webkit.org/changeset/211582>.
(In reply to comment #17) > Comment on attachment 300417 [details] > Patch for landing > > Clearing flags on attachment: 300417 > > Committed r211571: <http://trac.webkit.org/changeset/211571> This change appears to have caused API test WebKit1.MemoryPressureHandler to fail with an assertion: https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20WK2%20%28Tests%29/builds/10779/steps/run-api-tests/logs/stdio UNEXPECTEDLY EXITED WebKit1.MemoryPressureHandler ASSERTION FAILED: s_mainRunLoop /Volumes/Data/slave/elcapitan-debug/build/Source/WTF/wtf/RunLoop.cpp(66) : static WTF::RunLoop &WTF::RunLoop::main() 1 0x109fe2ca0 WTFCrash 2 0x10a033afe WTF::RunLoop::main() 3 0x1124ee43e WebCore::MemoryPressureHandler::MemoryPressureHandler() 4 0x1124ee6f5 WebCore::MemoryPressureHandler::MemoryPressureHandler() 5 0x1124ef110 WTF::NeverDestroyed<WebCore::MemoryPressureHandler>::NeverDestroyed<>() 6 0x1124eeb15 WTF::NeverDestroyed<WebCore::MemoryPressureHandler>::NeverDestroyed<>() 7 0x1124ee3dd WebCore::MemoryPressureHandler::singleton() 8 0x10fe539e2 WebInstallMemoryPressureHandler::$_4::operator()() const 9 0x10fe539b5 void std::__1::__call_once_proxy<std::__1::tuple<WebInstallMemoryPressureHandler::$_4> >(void*) 10 0x7fff84eb26e9 std::__1::__call_once(unsigned long volatile&, void*, void (*)(void*)) 11 0x10fe28e85 WebInstallMemoryPressureHandler 12 0x108507b41 TestWebKitAPI::WebKit1_MemoryPressureHandler_Test::TestBody() 13 0x1085f73da testing::Test::Run() 14 0x1085f7cbd testing::internal::TestInfoImpl::Run() 15 0x1085f885d testing::TestCase::Run() 16 0x1085fe7e2 testing::internal::UnitTestImpl::RunAllTests() 17 0x1085fe459 testing::UnitTest::Run() 18 0x108505e2e TestWebKitAPI::TestsController::run(int, char**) 19 0x1085edde4 main 20 0x7fff9459c5ad start
Re-opened since this is blocked by bug 167751
Assertion means someone needs to call RunLoop::initializeMainRunLoop().
Created attachment 300494 [details] Patch for landing II Put the RunLoop::Timer in a unique_ptr so we don't try to instantiate it on WK1. Merged in Windows build fixes from Chris.
Comment on attachment 300494 [details] Patch for landing II Clearing flags on attachment: 300494 Committed r211622: <http://trac.webkit.org/changeset/211622>