zig icon indicating copy to clipboard operation
zig copied to clipboard

Runtime page size detection

Open archbirdplus opened this issue 1 year ago • 16 comments

This PR implements runtime page sizes. Closes #4082. Closes #11308. Closes #16331. Closes #20714.

Notable changes:

  • [x] Introduce pageSize() to get the runtime page size.
  • [x] Introduce page_size_cap to get the comptime upper bound on page size.
  • [x] Correct the use of page_size in the standard library.
  • [x] Move page_size to from std.mem to std.heap.

Not implemented (debatable):

  • Zig could have a dedicated alignPage alignment. This would generate slightly stricter runtime validation than the current align(page_size).
  • Zig could add a compile option to override the page size. This wouldn't optimize much besides the GPA, but it would provide users with a fallback before editing the standard library.

archbirdplus avatar Jul 06 '24 00:07 archbirdplus

Hi, thanks so much for putting in the work to make runtime page size a reality :) These else prongs in heap.zig worry me, it seems too easy to add an architecture and have an incorrect page size set as default. I think a compile error would be more appropriate. Also, I think it would be good if pageSize was marked inline to propagate the comptime-ness of the result.

notcancername avatar Jul 12 '24 12:07 notcancername

@notcancername

Inlining: Looks like pageSize is already comptime and inlined during optimization. Comptime is a problem though. It suggests to some people that their code is valid when it isn't. I would like to ban the use of this function in comptime-dependent cases, demanding that the user prove that pageSize will be comptime (in which case they should use page_size instead). But sniffing for comptime is a bug, not a feature: https://github.com/ziglang/zig/issues/7539#issuecomment-750969917. Since inlining has no semantic difference, I'll inline it anyways.

Else: Do you mean the page_size else prongs? I suppose it's fine to break the few projects that depended on that.

archbirdplus avatar Jul 12 '24 17:07 archbirdplus

I would like to ban the use of this function in comptime-dependent cases, demanding that the user prove that pageSize will be comptime (in which case they should use page_size instead).

Thanks, I am in favor of this. I believe you may be able to achieve this by assigning to a var and returning that. For what it's worth, #868 has been implemented, so you might want to use that.

Do you mean the page_size else prongs? I suppose it's fine to break the few projects that depended on that.

Yes, page_size should match all current CPU architectures, so that when a new architecture is added, the implementer gets a compile error when code tries to access page_size (ditto for page_size_cap), and adds the correct value.

notcancername avatar Jul 12 '24 20:07 notcancername

Local tests now pass. I just can't figure out how to test on Windows.

archbirdplus avatar Jul 15 '24 09:07 archbirdplus

I'd like to re-run CI. I cannot get local tests to pass, but I would like to see how the CI runner behaves.

There seems to be another issue besides page size that prevents me from running tests myself. Running the test command from the CI script on Asahi Linux fails like so:

+ ./stage4/bin/zig build test docs --maxrss 24696061952 -fwasmtime -Denable-llvm -Dtarget=native-native-musl --zig-lib-dir /home/arch/zig/lib --cache-dir /home/arch/zig/builddir-alt/.zig-cache -Dskip-release
test
└─ zig build-exe zig Debug native-native-musl 2 errors
error: ld.lld: undefined symbol: __libc_single_threaded
    note: referenced by zig_llvm.cpp
    note:               zig_llvm.cpp.o:(ZigLLVMCreateTargetMachine) in archive /home/arch/zig/builddir-alt/zigcpp/libzigcpp.a
    note: referenced by zig_llvm.cpp
    note:               zig_llvm.cpp.o:(ZigLLVMCreateTargetMachine) in archive /home/arch/zig/builddir-alt/zigcpp/libzigcpp.a
    note: referenced by zig_clang.cpp
    note:               zig_clang.cpp.o:(ZigClangLoadFromCommandLine) in archive /home/arch/zig/builddir-alt/zigcpp/libzigcpp.a
    note: referenced 17 more times
