Bug 167491 – [Mac] In-process memory pressure monitor for WebContent processes.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Andreas Kling]

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.

[Andreas Kling]

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.

[WebKit Commit Bot]

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.

[Andreas Kling]

Created attachment 299920 [details]
Work in progress

[Andreas Kling]

Created attachment 299925 [details]
Work in progress

All the mac EWS'es not having RELEASE_LOG is pretty sad :|

[Carlos Alberto Lopez Perez]

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

[Andreas Kling]

Yep, I don't intend to enable this on any mobile operating systems :)

[Andreas Kling]

Created attachment 300098 [details]
Getting closer

[Andreas Kling]

<rdar://problem/30116072>

[Andreas Kling]

Created attachment 300172 [details]
Basically a patch

[Andreas Kling]

Created attachment 300178 [details]
Basically a patch that also builds

[Andreas Kling]

Created attachment 300185 [details]
Patch

Without unrelated Tools changes this time.

[Antti Koivisto]

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?

[Antti Koivisto]

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.

[Andreas Kling]

Created attachment 300407 [details]
Patch

With suggestions from Antti and Geoff slightly incorporated.

[Antti Koivisto]

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

[Andreas Kling]

Created attachment 300417 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 300417 [details]
Patch for landing

Clearing flags on attachment: 300417

Committed r211571: <http://trac.webkit.org/changeset/211571>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Follow-up Windows / GTK build fix at <http://trac.webkit.org/changeset/211582>.

[Ryan Haddad]

(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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 167751

[Joseph Pecoraro]

Assertion means someone needs to call RunLoop::initializeMainRunLoop().

[Andreas Kling]

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.

[WebKit Commit Bot]

Comment on attachment 300494 [details]
Patch for landing II

Clearing flags on attachment: 300494

Committed r211622: <http://trac.webkit.org/changeset/211622>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.