Bug 199195 – Perform less work when a pre-warmed WebProcess is suspended or resumed.

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

This is a confirmed improvement in page load time.

[Per Arne Vollan]

Created attachment 372848 [details]
Patch

[Per Arne Vollan]

Created attachment 372853 [details]
Patch

[Brent Fulgham]

Comment on attachment 372853 [details]
Patch

R=me

[Chris Dumez]

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?

[Chris Dumez]

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?

[Per Arne Vollan]

(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!

[Per Arne Vollan]

Created attachment 373132 [details]
Patch

[Per Arne Vollan]

(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?

[Per Arne Vollan]

Created attachment 373134 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/52349969>

[Per Arne Vollan]

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.

[Sam Weinig]

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(...).

[Per Arne Vollan]

(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?

[Sam Weinig]

(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).

[Per Arne Vollan]

(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!

[Per Arne Vollan]

Created attachment 373186 [details]
Patch

[Darin Adler]

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.

[Per Arne Vollan]

(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!

[Per Arne Vollan]

Created attachment 373234 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 373234 [details]
Patch

Clearing flags on attachment: 373234

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