Bug 236564

Summary: [Linux/aarch64] Move page size ceiling to 16k
Product: WebKit Reporter: Gustavo Noronha (kov) <gustavo>
Component: New BugsAssignee: Gustavo Noronha (kov) <gustavo>
Severity: Normal CC: benjamin, bugs-noreply, cdumez, cmarcelo, ews-watchlist, mcatanzaro, webkit-bug-importer, ysuzuki, zan, zdobersek
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch none

Description Gustavo Noronha (kov) 2022-02-13 09:53:53 PST
[Linux/aarch64] Move page size ceiling to 16k
Comment 1 Gustavo Noronha (kov) 2022-02-13 09:55:51 PST
Created attachment 451821 [details]
Comment 2 Gustavo Noronha (kov) 2022-02-13 09:57:39 PST
No visible impact on the binary size:

kov@uva ~/P/WebKit (16k)> ls -l WebKitBuild/Release-*/lib/*.so.*.*.*
-rwxr-xr-x. 1 kov kov 27263544 fev 13 13:55 WebKitBuild/Release-16k/lib/libjavascriptcoregtk-4.0.so.18.20.2
-rwxr-xr-x. 1 kov kov 98987608 fev 13 14:15 WebKitBuild/Release-16k/lib/libwebkit2gtk-4.0.so.37.56.2
-rwxr-xr-x. 1 kov kov 27263544 fev 13 14:17 WebKitBuild/Release-4k/lib/libjavascriptcoregtk-4.0.so.18.20.2
-rwxr-xr-x. 1 kov kov 98987608 fev 13 14:23 WebKitBuild/Release-4k/lib/libwebkit2gtk-4.0.so.37.56.2

I did check that the files are actually different - the 4k one crashes on my 16k page size Linux on M1.
Comment 3 Yusuke Suzuki 2022-02-14 09:21:40 PST
Comment on attachment 451821 [details]

Comment 4 EWS 2022-02-14 12:27:35 PST
gns@gnome.org does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/metadata/contributors.json.

Rejecting attachment 451821 [details] from commit queue.
Comment 5 EWS 2022-02-14 13:14:06 PST
Committed r289761 (247233@main): <https://commits.webkit.org/247233@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 451821 [details].
Comment 6 Radar WebKit Bug Importer 2022-02-14 13:15:37 PST
Comment 7 Michael Catanzaro 2022-02-15 07:46:16 PST
This might have performance implications for embedded systems, not sure.
Comment 8 Zan Dobersek 2022-02-15 22:19:34 PST
Ideally the page size would be detected and used at runtime. This isn't possible since it's used at build-time in different places.

Last time round, the increase was to 64kB, which was causing crashes in general but it also (most prominently) increased the size of each MarkedBlock in JSC, meaning in certain situation the JS memory usage would have increased. See bug #209670.

This change does the increase to 16kB which matches minimum MarkedBlock size, so there's no change there. But in general, if it were possible to detect at build-time whether the build is intended for Apple silicon due to some specific tune option in GCC/Clang, I only enabling 16kB ceiling for ARM64 under that condition would be preferred.
Comment 9 Gustavo Noronha (kov) 2022-02-19 10:26:08 PST
Only enabling it if building specifically for Apple Silicon would mean distributions would need to have a port specifically for that to support the 16k page size kernel, which would be a shame.

I expect it will be more common to have custom builds for specific embedded devices than M1 as Asahi gains features and stabilizes (it probably already is right now with the exception of maybe Raspberry Pi?). Is there a particular platform you are worried about? Maybe we can run some performance measurements for that and make sure it isn't really a concern.

All that said, since we are not increasing binary size and are matching the minimum MarkedBlock size we should not really have any issues, be it functional or performance, right? I keep wondering if we should not just increase all of Linux to a 16k ceiling as a minimum tbh, match what Apple did as it will probably be the best supported configuration in the long run.
Comment 10 Gustavo Noronha (kov) 2022-02-19 10:52:55 PST
I was also doing some reading on bmalloc's replacement libpas and it also seems to use 16k as the minimum page size it cares about (unless it is configured to use 64k pages), so I guess unless the system malloc is used (which would probably be a big perf hit) we are already pretty much at a 16k minimum right now (which is what keeps me going back to maybe we should just push all of Linux up and future-proof potentially moving the other arches to bigger page sizes as well).

kov@morango ~/P/WebKit (16k)> git grep -e '.small_segregated_page_size =' Source/
Source/bmalloc/libpas/Documentation.md:        .small_segregated_page_size = PAS_SMALL_PAGE_DEFAULT_SIZE, \
Source/bmalloc/libpas/src/libpas/bmalloc_heap_config.h:    .small_segregated_page_size = PAS_SMALL_PAGE_DEFAULT_SIZE, \
Source/bmalloc/libpas/src/libpas/hotbit_heap_config.h:    .small_segregated_page_size = PAS_SMALL_PAGE_DEFAULT_SIZE, \
Source/bmalloc/libpas/src/libpas/iso_heap_config.h:    .small_segregated_page_size = PAS_SMALL_PAGE_DEFAULT_SIZE, \
Source/bmalloc/libpas/src/libpas/iso_test_heap_config.h:    .small_segregated_page_size = PAS_SMALL_PAGE_DEFAULT_SIZE, \
Source/bmalloc/libpas/src/libpas/minalign32_heap_config.h:    .small_segregated_page_size = PAS_SMALL_PAGE_DEFAULT_SIZE, \
Source/bmalloc/libpas/src/libpas/pagesize64k_heap_config.h:    .small_segregated_page_size = PAGESIZE64K_SMALL_PAGE_SIZE, \
Source/bmalloc/libpas/src/libpas/thingy_heap_config.h:    .small_segregated_page_size = PAS_SMALL_PAGE_DEFAULT_SIZE, \
kov@morango ~/P/WebKit (16k)> git grep 'define PAS_SMALL_PAGE_DEFAULT_SIZE' Source/
Source/bmalloc/libpas/src/libpas/pas_internal_config.h:#define PAS_SMALL_PAGE_DEFAULT_SIZE      ((size_t)1 << PAS_SMALL_PAGE_DEFAULT_SHIFT)
kov@morango ~/P/WebKit (16k)> git grep 'define PAS_SMALL_PAGE_DEFAULT_SHIFT' Source/
Source/bmalloc/libpas/src/libpas/pas_internal_config.h:#define PAS_SMALL_PAGE_DEFAULT_SHIFT     14
kov@morango ~/P/WebKit (16k)>
Comment 11 Zan Dobersek 2022-02-20 23:20:58 PST
These changes would be easier on the mind if there was data provided in parallel on how memory consumption behaves on different benchmarks.

But that's about page sizes in general. This specific change doesn't really affect that. It doesn't affect much in general.
Comment 12 Zan Dobersek 2022-02-20 23:31:15 PST
Concretely, small-page size in vanilla bmalloc is still 4kB. If you're not using libpas, you should be hitting assertions in bmalloc::HeapConstants::HeapConstants() on Asahi.

I don't know the state of libpas on ARMv7, I would have to check with others. But yeah, quadrupled small-page size on those platforms is possibly not great.
Comment 13 Gustavo Noronha (kov) 2022-02-21 03:53:13 PST
I'd argue that even if it does affect armv7* it probably makes more sense to add tweaks / build time detection for them and keep 16k as the default, as that is what will make more sense for most people using regular distributions rather than custom-building for their devices, especially going forward. It is quite easy for us to even carry a patch for that kind of thing on our custom images for these boards.

* I think you're mainly concerned about the likes of imx6, raspberry pi 3 and other boards of that age? I'll see if I can get my imx6 sabrelite up and running to do some testing.