Bug 199195 - Perform less work when a pre-warmed WebProcess is suspended or resumed.
Summary: Perform less work when a pre-warmed WebProcess is suspended or resumed.
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-25 11:18 PDT by Per Arne Vollan
Modified: 2019-07-01 08:34 PDT (History)
8 users (show)

See Also:


Attachments
Patch (1.99 KB, patch)
2019-06-25 11:21 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (2.07 KB, patch)
2019-06-25 13:05 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (2.36 KB, patch)
2019-06-28 11:00 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (2.43 KB, patch)
2019-06-28 11:09 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (2.93 KB, patch)
2019-06-29 21:47 PDT, Per Arne Vollan
darin: review+
Details | Formatted Diff | Diff
Patch (2.47 KB, patch)
2019-07-01 07:42 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2019-06-25 11:18:10 PDT
This is a confirmed improvement in page load time.
Comment 1 Per Arne Vollan 2019-06-25 11:21:34 PDT
Created attachment 372848 [details]
Patch
Comment 2 Per Arne Vollan 2019-06-25 13:05:26 PDT
Created attachment 372853 [details]
Patch
Comment 3 Brent Fulgham 2019-06-26 07:42:46 PDT
Comment on attachment 372853 [details]
Patch

R=me
Comment 4 Chris Dumez 2019-06-26 07:43:29 PDT
Comment on attachment 372853 [details]
Patch

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

> Source/WebKit/WebProcess/WebProcess.cpp:1476
> +        return;

Seems like it should do something like this before returning:
if (shouldAcknowledgeWhenReadyToSuspend == ShouldAcknowledgeWhenReadyToSuspend::Yes) {
    RELEASE_LOG(ProcessSuspension, "%p - WebProcess::actualPrepareToSuspend() Sending ProcessReadyToSuspend IPC message", this);
    parentProcessConnection()->send(Messages::WebProcessProxy::ProcessReadyToSuspend(), 0);
}

> Source/WebKit/WebProcess/WebProcess.cpp:1536
> +    ASSERT(m_processType != ProcessType::PrewarmedWebContent);

I am not convinced this assert is correct. Why do you think it is?
Comment 5 Chris Dumez 2019-06-26 07:45:46 PDT
Comment on attachment 372853 [details]
Patch

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

> Source/WebKit/ChangeLog:8
> +        Return early from WebProcess::actualPrepareToSuspend and WebProcess::processDidResume

