Bug 227957

Summary: [BMalloc] Lazily allocate physical pages
Product: WebKit Reporter: Michael Saboff <msaboff>
Component: bmallocAssignee: Michael Saboff <msaboff>
Status: RESOLVED FIXED    
Severity: Normal CC: ggaren, mark.lam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Michael Saboff 2021-07-14 10:09:15 PDT
Currently we call madvise(p, vmSize, MADV_FREE_REUSE) when we want to commit physical pages to a range of reserved memory.  On Darwin systems, the kernel will do this for us as we touch the pages.  Therefore we can eliminate this call, reducing our memory footprint when pages within a range are not used.
Comment 1 Radar WebKit Bug Importer 2021-07-14 10:09:49 PDT
<rdar://problem/80583372>
Comment 2 Michael Saboff 2021-07-14 10:21:59 PDT
Created attachment 433509 [details]
Patch

This patch provides a 3.5% (AS) to 4.6% (x86) reduction in memory in RAMification in my tests.

When testing JetStream2 and Speedometer2, it didn't regress on AS and is a 1.2% progression on both benchmarks for x86.
Comment 3 Mark Lam 2021-07-14 10:31:00 PDT
Comment on attachment 433509 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433509&action=review

r=me

> Source/bmalloc/bmalloc/VMAllocate.h:-218
> -    SYSCALL(madvise(p, vmSize, MADV_FREE_REUSE));

nit: Can you put the comment blurb from your ChangeLog in here about DARWIN not needing this madvise?  Otherwise, I can see someone not-in-the-know naively re-adding this back in the future with no visible immediate fallout to provide feedback that it should not be done.
Comment 4 Michael Saboff 2021-07-14 10:31:51 PDT
(In reply to Mark Lam from comment #3)
> Comment on attachment 433509 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433509&action=review
> 
> r=me
> 
> > Source/bmalloc/bmalloc/VMAllocate.h:-218
> > -    SYSCALL(madvise(p, vmSize, MADV_FREE_REUSE));
> 
> nit: Can you put the comment blurb from your ChangeLog in here about DARWIN
> not needing this madvise?  Otherwise, I can see someone not-in-the-know
> naively re-adding this back in the future with no visible immediate fallout
> to provide feedback that it should not be done.

Sure.
Comment 5 Geoffrey Garen 2021-07-14 11:49:08 PDT
Comment on attachment 433509 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433509&action=review

> Source/bmalloc/bmalloc/VMAllocate.h:220
> +    vmValidatePhysical(p, vmSize);

Seems unnecessary to lose this assertion on Darwin.
Comment 6 Michael Saboff 2021-07-14 13:55:56 PDT
Comment on attachment 433509 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433509&action=review

>> Source/bmalloc/bmalloc/VMAllocate.h:220
>> +    vmValidatePhysical(p, vmSize);
> 
> Seems unnecessary to lose this assertion on Darwin.

I'll add it back.
Comment 7 Michael Saboff 2021-07-14 14:18:26 PDT
Committed r279922 (239669@main): <https://commits.webkit.org/239669@main>