Bug 227957 - [BMalloc] Lazily allocate physical pages
Summary: [BMalloc] Lazily allocate physical pages
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: bmalloc (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-14 10:09 PDT by Michael Saboff
Modified: 2021-07-14 14:18 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.30 KB, patch)
2021-07-14 10:21 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>