204286 2019-11-17 16:09:47 -0800 [bmalloc] Some chunks have unused region in the tail of its memory block. 2019-12-03 12:54:12 -0800 1 1 1 Unclassified WebKit bmalloc WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=204773 InRadar P2 Normal --- 1 basuke basuke basuke commit-queue ggaren webkit-bug-importer ysuzuki oldest_to_newest 1590957 0 basuke 2019-11-17 16:09:47 -0800 Depending on its pageSize, chunk block has an unused memory block at the tail of its allocated block. All chunks are allocated in chunkSize and initialized by pageSize (pageClass). At this moment, some pageSize doesn't use some amount of memory at the end. For instance, pageClass 5 has 16k unused memory and pageClass 12 has 36k of it. These areas are kept committed but unused until it's pushed to largeFree. 1591223 1 383787 basuke 2019-11-18 13:45:49 -0800 Created attachment 383787 PATCH 1591404 2 383787 ysuzuki 2019-11-18 18:44:38 -0800 Comment on attachment 383787 PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=383787&action=review r=me > Source/bmalloc/ChangeLog:12 > + a chunk can hold 42 pages and its size is 24k * 42 = 1008k which is smaller than chunkSize = 1024k. I was a bit considering about huge-page things. But it seems that chunkSize is 1MB (not 2MB), so we are not using huge-page feature so aggressively. 1591411 3 383787 commit-queue 2019-11-18 19:29:18 -0800 Comment on attachment 383787 PATCH Clearing flags on attachment: 383787 Committed r252621: <https://trac.webkit.org/changeset/252621> 1591412 4 commit-queue 2019-11-18 19:29:20 -0800 All reviewed patches have been landed. Closing bug. 1591413 5 webkit-bug-importer 2019-11-18 19:30:21 -0800 <rdar://problem/57308973> 1591429 6 ggaren 2019-11-18 21:36:02 -0800 The rationale for this patch is wrong. Untouched virtual pages have no physical size. Also, choosing to fragment the OS VM allocator like this usually produces bad performance. 1591450 7 ysuzuki 2019-11-18 23:27:53 -0800 (In reply to Geoffrey Garen from comment #6) > The rationale for this patch is wrong. Untouched virtual pages have no > physical size. Sounds correct. If we are touching Chunk memory or calling explicit madvise to populate backing pages, pages are populated, but maybe we don't. > Also, choosing to fragment the OS VM allocator like this > usually produces bad performance. But are we already doing the exact same thing in bmalloc::Scavenger? (decommitting some part of pages) 1591451 8 ysuzuki 2019-11-18 23:28:59 -0800 (In reply to Yusuke Suzuki from comment #7) > But are we already doing the exact same thing in bmalloc::Scavenger? > (decommitting some part of pages) /some part of pages/some unused pages in Chunk/s 1591454 9 ysuzuki 2019-11-18 23:45:34 -0800 (In reply to Yusuke Suzuki from comment #7) > (In reply to Geoffrey Garen from comment #6) > > The rationale for this patch is wrong. Untouched virtual pages have no > > physical size. > > Sounds correct. If we are touching Chunk memory or calling explicit madvise > to populate backing pages, pages are populated, but maybe we don't. Hmm, when we are calling splitAndAllocate (allocate LargeRange, which will be used for Chunk's memory), are we populating pages? https://github.com/WebKit/webkit/blob/master/Source/bmalloc/bmalloc/Heap.cpp#L580 1591456 10 ysuzuki 2019-11-19 00:09:10 -0800 (In reply to Yusuke Suzuki from comment #9) > (In reply to Yusuke Suzuki from comment #7) > > (In reply to Geoffrey Garen from comment #6) > > > The rationale for this patch is wrong. Untouched virtual pages have no > > > physical size. > > > > Sounds correct. If we are touching Chunk memory or calling explicit madvise > > to populate backing pages, pages are populated, but maybe we don't. > > Hmm, when we are calling splitAndAllocate (allocate LargeRange, which will > be used for Chunk's memory), are we populating pages? > > https://github.com/WebKit/webkit/blob/master/Source/bmalloc/bmalloc/Heap. > cpp#L580 Unlike the name of this function, I think this is just clearing reusable-bit in VM. 1591461 11 ysuzuki 2019-11-19 00:24:43 -0800 (In reply to Yusuke Suzuki from comment #10) > (In reply to Yusuke Suzuki from comment #9) > > (In reply to Yusuke Suzuki from comment #7) > > > (In reply to Geoffrey Garen from comment #6) > > > > The rationale for this patch is wrong. Untouched virtual pages have no > > > > physical size. > > > > > > Sounds correct. If we are touching Chunk memory or calling explicit madvise > > > to populate backing pages, pages are populated, but maybe we don't. > > > > Hmm, when we are calling splitAndAllocate (allocate LargeRange, which will > > be used for Chunk's memory), are we populating pages? > > > > https://github.com/WebKit/webkit/blob/master/Source/bmalloc/bmalloc/Heap. > > cpp#L580 > > Unlike the name of this function, I think this is just clearing reusable-bit > in VM. I'll discuss about it tomorrow. If this vmAllocatePhysicalPagesSloppy is allocating physical pages, we were wasting pages. If it isn't this patch is not effective (maybe, this patch does not change anything). 1591624 12 basuke 2019-11-19 11:55:32 -0800 vmDeallocatePhysicalPagesSloppy() just tells system we don't need this area. No frangemntation happens. mmapped region kept in chunk size and once the life cycle of chunk finishes, it will be stored in largeFree and reused if they have chance or decommited by scavenger. Before this path, every unused page is decommited in next scavenge, but this region is left behind the scavenge. This patch makes decommit happen at the beginning. 1591626 13 basuke 2019-11-19 11:58:27 -0800 (In reply to Yusuke Suzuki from comment #10) > Unlike the name of this function, I think this is just clearing reusable-bit > in VM. Yes, the name of this function is mis-directional. It may be named `vmDecommitPhysicalPage()`. 1591748 14 ysuzuki 2019-11-19 16:18:46 -0800 Talked with Geoff. Geoff pointed very interesting thing, like, Once Chunk is allocated for small pages, and fully used, and returned to LargeRange allocator. Then pages are marked as REUSABLE. But it is possible that we allocate this region and mark them REUSE until OS collects backing pages. So, then, OS cannot collect them, and we start actually wasting pages. 1592028 15 basuke 2019-11-20 09:37:19 -0800 (In reply to Yusuke Suzuki from comment #14) > Once Chunk is allocated for small pages, and fully used, and returned to > LargeRange allocator. Then pages are marked as REUSABLE. > But it is possible that we allocate this region and mark them REUSE until OS > collects backing pages. So, then, OS cannot collect them, and we start > actually wasting pages. That's right. allocateLarge can return not only the region allocated at the moment, but it can be came from large cache. There's a chance returned region has physical memory. At that time, this patch is effective. 383787 2019-11-18 13:45:49 -0800 2019-11-18 19:29:18 -0800 PATCH bug-204286.diff text/plain 4463 basuke ZGlmZiAtLWdpdCBhL1NvdXJjZS9ibWFsbG9jL0NoYW5nZUxvZyBiL1NvdXJjZS9ibWFsbG9jL0No YW5nZUxvZwppbmRleCA1OGQwYjM1NjQ4MC4uODA0MGM3MzFjNDggMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9ibWFsbG9jL0NoYW5nZUxvZworKysgYi9Tb3VyY2UvYm1hbGxvYy9DaGFuZ2VMb2cKQEAgLTEs MyArMSw1MSBAQAorMjAxOS0xMS0xOCAgQmFzdWtlIFN1enVraSAgPGJhc3VrZS5zdXp1a2lAc29u eS5jb20+CisKKyAgICAgICAgW2JtYWxsb2NdIFNvbWUgY2h1bmtzIGhhdmUgdW51c2VkIHJlZ2lv biBpbiB0aGUgdGFpbCBvZiBpdHMgbWVtb3J5IGJsb2NrLgorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjA0Mjg2CisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgV2hlbiBjaHVuayBpcyBpbml0aWFsaXplZCwgc29t ZSBhbW91bnQgb2YgbWVtb3J5IGFyZSBub3QgdXNlZCBhbmQgYmUga2VwdCB1bnRvdWNoZWQgdW50 aWwgaXRzIGVuZC4KKyAgICAgICAgVGhpcyBwYXRjaCB0cmllcyB0byBkZWNvbW1pdCB0aG9zZSBy ZWdpb24gYXQgdGhlIGVuZCBvZiBpbml0aWFsaXphdGlvbi4KKworICAgICAgICBGb3IgaW5zdGFu Y2UsIHRoaW5rIGFib3V0IHRoZSBjYXNlIHRoYXQgdGhlIHBhZ2VDbGFzcyBpcyA1LiBUaGVuIHBh Z2VTaXplIGlzIDI0ay4gV2l0aCB0aGlzIHBhZ2VTaXplLAorICAgICAgICBhIGNodW5rIGNhbiBo b2xkIDQyIHBhZ2VzIGFuZCBpdHMgc2l6ZSBpcyAyNGsgKiA0MiA9IDEwMDhrIHdoaWNoIGlzIHNt YWxsZXIgdGhhbiBjaHVua1NpemUgPSAxMDI0ay4KKyAgICAgICAgSGVyZSBpcyB0aGUgY29tcGxl dGUgcmVzdWx0OgorCisgICAgICAgICAgICAgICAgcGFnZSAgICBwYWdlICAgIHBhZ2UKKyAgICAg ICAgICAgICAgICBjbGFzcyAgIHNpemUgICAgY291bnQgICByZW1haW5pbmdzCisgICAgICAgICAg ICAgICAgLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQorICAgICAgICAgICAgICAg IDAgICAgICAgNGtCICAgICAyNTYgICAgIDAKKyAgICAgICAgICAgICAgICAxICAgICAgIDhrQiAg ICAgMTI4ICAgICAwCisgICAgICAgICAgICAgICAgMiAgICAgICAxMmtCICAgIDg1ICAgICAgNGtC CisgICAgICAgICAgICAgICAgMyAgICAgICAxNmtCICAgIDY0ICAgICAgMAorICAgICAgICAgICAg ICAgIDQgICAgICAgMjBrQiAgICA1MSAgICAgIDRrQgorICAgICAgICAgICAgICAgIDUgICAgICAg MjRrQiAgICA0MiAgICAgIDE2a0IKKyAgICAgICAgICAgICAgICA2ICAgICAgIDI4a0IgICAgMzYg ICAgICAxNmtCCisgICAgICAgICAgICAgICAgNyAgICAgICAzMmtCICAgIDMyICAgICAgMAorICAg ICAgICAgICAgICAgIDggICAgICAgMzZrQiAgICAyOCAgICAgIDE2a0IKKyAgICAgICAgICAgICAg ICA5ICAgICAgIDQwa0IgICAgMjUgICAgICAyNGtCCisgICAgICAgICAgICAgICAgMTAgICAgICA0 NGtCICAgIDIzICAgICAgMTJrQgorICAgICAgICAgICAgICAgIDExICAgICAgNDhrQiAgICAyMSAg ICAgIDE2a0IKKyAgICAgICAgICAgICAgICAxMiAgICAgIDUya0IgICAgMTkgICAgICAzNmtCCisg ICAgICAgICAgICAgICAgMTMgICAgICA1NmtCICAgIDE4ICAgICAgMTZrQgorICAgICAgICAgICAg ICAgIDE0ICAgICAgNjBrQiAgICAxNyAgICAgIDRrQgorICAgICAgICAgICAgICAgIDE1ICAgICAg NjRrQiAgICAxNiAgICAgIDAKKworICAgICAgICBUZXN0ZWQgb24gTWFjIHRlc3RtZW0gYW5kIHJl c3VsdCBpcyBhbG1vc3Qgc2FtZSBvciBpbiBlcnJvciBtYXJnaW4uCisKKyAgICAgICAgICAgICAg ICBCZWZvcmU6ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgQWZ0ZXI6CisgICAgICAg ICAgICAgICAgZW5kIHNjb3JlOiAgICAgICAgICAgOC41NDI1IE1CICAgICAgICAgIGVuZCBzY29y ZTogICAgICAgICAgIDguNTEyNyBNQgorICAgICAgICAgICAgICAgIHBlYWsgc2NvcmU6ICAgICAg ICAgIDguNzk5NyBNQiAgICAgICAgICBwZWFrIHNjb3JlOiAgICAgICAgICA4Ljc4ODQgTUIKKyAg ICAgICAgICAgICAgICB0b3RhbCBtZW1vcnkgc2NvcmU6ICA4LjY3MDIgTUIgICAgICAgICAgdG90 YWwgbWVtb3J5IHNjb3JlOiAgOC42NDk1IE1CCisgICAgICAgICAgICAgICAgdGltZSBzY29yZTog ICAgICAgICAgNjY4LjE5IG1zICAgICAgICAgIHRpbWUgc2NvcmU6ICAgICAgICAgIDY2Ni4yNyBt cworCisgICAgICAgICogYm1hbGxvYy9DaHVuay5oOgorICAgICAgICAoYm1hbGxvYzo6Q2h1bms6 Om1ldGFkYXRhU2l6ZSk6CisgICAgICAgIChibWFsbG9jOjpmb3JFYWNoUGFnZSk6CisgICAgICAg ICogYm1hbGxvYy9IZWFwLmNwcDoKKyAgICAgICAgKGJtYWxsb2M6OkhlYXA6OmFsbG9jYXRlU21h bGxDaHVuayk6CisKIDIwMTktMTEtMTUgIEJhc3VrZSBTdXp1a2kgIDxiYXN1a2Uuc3V6dWtpQHNv bnkuY29tPgogCiAgICAgICAgIFtibWFsbG9jXSBUaGUgdHJhY2tpbmcgb2YgZnJlZWFibGVNZW1v cnkgb2YgSGVhcCBkb2Vzbid0IGNvdW50IENodW5rJ3MgbWV0YWRhdGEgc2l6ZS4KZGlmZiAtLWdp dCBhL1NvdXJjZS9ibWFsbG9jL2JtYWxsb2MvQ2h1bmsuaCBiL1NvdXJjZS9ibWFsbG9jL2JtYWxs b2MvQ2h1bmsuaAppbmRleCAzMWRiNmYyZDdiZi4uMjFmYmI4ZmU3OTggMTAwNjQ0Ci0tLSBhL1Nv dXJjZS9ibWFsbG9jL2JtYWxsb2MvQ2h1bmsuaAorKysgYi9Tb3VyY2UvYm1hbGxvYy9ibWFsbG9j L0NodW5rLmgKQEAgLTM4LDYgKzM4LDcgQEAgbmFtZXNwYWNlIGJtYWxsb2MgewogY2xhc3MgQ2h1 bmsgOiBwdWJsaWMgTGlzdE5vZGU8Q2h1bms+IHsKIHB1YmxpYzoKICAgICBzdGF0aWMgQ2h1bmsq IGdldCh2b2lkKik7CisgICAgc3RhdGljIHNpemVfdCBtZXRhZGF0YVNpemUoc2l6ZV90IHBhZ2VT aXplKTsKIAogICAgIENodW5rKHNpemVfdCBwYWdlU2l6ZSk7CiAgICAgCkBAIC03MywxMyArNzQs MTYgQEAgc3RydWN0IENodW5rSGFzaCB7CiAgICAgfQogfTsKIAotdGVtcGxhdGU8dHlwZW5hbWUg RnVuY3Rpb24+IHZvaWQgZm9yRWFjaFBhZ2UoQ2h1bmsqIGNodW5rLCBzaXplX3QgcGFnZVNpemUs IEZ1bmN0aW9uIGZ1bmN0aW9uKQoraW5saW5lIHNpemVfdCBDaHVuazo6bWV0YWRhdGFTaXplKHNp emVfdCBwYWdlU2l6ZSkKIHsKICAgICAvLyBXZSBhbGlnbiB0byBhdCBsZWFzdCB0aGUgcGFnZSBz aXplIHNvIHdlIGNhbiBzZXJ2aWNlIGFsaWduZWQgYWxsb2NhdGlvbnMKICAgICAvLyBhdCBlcXVh bCBhbmQgc21hbGxlciBwb3dlcnMgb2YgdHdvLCBhbmQgYWxzbyBzbyB3ZSBjYW4gdm1EZWFsbG9j YXRlUGh5c2ljYWxQYWdlcygpLgotICAgIHNpemVfdCBtZXRhZGF0YVNpemUgPSByb3VuZFVwVG9N dWx0aXBsZU9mTm9uUG93ZXJPZlR3byhwYWdlU2l6ZSwgc2l6ZW9mKENodW5rKSk7CisgICAgcmV0 dXJuIHJvdW5kVXBUb011bHRpcGxlT2ZOb25Qb3dlck9mVHdvKHBhZ2VTaXplLCBzaXplb2YoQ2h1 bmspKTsKK30KIAotICAgIE9iamVjdCBiZWdpbihjaHVuaywgbWV0YWRhdGFTaXplKTsKK3RlbXBs YXRlPHR5cGVuYW1lIEZ1bmN0aW9uPiB2b2lkIGZvckVhY2hQYWdlKENodW5rKiBjaHVuaywgc2l6 ZV90IHBhZ2VTaXplLCBGdW5jdGlvbiBmdW5jdGlvbikKK3sKKyAgICBPYmplY3QgYmVnaW4oY2h1 bmssIENodW5rOjptZXRhZGF0YVNpemUocGFnZVNpemUpKTsKICAgICBPYmplY3QgZW5kKGNodW5r LCBjaHVua1NpemUpOwogCiAgICAgZm9yIChhdXRvIGl0ID0gYmVnaW47IGl0ICsgcGFnZVNpemUg PD0gZW5kOyBpdCA9IGl0ICsgcGFnZVNpemUpCmRpZmYgLS1naXQgYS9Tb3VyY2UvYm1hbGxvYy9i bWFsbG9jL0hlYXAuY3BwIGIvU291cmNlL2JtYWxsb2MvYm1hbGxvYy9IZWFwLmNwcAppbmRleCAz MjUzYjUzYTMyZi4uYjgxMDMzZmZlMTIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9ibWFsbG9jL2JtYWxs b2MvSGVhcC5jcHAKKysrIGIvU291cmNlL2JtYWxsb2MvYm1hbGxvYy9IZWFwLmNwcApAQCAtMjg2 LDcgKzI4NiwxMSBAQCB2b2lkIEhlYXA6OmFsbG9jYXRlU21hbGxDaHVuayhzdGQ6OnVuaXF1ZV9s b2NrPE11dGV4PiYgbG9jaywgc2l6ZV90IHBhZ2VDbGFzcywgRgogICAgICAgICB9KTsKIAogICAg ICAgICBtX2ZyZWVhYmxlTWVtb3J5ICs9IGFjY291bnRlZEluRnJlZWFibGU7Ci0gICAgICAgIAor CisgICAgICAgIGF1dG8gZGVjb21taXRTaXplID0gY2h1bmtTaXplIC0gQ2h1bms6Om1ldGFkYXRh U2l6ZShwYWdlU2l6ZSkgLSBhY2NvdW50ZWRJbkZyZWVhYmxlOworICAgICAgICBpZiAoZGVjb21t aXRTaXplID4gMCkKKyAgICAgICAgICAgIHZtRGVhbGxvY2F0ZVBoeXNpY2FsUGFnZXNTbG9wcHko Q2h1bms6OmdldChjaHVuayktPmFkZHJlc3MoY2h1bmtTaXplIC0gZGVjb21taXRTaXplKSwgZGVj b21taXRTaXplKTsKKwogICAgICAgICBtX3NjYXZlbmdlci0+c2NoZWR1bGUoMCk7CiAKICAgICAg ICAgcmV0dXJuIGNodW5rOwo=