Bug 204286 – [bmalloc] Some chunks have unused region in the tail of its memory block.

• Component: bmalloc
• Status: RESOLVED FIXED

[Basuke Suzuki]

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.

[Basuke Suzuki]

Created attachment 383787 [details]
PATCH

[Yusuke Suzuki]

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.

[WebKit Commit Bot]

Comment on attachment 383787 [details]
PATCH

Clearing flags on attachment: 383787

Committed r252621: <https://trac.webkit.org/changeset/252621>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57308973>

[Geoffrey Garen]

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.

[Yusuke Suzuki]

(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)

[Yusuke Suzuki]

(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

[Yusuke Suzuki]

(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

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

(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).

[Basuke Suzuki]

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.

[Basuke Suzuki]

(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()`.

[Yusuke Suzuki]

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.

[Basuke Suzuki]

(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.