Bug 204135

Summary: [bmalloc] The tracking of freeableMemory of Heap doesn't count Chunk's metadata size.
Product: WebKit Reporter: Basuke Suzuki <basuke>
Component: bmallocAssignee: Basuke Suzuki <basuke>
Status: RESOLVED FIXED    
Severity: Normal CC: basuke, commit-queue, ggaren, msaboff, saam, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
PATCH
none
PATCH none

Basuke Suzuki
Reported 2019-11-12 17:11:18 PST
We were observing that freeableMemory keeped growing. Then we found that the chunk metadataSize was not counted.
Attachments
PATCH (4.75 KB, patch)
2019-11-12 17:20 PST, Basuke Suzuki 		no flags
DetailsFormatted DiffDiff
PATCH (1.71 KB, patch)
2019-11-15 17:28 PST, Basuke Suzuki 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Basuke Suzuki
Comment 1 2019-11-12 17:20:18 PST
Created attachment 383411 [details] PATCH This is first part of the fix of the bug. I couldn't figure out that yet, but at least this must be right fix.
Basuke Suzuki
Comment 2 2019-11-12 17:22:10 PST
We did an internal research to log out freeable memory and footprint on each scavenge. This issue was found on that process.
Yusuke Suzuki
Comment 3 2019-11-15 12:06:43 PST
Comment on attachment 383411 [details] PATCH I think freeable memory is tracking the size of memory usable for allocation. So I don't think we need to add/sub Chunk's metadata size. Is my understanding correct?
Basuke Suzuki
Comment 4 2019-11-15 14:29:12 PST
https://github.com/WebKit/webkit/blob/5f3db1e7192810f53c4668f01ecb4e4fdbb3dc18/Source/bmalloc/bmalloc/Heap.cpp#L141-L157 This is the former implementation of feeableMemory. So you are right. Then here is the issue: https://github.com/WebKit/webkit/blob/master/Source/bmalloc/bmalloc/Heap.cpp#L286 When allocating a chunk in allocateSmallChunk(), chunkSize is added to freeableMemory. It should add only sizes of free pages.
Yusuke Suzuki
Comment 5 2019-11-15 14:30:01 PST
(In reply to Basuke Suzuki from comment #4) > https://github.com/WebKit/webkit/blob/ > 5f3db1e7192810f53c4668f01ecb4e4fdbb3dc18/Source/bmalloc/bmalloc/Heap. > cpp#L141-L157 > > This is the former implementation of feeableMemory. So you are right. Then > here is the issue: > > https://github.com/WebKit/webkit/blob/master/Source/bmalloc/bmalloc/Heap. > cpp#L286 > > When allocating a chunk in allocateSmallChunk(), chunkSize is added to > freeableMemory. It should add only sizes of free pages. Right. This looks wrong, nice.
Basuke Suzuki
Comment 6 2019-11-15 17:28:56 PST
Created attachment 383677 [details] PATCH
Yusuke Suzuki
Comment 7 2019-11-15 17:38:20 PST
Comment on attachment 383677 [details] PATCH r=me
WebKit Commit Bot
Comment 8 2019-11-15 18:34:39 PST
Comment on attachment 383677 [details] PATCH Clearing flags on attachment: 383677 Committed r252519: <https://trac.webkit.org/changeset/252519>
WebKit Commit Bot
Comment 9 2019-11-15 18:34:40 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 10 2019-11-15 18:35:16 PST
<rdar://problem/57247662>
Note You need to log in before you can comment on or make changes to this bug.