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>
Created attachment 254341 [details] Patch
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.
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.
Created attachment 254361 [details] Patch
Comment on attachment 254361 [details] Patch Clearing flags on attachment: 254361 Committed r185273: <http://trac.webkit.org/changeset/185273>
All reviewed patches have been landed. Closing bug.
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
(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!