180374 2017-12-04 13:22:01 -0800 Don't force creation of process pool when enabling resource load statistics 2017-12-05 14:36:41 -0800 1 1 1 Unclassified WebKit WebKit2 WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 bfulgham bfulgham achristensen aestes bfulgham cdumez ggaren oldest_to_newest 1377854 0 bfulgham 2017-12-04 13:22:01 -0800 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. 1377855 1 bfulgham 2017-12-04 13:22:16 -0800 <rdar://problem/35545639> 1377862 2 328382 bfulgham 2017-12-04 13:26:28 -0800 Created attachment 328382 Patch 1377864 3 328382 achristensen 2017-12-04 13:29:20 -0800 Comment on attachment 328382 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. 1377868 4 bfulgham 2017-12-04 13:37:04 -0800 (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. 1377882 5 328382 ggaren 2017-12-04 13:49:30 -0800 Comment on attachment 328382 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()". 1377901 6 328386 bfulgham 2017-12-04 14:07:00 -0800 Created attachment 328386 Patch 1377903 7 328382 bfulgham 2017-12-04 14:09:25 -0800 Comment on attachment 328382 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); 1377906 8 328388 bfulgham 2017-12-04 14:11:01 -0800 Created attachment 328388 Patch 1378016 9 bfulgham 2017-12-04 17:11:54 -0800 Committed r225508: <https://trac.webkit.org/changeset/225508> 1378376 10 ggaren 2017-12-05 14:36:41 -0800 Still think it would be nice to name the arguments here. 328382 2017-12-04 13:26:28 -0800 2017-12-04 14:06:59 -0800 Patch bug-180374-20171204132627.patch text/plain 1638 bfulgham SW5kZXg6IFNvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJL aXQvQ2hhbmdlTG9nCShyZXZpc2lvbiAyMjU0ODcpCisrKyBTb3VyY2UvV2ViS2l0L0NoYW5nZUxv Zwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBACisyMDE3LTEyLTA0ICBCcmVudCBGdWxn aGFtICA8YmZ1bGdoYW1AYXBwbGUuY29tPgorCisgICAgICAgIERvbid0IGZvcmNlIGNyZWF0aW9u IG9mIHByb2Nlc3MgcG9vbCB3aGVuIGVuYWJsaW5nIHJlc291cmNlIGxvYWQgc3RhdGlzdGljcwor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTgwMzc0Cisg ICAgICAgIDxyZGFyOi8vcHJvYmxlbS8zNTU0NTYzOT4KKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICBQYXNzIGFwcHJvcHJpYXRlIGFyZ3VtZW50cyB0byB0 aGUgJ3Byb2Nlc3NQb29sJyBhY2Nlc3NvciBzbyB0aGF0IHdlIG9ubHkgZ2V0IHRoZQorICAgICAg ICBzZXQgb2YgZXhpc3RpbmcgcHJvY2VzcyBwb29scy4gV2UgZG9uJ3Qgd2FudCB0byBwYXkgdGhl IGNvc3Qgb2YgbGF1bmNoaW5nIG5ldworICAgICAgICBwcm9jZXNzIHBvb2xzIGp1c3QgdG8gc2V0 IHRoaXMgZmxhZy4gVGhlIGZsYWcgd2lsbCBiZSBwcm9wZXJseSBzZXQgd2hlbiB0aGUgcG9vbHMK KyAgICAgICAgYXJlIGNyZWF0ZWQgYXMgc29vbiBhcyBhIFdlYlZpZXcgaXMgaW5zdGFudGlhdGVk LgorCisgICAgICAgICogVUlQcm9jZXNzL1dlYnNpdGVEYXRhL1dlYnNpdGVEYXRhU3RvcmUuY3Bw OgorICAgICAgICAoV2ViS2l0OjpXZWJzaXRlRGF0YVN0b3JlOjpzZXRSZXNvdXJjZUxvYWRTdGF0 aXN0aWNzRW5hYmxlZCk6CisKIDIwMTctMTItMDQgIEZyZWRlcmljIFdhbmcgIDxmd2FuZ0BpZ2Fs aWEuY29tPgogCiAgICAgICAgIE1ha2UgU2Nyb2xsaW5nVHJlZU5vZGU6OmVuY2xvc2luZ0ZyYW1l Tm9kZSByZXR1cm4gdGhlIG5vZGUgaXRzZWxmIGZvciBmcmFtZSBub2RlcwpJbmRleDogU291cmNl L1dlYktpdC9VSVByb2Nlc3MvV2Vic2l0ZURhdGEvV2Vic2l0ZURhdGFTdG9yZS5jcHAKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gU291cmNlL1dlYktpdC9VSVByb2Nlc3MvV2Vic2l0ZURhdGEvV2Vic2l0ZURhdGFT dG9yZS5jcHAJKHJldmlzaW9uIDIyNDg4OCkKKysrIFNvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL1dl YnNpdGVEYXRhL1dlYnNpdGVEYXRhU3RvcmUuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xMzIwLDcg KzEzMjAsNyBAQCB2b2lkIFdlYnNpdGVEYXRhU3RvcmU6OnNldFJlc291cmNlTG9hZFN0CiAgICAg fQogCiAgICAgbV9yZXNvdXJjZUxvYWRTdGF0aXN0aWNzID0gbnVsbHB0cjsKLSAgICBmb3IgKGF1 dG8mIHByb2Nlc3NQb29sIDogcHJvY2Vzc1Bvb2xzKCkpCisgICAgZm9yIChhdXRvJiBwcm9jZXNz UG9vbCA6IHByb2Nlc3NQb29scygxLCBmYWxzZSkpCiAgICAgICAgIHByb2Nlc3NQb29sLT5zZXRS ZXNvdXJjZUxvYWRTdGF0aXN0aWNzRW5hYmxlZChmYWxzZSk7CiB9CiAK 328386 2017-12-04 14:07:00 -0800 2017-12-04 14:11:00 -0800 Patch bug-180374-20171204140700.patch text/plain 1728 bfulgham SW5kZXg6IFNvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJL aXQvQ2hhbmdlTG9nCShyZXZpc2lvbiAyMjU0ODcpCisrKyBTb3VyY2UvV2ViS2l0L0NoYW5nZUxv Zwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBACisyMDE3LTEyLTA0ICBCcmVudCBGdWxn aGFtICA8YmZ1bGdoYW1AYXBwbGUuY29tPgorCisgICAgICAgIERvbid0IGZvcmNlIGNyZWF0aW9u IG9mIHByb2Nlc3MgcG9vbCB3aGVuIGVuYWJsaW5nIHJlc291cmNlIGxvYWQgc3RhdGlzdGljcwor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTgwMzc0Cisg ICAgICAgIDxyZGFyOi8vcHJvYmxlbS8zNTU0NTYzOT4KKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICBQYXNzIGFwcHJvcHJpYXRlIGFyZ3VtZW50cyB0byB0 aGUgJ3Byb2Nlc3NQb29sJyBhY2Nlc3NvciBzbyB0aGF0IHdlIG9ubHkgZ2V0IHRoZQorICAgICAg ICBzZXQgb2YgZXhpc3RpbmcgcHJvY2VzcyBwb29scy4gV2UgZG9uJ3Qgd2FudCB0byBwYXkgdGhl IGNvc3Qgb2YgbGF1bmNoaW5nIG5ldworICAgICAgICBwcm9jZXNzIHBvb2xzIGp1c3QgdG8gc2V0 IHRoaXMgZmxhZy4gVGhlIGZsYWcgd2lsbCBiZSBwcm9wZXJseSBzZXQgd2hlbiB0aGUgcG9vbHMK KyAgICAgICAgYXJlIGNyZWF0ZWQgYXMgc29vbiBhcyBhIFdlYlZpZXcgaXMgaW5zdGFudGlhdGVk LgorCisgICAgICAgICogVUlQcm9jZXNzL1dlYnNpdGVEYXRhL1dlYnNpdGVEYXRhU3RvcmUuY3Bw OgorICAgICAgICAoV2ViS2l0OjpXZWJzaXRlRGF0YVN0b3JlOjpzZXRSZXNvdXJjZUxvYWRTdGF0 aXN0aWNzRW5hYmxlZCk6CisKIDIwMTctMTItMDQgIEZyZWRlcmljIFdhbmcgIDxmd2FuZ0BpZ2Fs aWEuY29tPgogCiAgICAgICAgIE1ha2UgU2Nyb2xsaW5nVHJlZU5vZGU6OmVuY2xvc2luZ0ZyYW1l Tm9kZSByZXR1cm4gdGhlIG5vZGUgaXRzZWxmIGZvciBmcmFtZSBub2RlcwpJbmRleDogU291cmNl L1dlYktpdC9VSVByb2Nlc3MvV2Vic2l0ZURhdGEvV2Vic2l0ZURhdGFTdG9yZS5jcHAKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gU291cmNlL1dlYktpdC9VSVByb2Nlc3MvV2Vic2l0ZURhdGEvV2Vic2l0ZURhdGFT dG9yZS5jcHAJKHJldmlzaW9uIDIyNDg4OCkKKysrIFNvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL1dl YnNpdGVEYXRhL1dlYnNpdGVEYXRhU3RvcmUuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xMzIwLDcg KzEzMjAsOSBAQCB2b2lkIFdlYnNpdGVEYXRhU3RvcmU6OnNldFJlc291cmNlTG9hZFN0CiAgICAg fQogCiAgICAgbV9yZXNvdXJjZUxvYWRTdGF0aXN0aWNzID0gbnVsbHB0cjsKLSAgICBmb3IgKGF1 dG8mIHByb2Nlc3NQb29sIDogcHJvY2Vzc1Bvb2xzKCkpCisKKyAgICBhdXRvIGV4aXN0aW5nUHJv Y2Vzc1Bvb2xzID0gcHJvY2Vzc1Bvb2xzKHN0ZDo6bnVtZXJpY19saW1pdHM8c2l6ZV90Pjo6bWF4 KCksIGZhbHNlKS4KKyAgICBmb3IgKGF1dG8mIHByb2Nlc3NQb29sIDogZXhpc3RpbmdQcm9jZXNz UG9vbHMpCiAgICAgICAgIHByb2Nlc3NQb29sLT5zZXRSZXNvdXJjZUxvYWRTdGF0aXN0aWNzRW5h YmxlZChmYWxzZSk7CiB9CiAK 328388 2017-12-04 14:11:01 -0800 2017-12-04 16:13:23 -0800 Patch bug-180374-20171204141100.patch text/plain 1728 bfulgham SW5kZXg6IFNvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJL aXQvQ2hhbmdlTG9nCShyZXZpc2lvbiAyMjU0ODcpCisrKyBTb3VyY2UvV2ViS2l0L0NoYW5nZUxv Zwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBACisyMDE3LTEyLTA0ICBCcmVudCBGdWxn aGFtICA8YmZ1bGdoYW1AYXBwbGUuY29tPgorCisgICAgICAgIERvbid0IGZvcmNlIGNyZWF0aW9u IG9mIHByb2Nlc3MgcG9vbCB3aGVuIGVuYWJsaW5nIHJlc291cmNlIGxvYWQgc3RhdGlzdGljcwor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTgwMzc0Cisg ICAgICAgIDxyZGFyOi8vcHJvYmxlbS8zNTU0NTYzOT4KKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICBQYXNzIGFwcHJvcHJpYXRlIGFyZ3VtZW50cyB0byB0 aGUgJ3Byb2Nlc3NQb29sJyBhY2Nlc3NvciBzbyB0aGF0IHdlIG9ubHkgZ2V0IHRoZQorICAgICAg ICBzZXQgb2YgZXhpc3RpbmcgcHJvY2VzcyBwb29scy4gV2UgZG9uJ3Qgd2FudCB0byBwYXkgdGhl IGNvc3Qgb2YgbGF1bmNoaW5nIG5ldworICAgICAgICBwcm9jZXNzIHBvb2xzIGp1c3QgdG8gc2V0 IHRoaXMgZmxhZy4gVGhlIGZsYWcgd2lsbCBiZSBwcm9wZXJseSBzZXQgd2hlbiB0aGUgcG9vbHMK KyAgICAgICAgYXJlIGNyZWF0ZWQgYXMgc29vbiBhcyBhIFdlYlZpZXcgaXMgaW5zdGFudGlhdGVk LgorCisgICAgICAgICogVUlQcm9jZXNzL1dlYnNpdGVEYXRhL1dlYnNpdGVEYXRhU3RvcmUuY3Bw OgorICAgICAgICAoV2ViS2l0OjpXZWJzaXRlRGF0YVN0b3JlOjpzZXRSZXNvdXJjZUxvYWRTdGF0 aXN0aWNzRW5hYmxlZCk6CisKIDIwMTctMTItMDQgIEZyZWRlcmljIFdhbmcgIDxmd2FuZ0BpZ2Fs aWEuY29tPgogCiAgICAgICAgIE1ha2UgU2Nyb2xsaW5nVHJlZU5vZGU6OmVuY2xvc2luZ0ZyYW1l Tm9kZSByZXR1cm4gdGhlIG5vZGUgaXRzZWxmIGZvciBmcmFtZSBub2RlcwpJbmRleDogU291cmNl L1dlYktpdC9VSVByb2Nlc3MvV2Vic2l0ZURhdGEvV2Vic2l0ZURhdGFTdG9yZS5jcHAKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gU291cmNlL1dlYktpdC9VSVByb2Nlc3MvV2Vic2l0ZURhdGEvV2Vic2l0ZURhdGFT dG9yZS5jcHAJKHJldmlzaW9uIDIyNDg4OCkKKysrIFNvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL1dl YnNpdGVEYXRhL1dlYnNpdGVEYXRhU3RvcmUuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xMzIwLDcg KzEzMjAsOSBAQCB2b2lkIFdlYnNpdGVEYXRhU3RvcmU6OnNldFJlc291cmNlTG9hZFN0CiAgICAg fQogCiAgICAgbV9yZXNvdXJjZUxvYWRTdGF0aXN0aWNzID0gbnVsbHB0cjsKLSAgICBmb3IgKGF1 dG8mIHByb2Nlc3NQb29sIDogcHJvY2Vzc1Bvb2xzKCkpCisKKyAgICBhdXRvIGV4aXN0aW5nUHJv Y2Vzc1Bvb2xzID0gcHJvY2Vzc1Bvb2xzKHN0ZDo6bnVtZXJpY19saW1pdHM8c2l6ZV90Pjo6bWF4 KCksIGZhbHNlKTsKKyAgICBmb3IgKGF1dG8mIHByb2Nlc3NQb29sIDogZXhpc3RpbmdQcm9jZXNz UG9vbHMpCiAgICAgICAgIHByb2Nlc3NQb29sLT5zZXRSZXNvdXJjZUxvYWRTdGF0aXN0aWNzRW5h YmxlZChmYWxzZSk7CiB9CiAK