Bug 191966

Summary: [PSON] process pre-warming should not be on for everyone
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, commit-queue, ggaren, koivisto, rniwa
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Chris Dumez 2018-11-26 09:07:24 PST
Process pre-warming should not be on for everyone as some clients would not benefit from it and it would be an unnecessary memory cost.
Comment 1 Chris Dumez 2018-11-26 09:07:40 PST
<rdar://problem/46138499>
Comment 2 Alex Christensen 2018-11-26 09:55:24 PST
How would we be able to tell which clients would benefit from it?
Comment 3 Chris Dumez 2018-11-26 13:03:14 PST
(In reply to Alex Christensen from comment #2)
> How would we be able to tell which clients would benefit from it?

Detailed on the radar but basically, we turn on process prewarming after the first process swap.
Comment 4 Chris Dumez 2018-11-26 13:53:43 PST
Created attachment 355671 [details]
Patch
Comment 5 Ryosuke Niwa 2018-11-26 13:58:36 PST
Comment on attachment 355671 [details]
Patch

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

> Source/WebKit/UIProcess/WebProcessPool.cpp:1288
> -    if (!configuration().isAutomaticProcessWarmingEnabled())
> +    if (!configuration().isAutomaticProcessWarmingEnabled() || !configuration().processSwapsOnNavigation())

But aren't there clients which would benefit from pre-warming without enabling PSON?
e.g. for opening multiple WKWebViews?
Comment 6 Chris Dumez 2018-11-26 14:03:14 PST
(In reply to Ryosuke Niwa from comment #5)
> Comment on attachment 355671 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355671&action=review
> 
> > Source/WebKit/UIProcess/WebProcessPool.cpp:1288
> > -    if (!configuration().isAutomaticProcessWarmingEnabled())
> > +    if (!configuration().isAutomaticProcessWarmingEnabled() || !configuration().processSwapsOnNavigation())
> 
> But aren't there clients which would benefit from pre-warming without
> enabling PSON?
> e.g. for opening multiple WKWebViews?

Automatic process prewarming is not currently API, which is why I am adding the heuristic to turn it on automatically when we determine it would be beneficial.

Process-Swap on Navigation is not not currently API and *is* enabled by default so clients do get PSON no matter what. They will also get process prewarming if our heuristic determines it would be beneficial.

As the change log explains, the reason I made this particular change is because Safari will turn automatic process prewarming on explicitly and rely on the experimental feature flag for PSON support. If PSON is disabled in Safari via experimental features, I think we'd also want to disable automatic process prewarming.
Comment 7 Chris Dumez 2018-11-26 14:06:27 PST
(In reply to Chris Dumez from comment #6)
> (In reply to Ryosuke Niwa from comment #5)
> > Comment on attachment 355671 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=355671&action=review
> > 
> > > Source/WebKit/UIProcess/WebProcessPool.cpp:1288
> > > -    if (!configuration().isAutomaticProcessWarmingEnabled())
> > > +    if (!configuration().isAutomaticProcessWarmingEnabled() || !configuration().processSwapsOnNavigation())
> > 
> > But aren't there clients which would benefit from pre-warming without
> > enabling PSON?
> > e.g. for opening multiple WKWebViews?
> 
> Automatic process prewarming is not currently API, which is why I am adding
> the heuristic to turn it on automatically when we determine it would be
> beneficial.
> 
> Process-Swap on Navigation is not not currently API and *is* enabled by
> default so clients do get PSON no matter what. They will also get process
> prewarming if our heuristic determines it would be beneficial.
> 
> As the change log explains, the reason I made this particular change is
> because Safari will turn automatic process prewarming on explicitly and rely
> on the experimental feature flag for PSON support. If PSON is disabled in
> Safari via experimental features, I think we'd also want to disable
> automatic process prewarming.

Note that the reason we support disabling PSON via experimental features is to easily identify if bugs are caused by PSON or not. I think we should include process-prewarming bugs in the same category, thus my proposal to disable process prewarming if PSON is disabled in experimental features.
Comment 8 Ryosuke Niwa 2018-11-26 14:26:41 PST
Comment on attachment 355671 [details]
Patch

Okay. Sounds sane to me.
Comment 9 WebKit Commit Bot 2018-11-26 14:54:56 PST
Comment on attachment 355671 [details]
Patch

Clearing flags on attachment: 355671

Committed r238525: <https://trac.webkit.org/changeset/238525>
Comment 10 WebKit Commit Bot 2018-11-26 14:54:58 PST
All reviewed patches have been landed.  Closing bug.