Bug 236564 - [Linux/aarch64] Move page size ceiling to 16k
Summary: [Linux/aarch64] Move page size ceiling to 16k
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gustavo Noronha (kov)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-13 09:53 PST by Gustavo Noronha (kov)
Modified: 2022-02-21 03:53 PST (History)
10 users (show)

See Also:


Attachments
Patch (1.85 KB, patch)
2022-02-13 09:55 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff

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

r=me
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
<rdar://problem/88924594>
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.