error: ld.lld: undefined symbol: __isoc23_strtol
    note: referenced by zig_llvm.cpp
    note:               zig_llvm.cpp.o:(ZigLLVMTargetMachineEmitToFile) in archive /home/arch/zig/builddir-alt/zigcpp/libzigcpp.a
error: the following command failed with 2 compilation errors:
/home/arch/zig/builddir-alt/stage4/bin/zig build-exe --stack 33554432 /home/arch/zig/builddir-alt/zigcpp/libzigcpp.a /usr/lib64/libclang-cpp.so /usr/lib64/liblldMinGW.so /usr/lib64/liblldELF.so /usr/lib64/liblldCOFF.so /usr/lib64/liblldWasm.so /usr/lib64/liblldMachO.so /usr/lib64/liblldCommon.so /usr/lib64/libLLVM-18.so -D_DEFAULT_SOURCE -D_XOPEN_SOURCE=600 -ltinfo /usr/lib64/libz.so /usr/lib/gcc/aarch64-redhat-linux/14/libstdc++.so -lunwind -fno-sanitize-thread -ODebug -target native-native-musl -mcpu native -I /usr/include -I /usr/include -L /usr/lib64 --dep aro --dep aro_translate_c --dep build_options -Mroot=/home/arch/zig/src/main.zig -Maro=/home/arch/zig/lib/compiler/aro/aro.zig --dep aro -Maro_translate_c=/home/arch/zig/lib/compiler/aro_translate_c.zig -Mbuild_options=/home/arch/zig/builddir-alt/.zig-cache/c/e867395cb58fc12949748f95b1b06d98/options.zig -lc --cache-dir /home/arch/zig/builddir-alt/.zig-cache --global-cache-dir /home/arch/.cache/zig --name zig --zig-lib-dir /home/arch/zig/lib/ --listen=-
test
└─ test-std
   └─ run test std-wasm32-wasi.0.1...0.1-musl-generic-Debug-libc 2600/2718 passed, 1 failed, 117 skipped
error: 'fs.test.test.file operations on directories' failed: expected error.IsDir, found fs.File{ .handle = 7 }
Unable to dump stack trace: not implemented for Wasm
error: while executing test 'zig.system.darwin.macos.test.detect', the following test command failed:
wasmtime --dir=. -- /home/arch/zig/builddir-alt/.zig-cache/o/0d427260497117aec1db32d42886460a/test.wasm --seed=0x563739fd --listen=-

Whether I use this PR or just edit the page_size alone, the errors are the same, so at least I haven't created any new errors. Before rebasing this PR today, I had completely different errors about the wasmtime test runner reaching unreachable code while parsing arguments. Running tests without any changes to Zig would crash with the classic #16331.

Do these errors suggest anything in particular? Maybe I didn't replicate the CI script closely enough?

archbirdplus avatar Aug 09 '24 06:08 archbirdplus

I think you might need to rebase this on master for CI to run? :thinking:

alexrp avatar Aug 10 '24 17:08 alexrp

Alright, rebased again. Though what I understood was that CI needed to be invoked by a Zig member. Hopefully I can fix up the stylistic issues soon.

archbirdplus avatar Aug 11 '24 00:08 archbirdplus

For the wasm failure in your local run of the tests, I see:

error: 'fs.test.test.file operations on directories' failed: expected error.IsDir, found fs.File{ .handle = 7 } Unable to dump stack trace: not implemented for Wasm error: while executing test 'zig.system.darwin.macos.test.detect', the following test command failed: wasmtime --dir=. -- /home/arch/zig/builddir-alt/.zig-cache/o/0d427260497117aec1db32d42886460a/test.wasm --seed=0x563739fd --listen=-

I suspect this is from using a too-recent wasmtime, and downgrading to v10.0.2 (which is the version CI runs) will fix it. See #20747.