Have you tried not doing the IPC round trip altogether?
Comment 6 Per Arne Vollan 2019-06-26 11:21:28 PDT
(In reply to Chris Dumez from comment #5)
> Comment on attachment 372853 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=372853&action=review
> 
> > Source/WebKit/ChangeLog:8
> > +        Return early from WebProcess::actualPrepareToSuspend and WebProcess::processDidResume
> 
> Have you tried not doing the IPC round trip altogether?

I will try that. Thanks for reviewing, everyone!
Comment 7 Per Arne Vollan 2019-06-28 11:00:31 PDT
Created attachment 373132 [details]
Patch
Comment 8 Per Arne Vollan 2019-06-28 11:03:29 PDT
(In reply to Chris Dumez from comment #5)
> Comment on attachment 372853 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=372853&action=review
> 
> > Source/WebKit/ChangeLog:8
> > +        Return early from WebProcess::actualPrepareToSuspend and WebProcess::processDidResume
> 
> Have you tried not doing the IPC round trip altogether?

I am still evaluating a patch for this, and might propose a patch later on depending on the results.

Perhaps we also could consider suspending the pre-warmed process after some timeout, instead of an immediate suspension?
Comment 9 Per Arne Vollan 2019-06-28 11:09:51 PDT
Created attachment 373134 [details]
Patch
Comment 10 Radar WebKit Bug Importer 2019-06-28 13:43:58 PDT
<rdar://problem/52349969>
Comment 11 Per Arne Vollan 2019-06-28 16:37:58 PDT
It seems the performance benefit of not doing the IPC roundtrip is comparable to the approach in the current patch, so I think we can stick with this approach.
Comment 12 Sam Weinig 2019-06-29 09:48:06 PDT
Comment on attachment 373134 [details]
Patch

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

> Source/WebKit/WebProcess/WebProcess.cpp:1474
> +#if PLATFORM(COCOA)

What makes this cocoa specific? Can this have a more specific conditional? #if SUPPORTS(...).
Comment 13 Per Arne Vollan 2019-06-29 12:53:54 PDT
(In reply to Sam Weinig from comment #12)
> Comment on attachment 373134 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373134&action=review
> 
> > Source/WebKit/WebProcess/WebProcess.cpp:1474
> > +#if PLATFORM(COCOA)
> 
> What makes this cocoa specific? Can this have a more specific conditional?
> #if SUPPORTS(...).

Currently, m_processType is only declared for cocoa platforms. Perhaps we could declare it for all platforms, and let it have the default value 'ProcessType::WebContent' for platforms not supporting pre-warming?
Comment 14 Sam Weinig 2019-06-29 21:24:14 PDT
(In reply to Per Arne Vollan from comment #13)
> (In reply to Sam Weinig from comment #12)
> > Comment on attachment 373134 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=373134&action=review
> > 
> > > Source/WebKit/WebProcess/WebProcess.cpp:1474
> > > +#if PLATFORM(COCOA)
> > 
> > What makes this cocoa specific? Can this have a more specific conditional?
> > #if SUPPORTS(...).
> 
> Currently, m_processType is only declared for cocoa platforms. Perhaps we
> could declare it for all platforms, and let it have the default value
> 'ProcessType::WebContent' for platforms not supporting pre-warming?

Declaring it for all platforms would be fine and/or guarding it with a more specific conditional. PLATFORM(COCOA) should only be used for conditions that are really Cocoa specific (e.g. requires NSColor).
Comment 15 Per Arne Vollan 2019-06-29 21:38:43 PDT
(In reply to Sam Weinig from comment #14)
> (In reply to Per Arne Vollan from comment #13)
> > (In reply to Sam Weinig from comment #12)
> > > Comment on attachment 373134 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=373134&action=review
> > > 
> > > > Source/WebKit/WebProcess/WebProcess.cpp:1474
> > > > +#if PLATFORM(COCOA)
> > > 
> > > What makes this cocoa specific? Can this have a more specific conditional?
> > > #if SUPPORTS(...).
> > 
> > Currently, m_processType is only declared for cocoa platforms. Perhaps we
> > could declare it for all platforms, and let it have the default value
> > 'ProcessType::WebContent' for platforms not supporting pre-warming?
> 
> Declaring it for all platforms would be fine and/or guarding it with a more
> specific conditional. PLATFORM(COCOA) should only be used for conditions
> that are really Cocoa specific (e.g. requires NSColor).

Sounds good! I will update the patch.

Thanks for reviewing!
Comment 16 Per Arne Vollan 2019-06-29 21:47:13 PDT
Created attachment 373186 [details]
Patch
Comment 17 Darin Adler 2019-06-30 13:14:14 PDT
Comment on attachment 373186 [details]
Patch

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

> Source/WebKit/WebProcess/WebProcess.h:-552
> -#if PLATFORM(COCOA)
>      enum class ProcessType { Inspector, ServiceWorker, PrewarmedWebContent, CachedWebContent, WebContent };
>      ProcessType m_processType { ProcessType::WebContent };
> -#endif

What motivates this change? The rest of the m_processType code is inside #if PLATFORM(MAC/COCOA/IOS_FAMILY) conditionals. That includes code in WebProcess functions initializeWebProcess, setIsInProcessCache, and markIsNoLongerPrewarmed. Why the change in strategy only for this newly written code? We could have removed some of those other conditionals too if we are deciding to change our conditionals strategy.

Typically I would suggest staying with the #if PLATFORM(COCOA) or remove more of it, not leave it mixed.
Comment 18 Per Arne Vollan 2019-07-01 07:37:34 PDT
(In reply to Darin Adler from comment #17)
> Comment on attachment 373186 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373186&action=review
> 
> > Source/WebKit/WebProcess/WebProcess.h:-552
> > -#if PLATFORM(COCOA)
> >      enum class ProcessType { Inspector, ServiceWorker, PrewarmedWebContent, CachedWebContent, WebContent };
> >      ProcessType m_processType { ProcessType::WebContent };
> > -#endif
> 
> What motivates this change? The rest of the m_processType code is inside #if
> PLATFORM(MAC/COCOA/IOS_FAMILY) conditionals. That includes code in
> WebProcess functions initializeWebProcess, setIsInProcessCache, and
> markIsNoLongerPrewarmed. Why the change in strategy only for this newly
> written code? We could have removed some of those other conditionals too if
> we are deciding to change our conditionals strategy.
> 
> Typically I would suggest staying with the #if PLATFORM(COCOA) or remove
> more of it, not leave it mixed.

I will add '#if PLATFORM(COCOA)' back for consistency. I created https://bugs.webkit.org/show_bug.cgi?id=199362 to declare m_processType for all platforms, since I think the related code could be cross platform.

Thanks for reviewing!
Comment 19 Per Arne Vollan 2019-07-01 07:42:12 PDT
Created attachment 373234 [details]
Patch
Comment 20 WebKit Commit Bot 2019-07-01 08:34:22 PDT
Comment on attachment 373234 [details]
Patch

Clearing flags on attachment: 373234

Committed r247009: <https://trac.webkit.org/changeset/247009>