207882 2020-02-18 00:57:28 -0800 NetworkCache should use 4KB threshold for mmap-ed files instead of 16KB 2020-02-20 14:23:41 -0800 1 1 1 Unclassified WebKit Page Loading WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=208004 InRadar P2 Normal --- 1 ysuzuki ysuzuki achristensen beidson cdumez cgarcia ews-watchlist ggaren koivisto Lawrence.j webkit-bug-importer oldest_to_newest 1620090 0 ysuzuki 2020-02-18 00:57:28 -0800 If the file size is smaller than the threshold (16KB), we put metadata, header, and body into one file, and disabling mmap feature for this. Previously, we are using mmap path for 4KB~ data in macOS. This sounds like a memory regression. Disabling mmap means that we have copy of content in memory in WebProcess, and this content cannot be discarded easily because it is not file-backed. File-backed mmap-ed content can be potentially discarded efficiently by OS (just purging the content and wiring it to disk). We should first try 4KB threshold in A/B test to see the effect. If this is effective, 1. We should consider decreasing this threshold in macOS since page size of macOS is 4KB. 2. We should try mmap-ping inlined body too. mmapping a part of metadata/header too, and using this file handle w/ offset. 1620097 1 koivisto 2020-02-18 02:13:59 -0800 Mmaps are not free. We have had problems with running out of file descriptors in the past and kernel people have complained about excessive use. It is anyway questionable if using them for small data chuncks is anything other than a memory benchmark hack. Only blobs are designed to be mappable, with mmap you need to be very careful about file mutations. If we wanted smaller chunks to be mappable we'd just change the inline size threshold. I don't think this is something we should ever do. 1620106 2 ysuzuki 2020-02-18 03:11:23 -0800 (In reply to Antti Koivisto from comment #1) > Mmaps are not free. We have had problems with running out of file > descriptors in the past and kernel people have complained about excessive > use. It is anyway questionable if using them for small data chuncks is > anything other than a memory benchmark hack. > > Only blobs are designed to be mappable, with mmap you need to be very > careful about file mutations. If we wanted smaller chunks to be mappable > we'd just change the inline size threshold. > > I don't think this is something we should ever do. I think we should try 4KB threshold in this case, because it was the previous configuration and it matches against the page size in macOS. I found a lot of images are using Vector for backing store in Membuster. My guess is that this is because our 16KB threshold is too large, and ends up having many of memory backed resources. I’m currently running A/B test which just configures this threshold to 4KB. If this shows improvement, I think we should do. If we think this is only profitable in Membyster, we should ignore Membuster, because it no longer reflects the workloads we would like to optimize. 1620108 3 koivisto 2020-02-18 03:22:17 -0800 I'm not a fan of creating a difference between iOS and Mac here either. 1620242 4 ysuzuki 2020-02-18 11:21:13 -0800 (In reply to Antti Koivisto from comment #3) > I'm not a fan of creating a difference between iOS and Mac here either. I'm not a fan of this too. But if this offers large benefit, I think we should not miss a chance of optimization because of clean / consistent code / policy. The A/B test is saying 4% improvement in Membuster, this is super large. I think, this is because so many files in Membuster is <= 16KB. At this point, I think there are two choices, 1. We should do that. 4% is super large. 2. We should not do that. But in that case, I think we should ignore Membuster from now. Ignoring 4% improvement means we are almost saying "this benchmark is useless". 1620260 5 koivisto 2020-02-18 12:00:47 -0800 Considering there are known costs in terms of system resources which Membuster doesn't capture, I think some real world benefits would need to be proven. 1620298 6 ysuzuki 2020-02-18 13:33:48 -0800 (In reply to Antti Koivisto from comment #5) > Considering there are known costs in terms of system resources which > Membuster doesn't capture, I think some real world benefits would need to be > proven. One of the benefit of using mmap-ed memory backed by file is that, applications can tell OS about the hint that this memory can be purgeable if OS wants since the content exists on the disk. This offers two benefits, 1. OS can purge it if it wants. 2. OS can avoid swapping / compressing this memory to get saved memory unnecessarily. Instead, it can purge it and recover it if it is accessed. Currently, non-dirty mmap-ed memory region is not accounted as memory, I think this makes sense since OS can purge it if it want. I have a question. If this does not look a real world benefit, why do we have mmap-ed file path? Using mmap-ed file if larger than 16KB sounds strange if it does not offer any benefit. If it is true, then, we should just remove this blob mechanism entirely and always use inline body. If it offers benefit, the same benefit can be achieved for 4KB too, and it is measured in Membuster. 1620640 7 koivisto 2020-02-19 07:26:48 -0800 Hmm, I suppose there is no likely harm in dropping this back to the page size on Mac. It is sort of logical. The problems with running out of file descriptors were always on iOS. 1620836 8 ysuzuki 2020-02-19 13:29:46 -0800 (In reply to Antti Koivisto from comment #7) > Hmm, I suppose there is no likely harm in dropping this back to the page > size on Mac. It is sort of logical. The problems with running out of file > descriptors were always on iOS. That sounds really nice! The final result of A/B test (with 10 runs for each) shows 2.5% progression w/ 98% significance. This means that I think this is really solid improvement in macOS. I've discussed with Phil, and we agree that this reduces memory pressure because of named-pages (instead of anonymous pages). 1620899 9 ysuzuki 2020-02-19 15:19:35 -0800 I discussed with Geoff about this. We agree that using named-pages is improvement in terms of memory because OS can purge it and recover it cheaply. We agree that just decreasing the threshold for macOS is the safest option and improvement. For now, we should start using page-size, this is the safest. Geoff also suggests an interesting idea: we should explore good threshold for iOS too. Currently, we are always using mmap-file-path if the size gets larger than 16KB. This means we could waste 16KB - 1 byte in the worst case (50% wasting). This also suggests that we should try 8KB (50% of 16KB) threshold for iOS. I think it is worth looking into it after fixing this issue. 1620901 10 391210 ysuzuki 2020-02-19 15:23:06 -0800 Created attachment 391210 Patch 1620911 11 webkit-bug-importer 2020-02-19 15:34:50 -0800 <rdar://problem/59608926> 1620973 12 391210 ysuzuki 2020-02-19 17:26:45 -0800 Comment on attachment 391210 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391210&action=review > Source/WebKit/ChangeLog:21 > + This patch reduces the threshold from 16KB to page size (4KB in macOS, 16KB in iOS). This offers 2.56% progression with 98% probability in Membuster. I note that this 4KB threshold is the default one before 2015. 1620976 13 391210 achristensen 2020-02-19 17:29:35 -0800 Comment on attachment 391210 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391210&action=review > Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp:297 > auto headerSizes = recordCount * 4096; Let's consider changing this to pageSize, too. 1620977 14 achristensen 2020-02-19 17:32:34 -0800 Actually, maybe not. 1620993 15 ysuzuki 2020-02-19 18:13:00 -0800 Committed r257025: <https://trac.webkit.org/changeset/257025> 1621225 16 Lawrence.j 2020-02-20 08:14:05 -0800 Reverted r257025 for reason: This commit broke a test on Mac wk2. Committed r257061: <https://trac.webkit.org/changeset/257061> 1621227 17 Lawrence.j 2020-02-20 08:16:37 -0800 This commit caused a regression with the http/tests/inspector/network/resource-sizes-disk-cache.html test. https://bugs.webkit.org/show_bug.cgi?id=208004 1621357 18 ysuzuki 2020-02-20 12:46:28 -0800 (In reply to Jason Lawrence from comment #17) > This commit caused a regression with the > http/tests/inspector/network/resource-sizes-disk-cache.html test. > https://bugs.webkit.org/show_bug.cgi?id=208004 The test looks broken, it is assuming that space100.png is always in non-mmap-file path, and this is wrong. I'll reland this with test update. 1621418 19 ysuzuki 2020-02-20 14:23:41 -0800 Committed r257089: <https://trac.webkit.org/changeset/257089> 391210 2020-02-19 15:23:06 -0800 2020-02-20 14:21:00 -0800 Patch bug-207882-20200219152305.patch text/plain 4352 ysuzuki U3VidmVyc2lvbiBSZXZpc2lvbjogMjU2OTQxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDQzMjc5NTY3MjdkNmRlYzhm ZjljN2Q4OTY4Y2VlYmEwMTA5ZWYzYjEuLjY4YmU2ODMwYjJiMzEwMTkzMWE5YTM1MWVlZDdhM2Rh Nzk3NDQ5NTIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMzAgQEAKKzIwMjAtMDItMTkgIFl1c3VrZSBT dXp1a2kgIDx5c3V6dWtpQGFwcGxlLmNvbT4KKworICAgICAgICBOZXR3b3JrQ2FjaGUgc2hvdWxk IHVzZSA0S0IgdGhyZXNob2xkIGZvciBtbWFwLWVkIGZpbGVzIGluc3RlYWQgb2YgMTZLQgorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjA3ODgyCisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgV2UgZm91bmQgdGhh dCBhIGxvdCBvZiBWZWN0b3JzIGluIE1lbWJ1c3RlciBpcyBob2xkaW5nIHJlc291cmNlIGNvbnRl bnQuIFRoaXMgaXMgYmVjYXVzZSB3ZSBoYXZlIDE2S0IgdGhyZXNob2xkIGZvciBtbWFwLWVkIGZp bGVzLgorICAgICAgICBJZiBhIGZpbGUgaXMgc21hbGxlciB0aGFuIDE2S0IsIGl0IGlzIGNvcGll ZCB0byBWZWN0b3IgaW5zdGVhZC4gQnV0IHRoaXMgaXMgY29zdGx5IGluIHRlcm1zIG9mIG1lbW9y eS4gSWYgd2UgdXNlIG1tYXAtZWQgZmlsZXMsCisgICAgICAgIGl0IGJlY29tZXMgbmFtZWQtcGFn ZXMgaW5zdGVhZCBvZiBhbm9ueW1vdXMtcGFnZXMuIEZpbGUtYmFja2VkIG5vbi1kaXJ0eSBuYW1l ZC1wYWdlcyBoYXZlIGEgbG90IG9mIGJlbmVmaXQuCisKKyAgICAgICAgMS4gVGhlIGFwcGxpY2F0 aW9uIGlzIG9mZmVyaW5nIGEgaGludCB0aGF0IHBhZ2VzIGFyZSBmaWxlLWJhY2tlZC4gVGhpcyBt ZWFucyB0aGF0IE9TIGNhbiBwdXJnZSB0aGVtIGF0IGFueSB0aW1lIHNpbmNlIHRoZSBjb250ZW50 IGNhbiBiZSByZWNvdmVyZWQKKyAgICAgICAgICAgZnJvbSB0aGUgZGlzay4gVGhpcyBpcyBjaGVh cGVyIHRoYW4gc3dhcHBpbmcgLyBjb21wcmVzc2luZyBhbm9ueW1vdXMgcGFnZXMgc2luY2UganVz dCBkaXNjYXJkaW5nIHdvcmtzLgorICAgICAgICAyLiBUaGUgYXBwbGljYXRpb24gaXMgb2ZmZXJp bmcgYSBoaW50IHRoYXQgcGFnZXMgaGF2ZSBzcGF0aWFsIGxvY2FsaXR5LiBQdXJnaW5nIHBhZ2Vz IGluIG9uZSBuYW1lZC1wYWdlcyByZWdpb24gaXMgYmV0dGVyIGNvbXBhcmVkIHRvIHB1cmdpbmcK KyAgICAgICAgICAgdGhlIHNhbWUgIyBvZiBhbm9ueW1vdXMgcGFnZXMgcmFuZG9tbHkuIEFub255 bW91cyBwYWdlcyBhcmUgc3BsaXQgYnkgbWFsbG9jIGltcGxlbWVudGF0aW9uIGFuZCBhY2Nlc3Mg cGF0dGVybiBvZiBwYWdlcyBpbiBvbmUgVkEgaXMgcmFuZG9tLiBPbgorICAgICAgICAgICB0aGUg b3RoZXIgaGFuZCwgbmFtZWQtcGFnZXMgYXJlIGFjY2Vzc2VkIHRvZ2V0aGVyIGJlY2F1c2UgaXQg aXMgZmlsZSwgYW5kIGZpbGUgdHlwaWNhbGx5IGhhcyBzZXF1ZW50aWFsIGxvY2FsaXR5LiBBbmQg cmVjb3Zlcnkgb2YgbmFtZWQgcGFnZXMgYXJlCisgICAgICAgICAgIGFsc28gY2hlYXAgY29tcGFy ZWQgdG8gYW5vbnltb3VzIHBhZ2VzIHNpbmNlIE9TIGNhbiBwcmVmZXRjaCBwYWdlcyBvbmNlIGFj Y2VzcyBoYXBwZW5zIGJlY2F1c2Ugb2Ygc2VxdWVudGlhbCBsb2NhbGl0eSBvZiBmaWxlcy4gVGhp cyB0ZW5kZW5jeSBtYWtlcworICAgICAgICAgICBPUyBsaWtlIHB1cmdpbmcgbmFtZWQgcGFnZXMg aW5zdGVhZCBvZiBhbm9ueW1vdXMgcGFnZXMuIEluIFdlYktpdCB1c2UgY2FzZSwgdGhpcyB3b3Jr cyBwZXJmZWN0bHkuIENhY2hlZFJlc291cmNlIHR5cGljYWxseSBoYXMgZGVjb2RlZCBjb250ZW50 LiBTbworICAgICAgICAgICB0eXBpY2FsbHkgV2ViUHJvY2VzcyBkb2VzIG5vdCBhY2Nlc3MgU2hh cmVkQnVmZmVyIGFmdGVyIHRoZSBjb250ZW50IGlzIGRlY29kZWQuCisKKyAgICAgICAgVGhpcyBw YXRjaCByZWR1Y2VzIHRoZSB0aHJlc2hvbGQgZnJvbSAxNktCIHRvIHBhZ2Ugc2l6ZSAoNEtCIGlu IG1hY09TLCAxNktCIGluIGlPUykuIFRoaXMgb2ZmZXJzIDIuNTYlIHByb2dyZXNzaW9uIHdpdGgg OTglIHByb2JhYmlsaXR5IGluIE1lbWJ1c3Rlci4KKworICAgICAgICAqIE5ldHdvcmtQcm9jZXNz L2NhY2hlL05ldHdvcmtDYWNoZVN0b3JhZ2UuY3BwOgorICAgICAgICAoV2ViS2l0OjpOZXR3b3Jr Q2FjaGU6Om1heGltdW1JbmxpbmVCb2R5U2l6ZSk6CisgICAgICAgIChXZWJLaXQ6Ok5ldHdvcmtD YWNoZTo6ZXN0aW1hdGVSZWNvcmRzU2l6ZSk6CisgICAgICAgIChXZWJLaXQ6Ok5ldHdvcmtDYWNo ZTo6U3RvcmFnZTo6c2hvdWxkU3RvcmVCb2R5QXNCbG9iKToKKwogMjAyMC0wMi0xOSAgV2Vuc29u IEhzaWVoICA8d2Vuc29uX2hzaWVoQGFwcGxlLmNvbT4KIAogICAgICAgICBbaU9TXSBmYXN0L2Rv bS9mb2N1cy1zaGlmdC1jcmFzaC5odG1sIGFuZCBlZGl0aW5nL3NlbGVjdGlvbi9zZWxlY3Rpb24t Y2hhbmdlLWluLWRpc2Nvbm5lY3RlZC1mcmFtZS1jcmFzaC5odG1sIGNyYXNoIGFmdGVyIHIyNTY4 NjQKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvTmV0d29ya1Byb2Nlc3MvY2FjaGUvTmV0d29y a0NhY2hlU3RvcmFnZS5jcHAgYi9Tb3VyY2UvV2ViS2l0L05ldHdvcmtQcm9jZXNzL2NhY2hlL05l dHdvcmtDYWNoZVN0b3JhZ2UuY3BwCmluZGV4IGI3OWJiMTMyYjY1M2ZlODBlYzMwYjc5MGQ0NmQy ODIxNWE1MGZlZDUuLmI1ZTM5YzgwNzRkYzE1MTIzZDM0NTVjYzM4YjM2ZDQ4NTE3NzU4OWEgMTAw NjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvTmV0d29ya1Byb2Nlc3MvY2FjaGUvTmV0d29ya0NhY2hl U3RvcmFnZS5jcHAKKysrIGIvU291cmNlL1dlYktpdC9OZXR3b3JrUHJvY2Vzcy9jYWNoZS9OZXR3 b3JrQ2FjaGVTdG9yYWdlLmNwcApAQCAtMzQsNiArMzQsNyBAQAogI2luY2x1ZGUgPG11dGV4Pgog I2luY2x1ZGUgPHd0Zi9Db25kaXRpb24uaD4KICNpbmNsdWRlIDx3dGYvTG9jay5oPgorI2luY2x1 ZGUgPHd0Zi9QYWdlQmxvY2suaD4KICNpbmNsdWRlIDx3dGYvUmFuZG9tTnVtYmVyLmg+CiAjaW5j bHVkZSA8d3RmL1J1bkxvb3AuaD4KICNpbmNsdWRlIDx3dGYvdGV4dC9DU3RyaW5nLmg+CkBAIC00 Nyw3ICs0OCwxMSBAQCBzdGF0aWMgY29uc3QgY2hhciB2ZXJzaW9uRGlyZWN0b3J5UHJlZml4W10g PSAiVmVyc2lvbiAiOwogc3RhdGljIGNvbnN0IGNoYXIgcmVjb3Jkc0RpcmVjdG9yeU5hbWVbXSA9 ICJSZWNvcmRzIjsKIHN0YXRpYyBjb25zdCBjaGFyIGJsb2JzRGlyZWN0b3J5TmFtZVtdID0gIkJs b2JzIjsKIHN0YXRpYyBjb25zdCBjaGFyIGJsb2JTdWZmaXhbXSA9ICItYmxvYiI7Ci1jb25zdGV4 cHIgc2l6ZV90IG1heGltdW1JbmxpbmVCb2R5U2l6ZSB7IDE2ICogMTAyNCB9OworCitzdGF0aWMg aW5saW5lIHNpemVfdCBtYXhpbXVtSW5saW5lQm9keVNpemUoKQoreworICAgIHJldHVybiBXVEY6 OnBhZ2VTaXplKCk7Cit9CiAKIHN0YXRpYyBkb3VibGUgY29tcHV0ZVJlY29yZFdvcnRoKEZpbGVU aW1lcyk7CiAKQEAgLTI5MCw3ICsyOTUsNyBAQCBzdGF0aWMgc2l6ZV90IGVzdGltYXRlUmVjb3Jk c1NpemUodW5zaWduZWQgcmVjb3JkQ291bnQsIHVuc2lnbmVkIGJsb2JDb3VudCkKIHsKICAgICBh dXRvIGlubGluZUJvZHlDb3VudCA9IHJlY29yZENvdW50IC0gc3RkOjptaW4oYmxvYkNvdW50LCBy ZWNvcmRDb3VudCk7CiAgICAgYXV0byBoZWFkZXJTaXplcyA9IHJlY29yZENvdW50ICogNDA5NjsK LSAgICBhdXRvIGlubGluZUJvZHlTaXplcyA9IChtYXhpbXVtSW5saW5lQm9keVNpemUgLyAyKSAq IGlubGluZUJvZHlDb3VudDsKKyAgICBhdXRvIGlubGluZUJvZHlTaXplcyA9IChtYXhpbXVtSW5s aW5lQm9keVNpemUoKSAvIDIpICogaW5saW5lQm9keUNvdW50OwogICAgIHJldHVybiBoZWFkZXJT aXplcyArIGlubGluZUJvZHlTaXplczsKIH0KIApAQCAtNzk4LDcgKzgwMyw3IEBAIHZvaWQgU3Rv cmFnZTo6ZGlzcGF0Y2hQZW5kaW5nV3JpdGVPcGVyYXRpb25zKCkKIAogYm9vbCBTdG9yYWdlOjpz aG91bGRTdG9yZUJvZHlBc0Jsb2IoY29uc3QgRGF0YSYgYm9keURhdGEpCiB7Ci0gICAgcmV0dXJu IGJvZHlEYXRhLnNpemUoKSA+IG1heGltdW1JbmxpbmVCb2R5U2l6ZTsKKyAgICByZXR1cm4gYm9k eURhdGEuc2l6ZSgpID4gbWF4aW11bUlubGluZUJvZHlTaXplKCk7CiB9CiAKIHZvaWQgU3RvcmFn ZTo6ZGlzcGF0Y2hXcml0ZU9wZXJhdGlvbihzdGQ6OnVuaXF1ZV9wdHI8V3JpdGVPcGVyYXRpb24+ IHdyaXRlT3BlcmF0aW9uUHRyKQo=