rootbeer avatar Aug 11 '24 06:08 rootbeer

I believe this also resolves #4082, #11308, and #20714. Please put "closes #..." in the PR description for each of these as well.

alexrp avatar Aug 17 '24 22:08 alexrp

I can't think of anything else that needs changes.

archbirdplus avatar Aug 18 '24 22:08 archbirdplus

Just to be sure, I'll do a final check over all the min/max values before signing off on this. It's looking good so far.

alexrp avatar Aug 19 '24 11:08 alexrp

Some more to add:

  • solaris/illumos: 4K min/max for x86/x86_64.
  • fuchsia: 4K min/max for x86_64, arm64, riscv64.
  • serenity: 4K min/max everywhere. (cc @linusg to double-check)

alexrp avatar Aug 19 '24 19:08 alexrp

serenity: 4K min/max everywhere. (cc @linusg to double-check)

Correct: https://github.com/SerenityOS/serenity/blob/62b938b798dc009605b5df8a71145942fc53808b/Kernel/API/POSIX/sys/limits.h#L11-L13

linusg avatar Aug 19 '24 20:08 linusg

Updated. @alexrp, I did not find a riscv page size for fuchsia. Do you have a source for that? https://fuchsia.googlesource.com/fuchsia/+/8b0e396b54a587bc65ed8ab8a521f26b6a3c5cb2/docs/reference/syscalls/system_get_page_size.md

archbirdplus avatar Aug 21 '24 22:08 archbirdplus

Everything I wrote above is based on inspecting the source code for each OS. For Fuchsia, that's zircon/kernel/arch/$arch/include/arch/defines.h.

alexrp avatar Aug 21 '24 22:08 alexrp

I guess I might as well just finish up the remaining few :shrug:

archbirdplus avatar Aug 23 '24 03:08 archbirdplus

We are now missing only runtime querying for plan9, and comptime sizes for contiki, rtems, elfiamcu, hermit, aix, zos, and GPU targets. There targets are all either too confusing or too closed source for me to add.

archbirdplus avatar Aug 25 '24 00:08 archbirdplus

Other than the Plan 9 comment above, I think I'm comfortable signing off on the page size values picked here and the overall direction of the changes. @andrewrk I think now would be a good time for you to review the resulting changes throughout the standard library; you're probably more qualified for that than me.

alexrp avatar Aug 26 '24 01:08 alexrp

Thanks, looking now.

andrewrk avatar Aug 26 '24 22:08 andrewrk

Looks like this needs a rebase.

Also, what's the status of this? I've been working on other stuff, so not sure where we're at with this overall.

alexrp avatar Sep 17 '24 02:09 alexrp

I'd like to double-check it again before marking it ready. There are still minor issues apparently.

Aside from that, I don't know how to handle freestanding/other elegantly. I'm considering just adopting the extreme values from the other OSes.

archbirdplus avatar Sep 18 '24 02:09 archbirdplus

You shouldn't handle those since, by definition, there is no known OS in those cases - not even a bare runtime environment like UEFI. At most, you could just add a bring-your-own-OS hook for people to define a page size value if they need it, but I don't think you need to do that now unless there's an actual immediate need.

alexrp avatar Sep 18 '24 03:09 alexrp

Alright. If we care at all about GPA/mmap on freestanding+libc (which some people apparently do use), I'll do the following:

  • introduce has_page_size_bounds
  • introduce min_page_alignment, which falls back to 1 if min_page_size would error
  • let GPA fall back to 4K bucket size if there is no max_page_size

That would let us keep our page-aligned pointers and avoid breaking GPA needlessly.

Limiting the bucket classes for GPA will cause fragmentation for certain large objects, but otherwise has less overhead compared to dynamically allocating the exact number of buckets. I would leave optimizing the freestanding case for a subsequent PR if needed.

archbirdplus avatar Sep 18 '24 06:09 archbirdplus

