Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Runtime page size detection #20511

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

archbirdplus
Copy link

@archbirdplus archbirdplus commented Jul 6, 2024

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

Notable changes:

  • Remove mem.page_size.
  • Add heap.min_page_size, heap.max_page_size, heap.pageSize().
  • Add options.min_page_size, options.max_page_size, options.queryPageSizeFn.

Not implemented:

  • A special-cased align_page alignment value. Use align(std.heap.min_page_size) on your pointers instead.

@archbirdplus archbirdplus marked this pull request as ready for review July 12, 2024 00:11
@archbirdplus archbirdplus requested a review from kprotty as a code owner July 12, 2024 00:11
@notcancername
Copy link
Contributor

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.

@archbirdplus
Copy link
Author

archbirdplus commented Jul 12, 2024

@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: #7539 (comment).
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.

@notcancername
Copy link
Contributor

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.

@archbirdplus
Copy link
Author

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

lib/std/heap.zig Outdated Show resolved Hide resolved
lib/std/heap.zig Outdated Show resolved Hide resolved
lib/std/heap.zig Outdated Show resolved Hide resolved
lib/std/heap.zig Outdated Show resolved Hide resolved
lib/std/heap.zig Outdated Show resolved Hide resolved
lib/std/heap.zig Outdated Show resolved Hide resolved
lib/std/heap.zig Outdated Show resolved Hide resolved
lib/std/heap.zig Outdated Show resolved Hide resolved
lib/std/heap.zig Outdated Show resolved Hide resolved
lib/std/heap.zig Outdated Show resolved Hide resolved
lib/std/heap.zig Outdated Show resolved Hide resolved
lib/std/heap.zig Outdated Show resolved Hide resolved
lib/std/heap.zig Outdated Show resolved Hide resolved
lib/std/heap.zig Outdated Show resolved Hide resolved
@archbirdplus
Copy link
Author

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?

@alexrp
Copy link
Member

alexrp commented Aug 10, 2024

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

@archbirdplus
Copy link
Author

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.

@rootbeer
Copy link
Contributor

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.

@archbirdplus archbirdplus requested a review from alexrp August 14, 2024 01:53
@archbirdplus archbirdplus requested a review from alexrp September 30, 2024 14:24
@alexrp
Copy link
Member

alexrp commented Sep 30, 2024

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.

@archbirdplus
Copy link
Author

Ah, I took being cross platform out of the box to be a requirement. I'll do that then.

lib/std/heap.zig Outdated Show resolved Hide resolved
lib/std/heap.zig Outdated Show resolved Hide resolved
@archbirdplus archbirdplus requested a review from alexrp October 4, 2024 20:28
lib/std/heap.zig Outdated Show resolved Hide resolved
lib/std/heap.zig Outdated Show resolved Hide resolved
lib/std/heap.zig Outdated Show resolved Hide resolved
@alexrp
Copy link
Member

alexrp commented Oct 5, 2024

Other than the above minor comments, I think this looks good. @andrewrk might want to take another look.

Copy link
Member

@alexrp alexrp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all my concerns have been addressed.

@alexrp

This comment was marked as resolved.

@RossComputerGuy
Copy link
Contributor

git rebase, not git merge please.

@archbirdplus archbirdplus force-pushed the asahi branch 2 times, most recently from c900fd3 to 84a301e Compare October 21, 2024 01:02
@RossComputerGuy
Copy link
Contributor

Needs a rebase.

@andrewrk
Copy link
Member

andrewrk commented Nov 4, 2024

@RossComputerGuy please do not ask other contributors to rebase their code.

@archbirdplus you do not need to rebase. This is waiting only on me to review and merge. Furthermore, since @alexrp already accepted it, if I find any issues, I will simply address them myself while merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
10 participants