Bug 167491

Summary: [Mac] In-process memory pressure monitor for WebContent processes.
Product: WebKit Reporter: Andreas Kling <kling>
Component: WebCore Misc.Assignee: Andreas Kling <kling>
Severity: Normal CC: barraclough, cdumez, cgarcia, clopez, commit-queue, ggaren, joepeck, kling, ryanhaddad
Priority: P2 Keywords: InRadar, Performance
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=175133
Bug Depends on: 167751    
Bug Blocks:    
Description Flags
Work in progress
Work in progress
Work in progress
Getting closer
Basically a patch
Basically a patch that also builds
koivisto: review+
Patch for landing
Patch for landing II none

Description Andreas Kling 2017-01-27 00:34:42 PST
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.
Comment 1 Andreas Kling 2017-01-27 00:52:13 PST
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.
Comment 2 WebKit Commit Bot 2017-01-27 00:53:24 PST
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.
Comment 3 Andreas Kling 2017-01-27 01:08:45 PST
Created attachment 299920 [details]
Work in progress
Comment 4 Andreas Kling 2017-01-27 01:47:59 PST
Created attachment 299925 [details]
Work in progress

All the mac EWS'es not having RELEASE_LOG is pretty sad :|
Comment 5 Carlos Alberto Lopez Perez 2017-01-27 05:37:16 PST
(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 .....
Comment 6 Andreas Kling 2017-01-27 06:15:39 PST
Yep, I don't intend to enable this on any mobile operating systems :)
Comment 7 Andreas Kling 2017-01-30 04:37:53 PST
Created attachment 300098 [details]
Getting closer
Comment 8 Andreas Kling 2017-01-30 16:59:58 PST
Comment 9 Andreas Kling 2017-01-30 17:52:16 PST
Created attachment 300172 [details]
Basically a patch
Comment 10 Andreas Kling 2017-01-30 18:29:07 PST
Created attachment 300178 [details]
Basically a patch that also builds
Comment 11 Andreas Kling 2017-01-30 21:26:31 PST
Created attachment 300185 [details]

Without unrelated Tools changes this time.
Comment 12 Antti Koivisto 2017-01-31 03:58:49 PST
Comment on attachment 300185 [details]

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


> 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

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 13 Antti Koivisto 2017-01-31 04:18:19 PST
Comment on attachment 300185 [details]

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.
Comment 14 Andreas Kling 2017-02-02 06:51:02 PST
Created attachment 300407 [details]

With suggestions from Antti and Geoff slightly incorporated.
Comment 15 Antti Koivisto 2017-02-02 07:45:41 PST
Comment on attachment 300407 [details]

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)


> 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"?
Comment 16 Andreas Kling 2017-02-02 09:44:42 PST
Created attachment 300417 [details]
Patch for landing
Comment 17 WebKit Commit Bot 2017-02-02 10:37:24 PST
Comment on attachment 300417 [details]
Patch for landing

Clearing flags on attachment: 300417

Committed r211571: <http://trac.webkit.org/changeset/211571>
Comment 18 WebKit Commit Bot 2017-02-02 10:37:31 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Chris Dumez 2017-02-02 12:24:26 PST
Follow-up Windows / GTK build fix at <http://trac.webkit.org/changeset/211582>.
Comment 20 Ryan Haddad 2017-02-02 13:01:28 PST
(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:


UNEXPECTEDLY EXITED WebKit1.MemoryPressureHandler
/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
Comment 21 WebKit Commit Bot 2017-02-02 13:18:15 PST
Re-opened since this is blocked by bug 167751
Comment 22 Joseph Pecoraro 2017-02-02 14:33:47 PST
Assertion means someone needs to call RunLoop::initializeMainRunLoop().
Comment 23 Andreas Kling 2017-02-02 21:19:15 PST
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 24 WebKit Commit Bot 2017-02-02 23:26:51 PST
Comment on attachment 300494 [details]
Patch for landing II

Clearing flags on attachment: 300494

Committed r211622: <http://trac.webkit.org/changeset/211622>
Comment 25 WebKit Commit Bot 2017-02-02 23:26:58 PST
All reviewed patches have been landed.  Closing bug.