Bug 191966 - [PSON] process pre-warming should not be on for everyone
Summary: [PSON] process pre-warming should not be on for everyone
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-26 09:07 PST by Chris Dumez
Modified: 2018-11-26 14:54 PST (History)
6 users (show)

See Also:


Attachments
Patch (8.41 KB, patch)
2018-11-26 13:53 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.