Bug 194594 – [PSON] Introduce a WebContent Process cache

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

[Chris Dumez]

Introduce a WebContent Process cache to avoid launching new processes too frequently and reduce the power impact of PSON.

[Chris Dumez]

<rdar://problem/46793397>

[Chris Dumez]

Created attachment 361952 [details]
Patch

[Chris Dumez]

Created attachment 361953 [details]
Patch

[Chris Dumez]

Created attachment 361986 [details]
Patch

[Geoffrey Garen]

Comment on attachment 361986 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/WebProcessCache.cpp:121
> +void WebProcessCache::setApplicationIsActive(bool value)

value => isActive

> Source/WebKit/UIProcess/WebProcessCache.h:48
> +    unsigned maximumSize() { return m_maximumSize; }

We usually call this capacity.

> Source/WebKit/UIProcess/WebProcessPool.cpp:1337
>  void WebProcessPool::handleMemoryPressureWarning(Critical)
>  {
> +    RELEASE_LOG(PerformanceLogging, "%p - WebProcessPool::handleMemoryPressureWarning", this);
> +
> +    m_webProcessCache->clear();

Does the process suspension code run the low memory warning in the web process?

Relatedly, maybe the web process should listen to the low memory warning, in case it gets the warning before the UI process does.

> Source/WebKit/UIProcess/WebProcessPool.cpp:2301
> +    // FIXME: If the SuspendedPage is for this page, then we need to destroy the suspended page as we do not support having
> +    // multiple WebPages with the same ID in a given WebProcess currently (https://bugs.webkit.org/show_bug.cgi?id=191166).
> +    if (&(*it)->page() == &page)
> +        m_suspendedPages.remove(it);
>  

Do we need the same check when we retrieve a process from the web process cache?

[Chris Dumez]

Comment on attachment 361986 [details]
Patch

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

>> Source/WebKit/UIProcess/WebProcessPool.cpp:1337
>> +    m_webProcessCache->clear();
> 
> Does the process suspension code run the low memory warning in the web process?
> 
> Relatedly, maybe the web process should listen to the low memory warning, in case it gets the warning before the UI process does.

The processes suspension code does simulate a memory warning in the WebProcress on iOS. There is no process suspension on macOS and this cache will be macOS only I believe.

It is a good point that the WebProcess’s memory pressure handler should likely cause the process the exit if it is in the cache. I will look into this.

>> Source/WebKit/UIProcess/WebProcessPool.cpp:2301
>>  
> 
> Do we need the same check when we retrieve a process from the web process cache?

No, processes in the cache cannot have any pages or suspended pages, they only enter the cache when they are about to exit because they are no longer used for anything.

[Geoffrey Garen]

Yeah, I think the cache should be mac only.

[Chris Dumez]

Comment on attachment 361986 [details]
Patch

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

>> Source/WebKit/UIProcess/WebProcessCache.cpp:121
>> +void WebProcessCache::setApplicationIsActive(bool value)
> 
> value => isActive

Ok

>> Source/WebKit/UIProcess/WebProcessCache.h:48
>> +    unsigned maximumSize() { return m_maximumSize; }
> 
> We usually call this capacity.

Ok but for the record I prefer maximumSize because I feel it is clearer. Vector has the concept of capacity and it has nothing to do with maximum size.

>>> Source/WebKit/UIProcess/WebProcessPool.cpp:1337
>>> +    m_webProcessCache->clear();
>> 
>> Does the process suspension code run the low memory warning in the web process?
>> 
>> Relatedly, maybe the web process should listen to the low memory warning, in case it gets the warning before the UI process does.
> 
> The processes suspension code does simulate a memory warning in the WebProcress on iOS. There is no process suspension on macOS and this cache will be macOS only I believe.
> 
> It is a good point that the WebProcess’s memory pressure handler should likely cause the process the exit if it is in the cache. I will look into this.

Note that we could simulate a memory pressure warning when entering the cache to try and slim the process as much as possible. However, doing so could impact performance as some valuable caches would be gone when trying to reuse the process. This could impact power and PLT. For this reason, I have not implemented it in this patch.

[Chris Dumez]

Created attachment 362022 [details]
Patch

[Brady Eidson]

Comment on attachment 362022 [details]
Patch

👍

[Chris Dumez]

(In reply to Brady Eidson from comment #10)
> Comment on attachment 362022 [details]
> Patch
> 
> 👍

Thanks for looking as well.

[Chris Dumez]

I will try and figure out why the GTK bot is unhappy now :/

[Chris Dumez]

Created attachment 362026 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 362026 [details]
Patch

Clearing flags on attachment: 362026

Committed r241556: <https://trac.webkit.org/changeset/241556>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

Probably the right move is to land without the pressure warning when entering the cache, and then separately a/b test a change to add a pressure warning. 

It’s a good point that a cache’s capacity limits its size, whereas a vector’s capacity does not. One thing that was unclear to me about “maximum size” was that I first read it as “the maximum size I have ever been” rather than “the maximum size I am allowed to reach”. STL vectors have a max size — but that means the max size that any vector could be, and it’s not adjustable for a given vector. I do think people often refer to caches as having a capacity. e.g. http://kb.mozillazine.org/Browser.cache.memory.capacity. I’ve gotten used to size as “how many elements I have” and capacity as “how many elements I could have before triggering some action (grow, evict, ring buffer, crash, whatever)”. Maybe we should circulate with other folks and see what the communal preference is.