[webkit-reviews] review granted: [Bug 184765] Keep around a pre-warmed process when doing process swap on navigation : [Attachment 338487] patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Saam Barati <sbarati at apple.com>'s
request for review:
Bug 184765: Keep around a pre-warmed process when doing process swap on
navigation
https://bugs.webkit.org/show_bug.cgi?id=184765

Attachment 338487: patch

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




--- Comment #10 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 338487
  --> https://bugs.webkit.org/attachment.cgi?id=338487
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.