Bug 219689

Summary: [WPE][GTK] Should enable WebProcessCache
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aperez, berto, bugs-noreply, cdumez, cgarcia, ews-watchlist, gustavo, mcatanzaro
Priority: P2    
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
aperez: review+
Patch for landing none

Description Michael Catanzaro 2020-12-09 08:34:27 PST 
It looks like WPE and GTK do not enable the WebProcessCache feature added in r241556:

"""
Introduce a WebContent Process cache to reduce the number of process launches when
process swap on navigation is enabled, and to reduce the power cost of the feature.

If a WebProcess loaded pages from a single registrable domain then it is eligible
for the cache. When process-swapping on navigation to a new registrable domain, we
now attempt to retrieve a process from the cache for the domain in question instead
of always launching a new one.

The WebProcess cache currently has the following attributes:

It may contains 4 processes per GB of RAM the machine has, up to 30 processes.
WebProcesses automatically get evicted from the cache after 30 minutes.
If the application is no longer the active app, then the cache will get cleared after 5 minutes.
WebProcesses that are in the cache are reported as "(Cached)" in Activity Monitor.
The WebProcess cache is currently disabled by default and can by enabled by the
client via SPI.
"""

We probably want to have that enabled, or at least make it possible to enable it?
Comment 1 Chris Dumez 2020-12-09 08:36:16 PST 
If you have Process-Swap-On-Navigation (PSON) enabled, then I think you'd definitely want the process cache too. Note that Apple only enables the process-cache on desktop (not mobile) due to memory constraints.
Comment 2 Michael Catanzaro 2020-12-09 08:39:03 PST 
Yeah of course we have PSON. That has been enabled for ages now.

I think nobody realized that the process cache had to be enabled separately.
Comment 3 Michael Catanzaro 2020-12-09 08:42:22 PST 
(In reply to Chris Dumez from comment #1)
> If you have Process-Swap-On-Navigation (PSON) enabled, then I think you'd
> definitely want the process cache too. Note that Apple only enables the
> process-cache on desktop (not mobile) due to memory constraints.

Currently we have a chassis type check that could be used for this, but it doesn't work reliably due to bug #218122, and is currently only used to set a mobile user agent header.

It would probably make more sense to enable or disable the cache based on total available RAM, rather than checking form factor, since there's not really any difference between a mobile phone with 3 GB RAM (Librem 5) and a desktop with 3 GB RAM. The performance hit if you run out of RAM is going to be bad either way.
Comment 4 Chris Dumez 2020-12-09 08:44:34 PST 
(In reply to Michael Catanzaro from comment #3)
> (In reply to Chris Dumez from comment #1)
> > If you have Process-Swap-On-Navigation (PSON) enabled, then I think you'd
> > definitely want the process cache too. Note that Apple only enables the
> > process-cache on desktop (not mobile) due to memory constraints.
> 
> Currently we have a chassis type check that could be used for this, but it
> doesn't work reliably due to bug #218122, and is currently only used to set
> a mobile user agent header.
> 
> It would probably make more sense to enable or disable the cache based on
> total available RAM, rather than checking form factor, since there's not
> really any difference between a mobile phone with 3 GB RAM (Librem 5) and a
> desktop with 3 GB RAM. The performance hit if you run out of RAM is going to
> be bad either way.

Yes, Apple port listen to memory pressure warnings and kill those cached processes as necessary.
Comment 5 Carlos Garcia Campos 2020-12-10 05:22:33 PST 
I thought it was enabled. I'll submit a patch.
Comment 6 Carlos Garcia Campos 2020-12-10 05:23:25 PST 
Created attachment 415860 [details]
Patch
Comment 7 EWS Watchlist 2020-12-10 05:24:15 PST 
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 8 Adrian Perez 2020-12-10 06:28:46 PST 
Comment on attachment 415860 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:394
> +    configuration.setUsesWebProcessCache(true);

I think it's fine to land this as-is, but depending on things go we
might need to either add some smarts to choose a sensible default
for this and/or add API to WebKitWebContext to toggle it.
Comment 9 Michael Catanzaro 2020-12-10 06:56:39 PST 
The GTK API EWS is red. Score +1 for whoever set up that EWS bot. That is nice to have....
Comment 10 Carlos Garcia Campos 2020-12-14 03:01:00 PST 
Created attachment 416146 [details]
Patch for landing
Comment 11 Carlos Garcia Campos 2020-12-14 04:28:25 PST 
Committed r270767: <https://trac.webkit.org/changeset/270767>