Alright. If we care at all about GPA/mmap on freestanding+libc (which some people apparently do use)

Who/what/where? That doesn't sound like a configuration we support, but I could be forgetting something.

alexrp avatar Sep 18 '24 22:09 alexrp

Nothing seems to prevent one from using libc. Freestanding is listed as Tier 1 support, so:

  • the Standard Library cross-platform abstractions have implementations for these targets
  • libc is available for this target even when cross compiling

Here's one commit complaining about #20511 breaking GPA: https://github.com/kivikakk/ava/commit/263634dfc8c2e3a8ec136fcb6a0fc49f34fce5f4. I don't know if anyone else has this problem though.

archbirdplus avatar Sep 19 '24 06:09 archbirdplus

Nothing seems to prevent one from using libc.

None of the libcs we support work on freestanding; they all assume an OS exists (and usually a specific one). glibc, musl, wasi-libc, MinGW-w64, UCRT, libSystem, native libcs for other OSs (Solaris, BSDs, Haiku, ...), etc are all like this.

libc is available for this target even when cross compiling

I think this is just a case of the release notes being a bit imprecise for the sake of readability.

Here's one commit complaining about #20511 breaking GPA: kivikakk/ava@263634d. I don't know if anyone else has this problem though.

If GPA works on freestanding today, then we do care about keeping it working for sure. But I don't see how that's connected to libc since we don't currently support any libc on freestanding.

alexrp avatar Sep 19 '24 06:09 alexrp

Looks to me like the default setup has GPA backed by PageAllocator, and PageAllocator backed by mmap. Unless the system is known, mmap is provided by libc. Am I reading this right?

archbirdplus avatar Sep 19 '24 07:09 archbirdplus

GPA is backed by backing_allocator, which defaults to page_allocator. A freestanding user could initialize a GPA with a custom backing allocator that doesn't use std.posix.mmap.

To support freestanding std.heap.pageSize, an item field could be added to std.Options: query_page_size: fn() usize = std.heap.defaultQueryPageSize, and std.heap.pageSize() can call std.options.query_page_size() instead of queryPageSize()

pfgithub avatar Sep 19 '24 07:09 pfgithub

To support freestanding std.heap.pageSize, an item field could be added to std.Options

That sounds like a good idea, we already do it for various other things and it would enable the user to override the page size for freestanding and work around not-yet-supported targets.

notcancername avatar Sep 19 '24 07:09 notcancername

Just to make sure I correctly understand the motivation for the latest commits: The issue is that there's no page size defined for freestanding/other, which means we'll get compile errors in the GPA implementation (and probably other places), and these changes provide a minimum viable fallback for that case, right?

Assuming my understanding is correct, and assuming that this is the only motivation for these changes, then I would instead suggest just doing what @pfgithub suggested. It's a much smaller change and keeps the std.heap API simple.

Basically, you just add a few fields to std.Options looking something like this:

pub const Options = struct {
    // ...

    min_page_size: ?usize = null,
    max_page_size ?usize = null,
    query_page_size: fn () usize = heap.defaultQueryPageSize,

    // ...
};

Now you change the std.heap.(min,max)_page_size definitions to prefer std.options.(min,max)_page_size if non-null, and otherwise use the usual arch/OS logic, and fall back to a compile error. (Bonus points for making the compile error point to these std.Options fields in the freestanding/other cases so users know what to do in that case.)

Then you rename std.heap.queryPageSize() to defaultQueryPageSize() and make it pub. Finally, you change the std.heap.pageSize() implementation to call std.options.query_page_size instead of calling defaultQueryPageSize() directly. (Again, bonus points for making the compile error more helpful in the freestanding/other cases.)

And that should be it, I think. Now programmers targeting freestanding/other can use the usual std.Options mechanism to provide their own page size information without the standard library needing to go out of its way to accommodate them.

alexrp avatar Sep 30 '24 23:09 alexrp