Bug 145672 – [WK2][iOS] Limit the number of vnodes used by the WebContent processes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Limit the number of VNodes used by the WebContent processes to reduce the chance of getting killed due to running out of VNodes.

We sometimes see the WebContent process use up to 50% of the system's vnode-limit on some tests, which seems excessive. Most VNodes are due to CachedResources which are mmap'd from the WebKit disk cache and kept alive due to caches such as the Memory Cache / PageCache.

Radar: <rdar://problem/21126637>

[Chris Dumez]

Created attachment 254341 [details]
Patch

[Antti Koivisto]

Comment on attachment 254341 [details]
Patch

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

> Source/WebCore/platform/VNodeTracker.cpp:68
> +    Critical critical = vnodeCount > m_hardVNodeLimit ? Critical::Yes : Critical::No;

Bit suprising that we have type named Critical in WebCore global namespace.

Could just say auto.

> Source/WebCore/platform/VNodeTracker.cpp:76
> +    static const std::chrono::duration<double> minimumWarningInterval = std::chrono::seconds { 30 };

Why std::chrono::duration<double>? auto should work fine.

> Source/WebCore/platform/VNodeTracker.h:37
> +typedef std::function<void(Critical)> VNodePressureHandler;

Maybe move this to VNodeTracker namespace as well. (as VNodeTracker::PressureHandler)

> Source/WebCore/platform/VNodeTracker.h:43
> +    enum VNodeTokenType { };
> +    typedef RefCounter::Token<VNodeTokenType> VNodeToken;

These could be just TokenType and Token. "VNode" is redundant in VNodeTracker scope.

> Source/WebCore/platform/VNodeTracker.h:48
> +    void setVNodePressureHandler(VNodePressureHandler);
> +    VNodeToken vnodeToken();

Redudand vnodes in function names.

> Source/WebCore/platform/cf/SharedBufferCF.cpp:40
>  SharedBuffer::SharedBuffer(CFDataRef cfData)
>      : m_buffer(adoptRef(new DataBuffer))
>      , m_cfData(cfData)
> +    , m_vnodeToken(VNodeTracker::singleton().vnodeToken())
>  {
>  }

Do all clients for this actually use file backed buffers?

> Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:197
> +#if PLATFORM(IOS)
> +    // Track the number of vnodes we are using on iOS and make sure we only use a
> +    // reasonable amount because limits are fairly low on iOS devices and we can
> +    // get killed when reaching the limit.
> +    VNodeTracker::singleton().setVNodePressureHandler([] (Critical critical) {
> +        MemoryPressureHandler::singleton().releaseMemory(critical);
> +    });
> +#endif

Could just wipe out resources actually using vnodes. But I suppose this is fine.

[Chris Dumez]

Comment on attachment 254341 [details]
Patch

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

>> Source/WebCore/platform/cf/SharedBufferCF.cpp:40
>>  }
> 
> Do all clients for this actually use file backed buffers?

Not all of them but most of them (especially the ones that are long-lived). We likely overestimate the number of vnodes a bit but I think this is a good enough estimation. We could add an "isFileBacked" argument so that the client could provide the information but I am not convinced it is worth the extra complexity.

[Chris Dumez]

Created attachment 254361 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 254361 [details]
Patch

Clearing flags on attachment: 254361

Committed r185273: <http://trac.webkit.org/changeset/185273>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[zalan]

This patch broke https://build.webkit.org/builders/Apple%20Mavericks%20Debug%20WK2%20%28Tests%29/builds/12260/steps/run-api-tests/logs/stdio

Fixed here: https://trac.webkit.org/changeset/185285

[Chris Dumez]

(In reply to comment #7)
> This patch broke
> https://build.webkit.org/builders/
> Apple%20Mavericks%20Debug%20WK2%20%28Tests%29/builds/12260/steps/run-api-
> tests/logs/stdio
> 
> Fixed here: https://trac.webkit.org/changeset/185285

Thanks Zalan!