Bug 206044 – [bmalloc] Calculate LineMetadata for specific VM page size in compile time

• Status: RESOLVED
• Resolution: FIXED
• Component: bmalloc

[Basuke Suzuki]

LineMetadata is dependent only to vm page size. Precalculate them and store it in constexpr array makes benefits as it skips run time calculation and also page allocation.

[Basuke Suzuki]

Created attachment 387296 [details]
FIRST TRIAL

[Basuke Suzuki]

size of LineMetadata array is:
  sizeof(LineMetadata) * sizeClass(smallLineSize) * smallLineCount(VMPageSize)
  = 2 * maskSizeClass(256) * vmPageSize / 256
  = 62 * (vmPageSize / 256)

VMPageSize|size of LineMetadata array
----------+---------------
       4k | 62 * 16 = 992
      16k | 62 * 64 = 3968

[Basuke Suzuki]

I'm curious about VM page size for watchOS and tvOS.

[Basuke Suzuki]

Created attachment 387351 [details]
PATCH

[Yusuke Suzuki]

Comment on attachment 387351 [details]
PATCH

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

r=me so long as we have fallback mechanism which has dynamic calculation b/c some weird CPU has some weird page size (PowerPC has 64KB).
But I also want to have some comments from Geoff about page size assumption. If we can dynamically change the page size (maybe we don't especially in macOS / iOS), we should use dynamic calculation version.

> Source/bmalloc/bmalloc/BPlatform.h:270
> +#if BPLATFORM(iOS) || BPLATFORM(PLAYSTATION)

Typo, BPLATFORM(iOS) => BPLATFORM(IOS). BTW, I'm not sure whether all iOS uses 16KB pages.
@Geoff, do we use 16KB for all iOS (not including watchOS, tvOS and Catalyst)?

For macOS, I think 4KB is OK. Hugepage exists, but from the point of application, anyway unit of page size is 4KB.

[Geoffrey Garen]

Comment on attachment 387351 [details]
PATCH

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

Looks great to me -- but perhaps we should remove the #ifdefs.

>> Source/bmalloc/bmalloc/BPlatform.h:270
>> +#if BPLATFORM(iOS) || BPLATFORM(PLAYSTATION)
> 
> Typo, BPLATFORM(iOS) => BPLATFORM(IOS). BTW, I'm not sure whether all iOS uses 16KB pages.
> @Geoff, do we use 16KB for all iOS (not including watchOS, tvOS and Catalyst)?
> 
> For macOS, I think 4KB is OK. Hugepage exists, but from the point of application, anyway unit of page size is 4KB.

I believe some iOS variants still use 4kB pages. (Certainly they have used 4kB pages in the past.)

> Source/bmalloc/bmalloc/BPlatform.h:271
> +#define BUSE_PRECOMPUTED_CONSTANTS_VMPAGE16K 1

Since some Apple platforms vary page size at boot time or runtime rather than compile time, I think I'd prefer it if all Apple ports compiled in these values, and then selected the right variant to use at runtime. And perhaps that's the best strategy for all platforms -- unless the compiled size is prohibitive?

[Basuke Suzuki]

(In reply to Geoffrey Garen from comment #6)
> Comment on attachment 387351 [details]
> PATCH
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387351&action=review
> 
> Looks great to me -- but perhaps we should remove the #ifdefs.
> 
> >> Source/bmalloc/bmalloc/BPlatform.h:270
> >> +#if BPLATFORM(iOS) || BPLATFORM(PLAYSTATION)
> > 
> > Typo, BPLATFORM(iOS) => BPLATFORM(IOS). BTW, I'm not sure whether all iOS uses 16KB pages.
> > @Geoff, do we use 16KB for all iOS (not including watchOS, tvOS and Catalyst)?
> > 
> > For macOS, I think 4KB is OK. Hugepage exists, but from the point of application, anyway unit of page size is 4KB.
> 
> I believe some iOS variants still use 4kB pages. (Certainly they have used
> 4kB pages in the past.)
> 
> > Source/bmalloc/bmalloc/BPlatform.h:271
> > +#define BUSE_PRECOMPUTED_CONSTANTS_VMPAGE16K 1
> 
> Since some Apple platforms vary page size at boot time or runtime rather
> than compile time, I think I'd prefer it if all Apple ports compiled in
> these values, and then selected the right variant to use at runtime. And
> perhaps that's the best strategy for all platforms -- unless the compiled
> size is prohibitive?

Thanks for the review. Okay, I'll stop defining those macro in BPlatform.h, but define them explicitly in outside code. We want our code size minimum, I will add #if !defined() macro definition to make default precalculated for all page sizes and opt-out by defining them as zero explicitly.

[Basuke Suzuki]

Created attachment 387679 [details]
PATCH

[Basuke Suzuki]

Created attachment 387683 [details]
PATCH

[WebKit Commit Bot]

Comment on attachment 387683 [details]
PATCH

Clearing flags on attachment: 387683

Committed r254527: <https://trac.webkit.org/changeset/254527>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58577961>