Bug 184765 – Keep around a pre-warmed process when doing process swap on navigation

• Component: WebKit2
• Status: RESOLVED FIXED

[Saam Barati]

...

[Saam Barati]

Created attachment 338349 [details]
WIP

I still need to figure out what to do w/ the website data store since I don't know what invariants exist around it.

[Ryosuke Niwa]

Comment on attachment 338349 [details]
WIP

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

> Source/WebKit/UIProcess/WebProcessPool.cpp:760
> +        bool& isPrewarmed = pair.second;

Please declare a new struct which clearly states whether a process is prewarmed or not.
Alternatively, we can have two vectors of processes; one for regular ones and another for pre-warmed.

> Source/WebKit/UIProcess/WebProcessPool.cpp:1115
> +    RefPtr<WebProcessProxy> process = tryTakePrewarmedProcess();
> +    if (!process) {
> +        if (pageConfiguration->relatedPage()) {
> +            // Sharing processes, e.g. when creating the page via window.open().
> +            process = &pageConfiguration->relatedPage()->process();

I don't think this is right. If pageConfiguration->relatedPage() is set, we need to reuse the process
regardless of whether there is a pre-warmed process or not.

> Source/WebKit/UIProcess/WebProcessPool.cpp:1263
> +        // OOPS: This is almost certainly wrong w.r.t the store. Figure out what the invariant of that is.
> +        createNewWebProcess(store, true);

We should suspend these web content processes once they're launched.

[Saam Barati]

Comment on attachment 338349 [details]
WIP

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

>> Source/WebKit/UIProcess/WebProcessPool.cpp:760
>> +        bool& isPrewarmed = pair.second;
> 
> Please declare a new struct which clearly states whether a process is prewarmed or not.
> Alternatively, we can have two vectors of processes; one for regular ones and another for pre-warmed.

SGTM

Regarding having two vectors: I originally did that, but it felt more cumbersome.

>> Source/WebKit/UIProcess/WebProcessPool.cpp:1115
>> +            process = &pageConfiguration->relatedPage()->process();
> 
> I don't think this is right. If pageConfiguration->relatedPage() is set, we need to reuse the process
> regardless of whether there is a pre-warmed process or not.

I agree. The old code seemed to have this error when m_haveInititalEmptyProcess was true. That said, we probably never ran into this in practice. I'll fix it.

[Saam Barati]

Created attachment 338394 [details]
WIP

Just a bit of refactoring from previous patch

[Saam Barati]

Created attachment 338464 [details]
WIP

make it compile

[Saam Barati]

Created attachment 338487 [details]
patch

[Saam Barati]

Comment on attachment 338487 [details]
patch

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

> Source/WebKit/ChangeLog:16
> +        This is a 30% progression on PLT with process swap on navigation turned on.

I got this number when testing with a different goodTimeToPrewarm heuristic. I'm going to verify that DidFirstVisuallyNonEmptyLayout gives us the same progression. If the progression lessens, I'll need to edit the heuristic a bit.

[EWS Watchlist]

Attachment 338487 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebProcessPool.h:59:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit/UIProcess/WebProcessPool.h:177:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/WebProcessPool.h:183:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/WebProcessPool.cpp:331:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 338487 [details]
patch

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

>> Source/WebKit/ChangeLog:16
>> +        This is a 30% progression on PLT with process swap on navigation turned on.
> 
> I got this number when testing with a different goodTimeToPrewarm heuristic. I'm going to verify that DidFirstVisuallyNonEmptyLayout gives us the same progression. If the progression lessens, I'll need to edit the heuristic a bit.

I just verified this. It's a 40% progression on PLT with this heuristic. I'll update the changelog to reflect this.

[Ryosuke Niwa]

Comment on attachment 338487 [details]
patch

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

Looks good. You might want to get some sanity checks from Chris/Brady as well.

> Source/WebKit/UIProcess/PerActivityStateCPUUsageSampler.cpp:98
> +    m_processPool.forEachProcess([&] (auto& webProcess) {

Can we add findProcess which takes a lambda and do:
std::optional<WebPageProxy*> proxy = m_processPool.forEachProcess.findProcess([&] (auto& webProcess) { return webProcess.pageCount() ? *webProcess.pages().begin() : std::nullopt; })

> Source/WebKit/UIProcess/WebPageProxy.cpp:494
> +    m_process->processPool().goodTimeToPrewarm();

A function name should be a verb.
I think we should call this prewarmIfNeeded or didReachPrewarmTime or didReachGoodTimeToPrewarm.

> Source/WebKit/UIProcess/WebProcessPool.cpp:328
> +    return m_processes.findMatching([&] (auto& pair) { return pair.process.ptr() == process; }) != WTF::notFound;

Since this is no longer an pair, "item" might be a better variable name here.

> Source/WebKit/UIProcess/WebProcessPool.cpp:644
> -    m_processes.append(WTFMove(serviceWorkerProcessProxy));
> +    m_processes.append(ProcessPair { WTFMove(serviceWorkerProcessProxy), false });

ProcessPair is not a great name since it's not really a pair.
We may also want to add more information there. I think we should call it ProcessEntry or something.
On the other hand, I've started to think that maybe we should be adding an instance variable on WebProcess instead.

> Source/WebKit/UIProcess/WebProcessPool.cpp:764
> +        if (pair.isPrewarmed) {
> +            --m_prewarmedProcessCount;
> +            pair.isPrewarmed = false;

It's a bit strange to set a variable named isPrewarmed to false here.
Loading a web page doesn't change the fact the process has been pre-warmed.
isPrewarmedAndUnused would be better but it's kind of long & wordy.
Maybe isInPrewarmedState to emphasize the fact it's in a specific state?

> Source/WebKit/UIProcess/WebProcessPool.cpp:1037
> +    m_processes.removeFirstMatching([&] (auto& pair) {
> +        if (pair.process.ptr() == process) {

Ditto about the variable name. Use a more sensible variable name like entry instead of pair.

> Source/WebKit/UIProcess/WebProcessPool.cpp:1077
> +    for (auto& pair : m_processes) {

Ditto.

> Source/WebKit/UIProcess/WebProcessPool.cpp:1255
> +void WebProcessPool::goodTimeToPrewarm()

Again, the function name should be a verb.

> Source/WebKit/UIProcess/WebProcessPool.h:157
> +    struct ProcessPair {

Again, this should be of more sensible name like ProcessEntry.
But I think a better approach is to add an instance variable on WebProcessProxy itself.
I don't think there is any reason to have it has a separate state from WebProcessProxy.
In fact, we may need to do something special in WebProcessProxy for pre-warmed processes.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:183
> +    bool isMainFrame = false;
> +    m_processPool->forEachProcess([&] (auto& process) {

I think this would be a lot simpler if we added findProcess function I suggested above.

[Saam Barati]

Comment on attachment 338487 [details]
patch

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

>> Source/WebKit/UIProcess/WebProcessPool.cpp:644
>> +    m_processes.append(ProcessPair { WTFMove(serviceWorkerProcessProxy), false });
> 
> ProcessPair is not a great name since it's not really a pair.
> We may also want to add more information there. I think we should call it ProcessEntry or something.
> On the other hand, I've started to think that maybe we should be adding an instance variable on WebProcess instead.

Yeah we totally should. What I did doesn’t make sense. It’s just leftover from my initial WIP patch. If we make it an ivar, this patch will be a lot smaller since I’ll be able to remove a bunch of the cruft.

>> Source/WebKit/UIProcess/WebProcessPool.cpp:764
>> +            pair.isPrewarmed = false;
> 
> It's a bit strange to set a variable named isPrewarmed to false here.
> Loading a web page doesn't change the fact the process has been pre-warmed.
> isPrewarmedAndUnused would be better but it's kind of long & wordy.
> Maybe isInPrewarmedState to emphasize the fact it's in a specific state?

Right. Mybe the ivar should just be named “isInUse”?

[Ryosuke Niwa]

(In reply to Saam Barati from comment #11)
> Comment on attachment 338487 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338487&action=review
>
> >> Source/WebKit/UIProcess/WebProcessPool.cpp:764
> >> +            pair.isPrewarmed = false;
> > 
> > It's a bit strange to set a variable named isPrewarmed to false here.
> > Loading a web page doesn't change the fact the process has been pre-warmed.
> > isPrewarmedAndUnused would be better but it's kind of long & wordy.
> > Maybe isInPrewarmedState to emphasize the fact it's in a specific state?
> 
> Right. Mybe the ivar should just be named “isInUse”?

InUse is kind of generic/vague too. Maybe hasLoadedPage? or isInPrewarmedPool?

[Saam Barati]

Created attachment 338560 [details]
patch for landing

Will wait for a thumbs up from Brady or Chris before landing.

[Saam Barati]

Created attachment 338561 [details]
patch for landing

rebased

[EWS Watchlist]

Comment on attachment 338561 [details]
patch for landing

Attachment 338561 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7406937

New failing tests:
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 338562 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Chris Dumez]

Comment on attachment 338561 [details]
patch for landing

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

> Source/WebKit/ChangeLog:13
> +        that this patch chooses is when we reach the DidFirstVisuallyNonEmptyLayout

Doesn't DidFirstVisuallyNonEmptyLayout usually happen before the end of the page load? If so, isn't it too soon to start pre-warning another process? Wouldn't it slow down the current load?

> Source/WebKit/UIProcess/WebPageProxy.cpp:3867
> +        notifyProcessPoolToPrewarm();

Seems like this could go into WebPageProxy::didFirstVisuallyNonEmptyLayoutForFrame(), then we wouldn't need the `layoutMilestones & DidFirstVisuallyNonEmptyLayout` check.

> Source/WebKit/UIProcess/WebProcessPool.cpp:739
> +WebProcessProxy& WebProcessPool::createNewWebProcess(WebsiteDataStore& websiteDataStore, bool isInPrewarmedPool)

This parameter should not be a boolean but a enum class so call sites look more readable.

> Source/WebKit/UIProcess/WebProcessProxy.cpp:108
> +Ref<WebProcessProxy> WebProcessProxy::create(WebProcessPool& processPool, WebsiteDataStore& websiteDataStore, bool isInPrewarmedPool)

Should be an enum class rather than a boolean.

[Saam Barati]

Comment on attachment 338561 [details]
patch for landing

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

>> Source/WebKit/ChangeLog:13
>> +        that this patch chooses is when we reach the DidFirstVisuallyNonEmptyLayout
> 
> Doesn't DidFirstVisuallyNonEmptyLayout usually happen before the end of the page load? If so, isn't it too soon to start pre-warning another process? Wouldn't it slow down the current load?

What would you consider the end of the page load?

>> Source/WebKit/UIProcess/WebPageProxy.cpp:3867
>> +        notifyProcessPoolToPrewarm();
> 
> Seems like this could go into WebPageProxy::didFirstVisuallyNonEmptyLayoutForFrame(), then we wouldn't need the `layoutMilestones & DidFirstVisuallyNonEmptyLayout` check.

That’s not the only caller of this, which is why I did it this way. I forget what the other callers are, but it seems logically more relevant here.

>> Source/WebKit/UIProcess/WebProcessPool.cpp:739
>> +WebProcessProxy& WebProcessPool::createNewWebProcess(WebsiteDataStore& websiteDataStore, bool isInPrewarmedPool)
> 
> This parameter should not be a boolean but a enum class so call sites look more readable.

SGTM, how about:
enum class IsInPrewarmedPool { No, Yes }

[Chris Dumez]

Comment on attachment 338561 [details]
patch for landing

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

>>> Source/WebKit/ChangeLog:13
>>> +        that this patch chooses is when we reach the DidFirstVisuallyNonEmptyLayout
>> 
>> Doesn't DidFirstVisuallyNonEmptyLayout usually happen before the end of the page load? If so, isn't it too soon to start pre-warning another process? Wouldn't it slow down the current load?
> 
> What would you consider the end of the page load?

didFinishLoadForFrame() for the main frame, which should match with the 'load' event in JS iirc.

>>> Source/WebKit/UIProcess/WebPageProxy.cpp:3867
>>> +        notifyProcessPoolToPrewarm();
>> 
>> Seems like this could go into WebPageProxy::didFirstVisuallyNonEmptyLayoutForFrame(), then we wouldn't need the `layoutMilestones & DidFirstVisuallyNonEmptyLayout` check.
> 
> That’s not the only caller of this, which is why I did it this way. I forget what the other callers are, but it seems logically more relevant here.

Seems like it would be a bug if this was getting called for DidFirstVisuallyNonEmptyLayout but WebPageProxy::didFirstVisuallyNonEmptyLayoutForFrame() wasn't called...

>>> Source/WebKit/UIProcess/WebProcessPool.cpp:739
>>> +WebProcessProxy& WebProcessPool::createNewWebProcess(WebsiteDataStore& websiteDataStore, bool isInPrewarmedPool)
>> 
>> This parameter should not be a boolean but a enum class so call sites look more readable.
> 
> SGTM, how about:
> enum class IsInPrewarmedPool { No, Yes }

SGTM.

[Chris Dumez]

Comment on attachment 338561 [details]
patch for landing

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

>>>> Source/WebKit/ChangeLog:13
>>>> +        that this patch chooses is when we reach the DidFirstVisuallyNonEmptyLayout
>>> 
>>> Doesn't DidFirstVisuallyNonEmptyLayout usually happen before the end of the page load? If so, isn't it too soon to start pre-warning another process? Wouldn't it slow down the current load?
>> 
>> What would you consider the end of the page load?
> 
> didFinishLoadForFrame() for the main frame, which should match with the 'load' event in JS iirc.

Note that my personal preference would have been on a timer after the load event, which is what we've done so far (e.g. reader mode detection) to avoid regressing page load time.

[Saam Barati]

I spoke with Chris offline. We're going to investigate a better heuristic for when to prewarm a process in a follow up patch:

https://bugs.webkit.org/show_bug.cgi?id=184899

[Saam Barati]

Comment on attachment 338561 [details]
patch for landing

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

>>>> Source/WebKit/UIProcess/WebPageProxy.cpp:3867
>>>> +        notifyProcessPoolToPrewarm();
>>> 
>>> Seems like this could go into WebPageProxy::didFirstVisuallyNonEmptyLayoutForFrame(), then we wouldn't need the `layoutMilestones & DidFirstVisuallyNonEmptyLayout` check.
>> 
>> That’s not the only caller of this, which is why I did it this way. I forget what the other callers are, but it seems logically more relevant here.
> 
> Seems like it would be a bug if this was getting called for DidFirstVisuallyNonEmptyLayout but WebPageProxy::didFirstVisuallyNonEmptyLayoutForFrame() wasn't called...

Agreed. I moved it into didFirstVisuallyNonEmptyLayoutForFrame()

[Saam Barati]

Created attachment 338623 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 338623 [details]
patch for landing

Clearing flags on attachment: 338623

Committed r230938: <https://trac.webkit.org/changeset/230938>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

This change caused 2 API test failures on iOS and macOS.

Tests that failed:
  ProcessSwap.OnePreviousProcessRemains
  ProcessSwap.PageCache1

https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/4169

[Saam Barati]

(In reply to Ryan Haddad from comment #26)
> This change caused 2 API test failures on iOS and macOS.
> 
> Tests that failed:
>   ProcessSwap.OnePreviousProcessRemains
>   ProcessSwap.PageCache1
> 
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/4169

Will look at these once I get into the office.

If you need to rollout before then that’s ok

[Radar WebKit Bug Importer]

<rdar://problem/39685099>

[Ryan Haddad]

Reverted r230938 for reason:

Introduced two ProcessSwap API test failures.

Committed r230957: <https://trac.webkit.org/changeset/230957>

[Saam Barati]

Created attachment 338663 [details]
patch for landing

Fixed broken API tests and added a new one

[Brady Eidson]

Comment on attachment 338663 [details]
patch for landing

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

Seems legit

> Source/WebKit/UIProcess/WebProcessPool.h:110
> +class WebProcessPool final : public API::ObjectImpl<API::Object::Type::ProcessPool>, private IPC::MessageReceiver {
> +

Could we nuke this empty line?

[Saam Barati]

Comment on attachment 338663 [details]
patch for landing

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

>> Source/WebKit/UIProcess/WebProcessPool.h:110
>> +
> 
> Could we nuke this empty line?

done

[Saam Barati]

Created attachment 338667 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 338667 [details]
patch for landing

Clearing flags on attachment: 338667

Committed r230977: <https://trac.webkit.org/changeset/230977>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.