Bug 199195

Created attachment 372848 [details] Patch

Created attachment 372853 [details] Patch

Comment on attachment 372853 [details] Patch R=me

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

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

Created attachment 373132 [details] Patch

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

Created attachment 373134 [details] Patch

<rdar://problem/52349969>

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

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

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

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

Created attachment 373186 [details] Patch

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.

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

Created attachment 373234 [details] Patch

Comment on attachment 373234 [details] Patch Clearing flags on attachment: 373234 Committed r247009: <https://trac.webkit.org/changeset/247009>