Bug 206044 - [bmalloc] Calculate LineMetadata for specific VM page size in compile time
Summary: [bmalloc] Calculate LineMetadata for specific VM page size in compile time
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: 2020-01-09 17:29 PST by Basuke Suzuki
Modified: 2020-01-14 12:52 PST (History)
6 users (show)

See Also:


Attachments
FIRST TRIAL (8.80 KB, patch)
2020-01-09 17:30 PST, Basuke Suzuki
no flags Details | Formatted Diff | Diff
PATCH (10.70 KB, patch)
2020-01-10 10:36 PST, Basuke Suzuki
ysuzuki: review+
Details | Formatted Diff | Diff
PATCH (10.74 KB, patch)
2020-01-14 11:31 PST, Basuke Suzuki
no flags Details | Formatted Diff | Diff
PATCH (10.74 KB, patch)
2020-01-14 11:57 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 2020-01-09 17:29:27 PST
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.
Comment 1 Basuke Suzuki 2020-01-09 17:30:11 PST
Created attachment 387296 [details]
FIRST TRIAL
Comment 2 Basuke Suzuki 2020-01-09 17:41:05 PST
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
Comment 3 Basuke Suzuki 2020-01-09 17:42:46 PST
I'm curious about VM page size for watchOS and tvOS.
Comment 4 Basuke Suzuki 2020-01-10 10:36:36 PST
Created attachment 387351 [details]
PATCH
Comment 5 Yusuke Suzuki 2020-01-13 16:27:33 PST
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.
Comment 6 Geoffrey Garen 2020-01-13 17:02:46 PST
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?
Comment 7 Basuke Suzuki 2020-01-13 20:19:50 PST
(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.
Comment 8 Basuke Suzuki 2020-01-14 11:31:29 PST
Created attachment 387679 [details]
PATCH
Comment 9 Basuke Suzuki 2020-01-14 11:57:06 PST
Created attachment 387683 [details]
PATCH
Comment 10 WebKit Commit Bot 2020-01-14 12:51:36 PST
Comment on attachment 387683 [details]
PATCH

Clearing flags on attachment: 387683

Committed r254527: <https://trac.webkit.org/changeset/254527>
Comment 11 WebKit Commit Bot 2020-01-14 12:51:37 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2020-01-14 12:52:15 PST
<rdar://problem/58577961>