Bug 180374 – Don't force creation of process pool when enabling resource load statistics

• Component: WebKit2
• Status: RESOLVED FIXED

[Brent Fulgham]

When activating resource load statistics, we should avoid forcing the creation of a new process pool if it doesn't yet exist. We will properly set the value when initializing the first web view, so there is no point in paying the construction cost for the process pool during initial launch.

[Brent Fulgham]

<rdar://problem/35545639>

[Brent Fulgham]

Created attachment 328382 [details]
Patch

[Alex Christensen]

Comment on attachment 328382 [details]
Patch

Reading the code, I don't understand what it does or what it means.  It's very not straightforward and if I wrote more code I would make this same mistake.
This has no tests.

[Brent Fulgham]

(In reply to Alex Christensen from comment #3)
> Comment on attachment 328382 [details]
> Patch
> 
> Reading the code, I don't understand what it does or what it means.  It's
> very not straightforward and if I wrote more code I would make this same
> mistake.
> This has no tests.

I'm not sure I follow the r- here. You're saying the patch is bad because it uses the existing (but confusing) API to fix the bug I was trying to resolve? It might be clearer if you said "please create a brand new API that is not confusing".

The functionality of activating resource load statistics is covered by existing tests, which should not fail after this patch.

We do not have testing infrastructure (supported by our LayoutTest) for measuring startup cost, which is what this is trying to resolve.

[Geoffrey Garen]

Comment on attachment 328382 [details]
Patch

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

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1323
> +    for (auto& processPool : processPools(1, false))

You can make this code clearer by putting 1 and false into local named variables that reflect their meanings.

It's weird to write this code as a loop since the 1 you've passed to processPools guarantees there will be only one item.

Maybe you can just reuse the processPoolForCookieStorageOperations() function here. But you're not doing cookie operations, so it needs a clearer name for its purpose. Maybe "anyProcessPool()".

[Brent Fulgham]

Created attachment 328386 [details]
Patch

[Brent Fulgham]

Comment on attachment 328382 [details]
Patch

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

>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1323
>> +    for (auto& processPool : processPools(1, false))
> 
> You can make this code clearer by putting 1 and false into local named variables that reflect their meanings.
> 
> It's weird to write this code as a loop since the 1 you've passed to processPools guarantees there will be only one item.
> 
> Maybe you can just reuse the processPoolForCookieStorageOperations() function here. But you're not doing cookie operations, so it needs a clearer name for its purpose. Maybe "anyProcessPool()".

Yeah -- this is wrong. I do want to get whatever pools exist -- I just don't want to create a process pool if it doesn't exist yet.

I was thinking about making a new method wrapper called "existingProcessPools()" that would do this:

return processPools(std::numeric_limits<size_t>::max(), false);

[Brent Fulgham]

Created attachment 328388 [details]
Patch

[Brent Fulgham]

Committed r225508: <https://trac.webkit.org/changeset/225508>

[Geoffrey Garen]

Still think it would be nice to name the arguments here.