Bug 204286 - [bmalloc] Some chunks have unused region in the tail of its memory block.
Summary: [bmalloc] Some chunks have unused region in the tail of its memory block.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: bmalloc (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Basuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-17 16:09 PST by Basuke Suzuki
Modified: 2019-12-03 12:54 PST (History)
5 users (show)

See Also:


Attachments
PATCH (4.36 KB, patch)
2019-11-18 13:45 PST, Basuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Basuke Suzuki 2019-11-17 16:09:47 PST
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.
Comment 1 Basuke Suzuki 2019-11-18 13:45:49 PST
Created attachment 383787 [details]
PATCH
Comment 2 Yusuke Suzuki 2019-11-18 18:44:38 PST
Comment on attachment 383787 [details]
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.
Comment 3 WebKit Commit Bot 2019-11-18 19:29:18 PST
Comment on attachment 383787 [details]
PATCH

Clearing flags on attachment: 383787

Committed r252621: <https://trac.webkit.org/changeset/252621>
Comment 4 WebKit Commit Bot 2019-11-18 19:29:20 PST
All reviewed patches have been landed.  Closing bug.
Comment 5 Radar WebKit Bug Importer 2019-11-18 19:30:21 PST
<rdar://problem/57308973>
Comment 6 Geoffrey Garen 2019-11-18 21:36:02 PST
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.
Comment 7 Yusuke Suzuki 2019-11-18 23:27:53 PST
(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)
Comment 8 Yusuke Suzuki 2019-11-18 23:28:59 PST
(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
Comment 9 Yusuke Suzuki 2019-11-18 23:45:34 PST
(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
Comment 10 Yusuke Suzuki 2019-11-19 00:09:10 PST
(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.
Comment 11 Yusuke Suzuki 2019-11-19 00:24:43 PST
(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).
Comment 12 Basuke Suzuki 2019-11-19 11:55:32 PST
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.
Comment 13 Basuke Suzuki 2019-11-19 11:58:27 PST
(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()`.
Comment 14 Yusuke Suzuki 2019-11-19 16:18:46 PST
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.
Comment 15 Basuke Suzuki 2019-11-20 09:37:19 PST
(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.