Skip to content

std.Build: Demote errors for exceeding max_rss to warnings.#23525

Merged
alexrp merged 2 commits intoziglang:masterfrom
alexrp:ci-max-rss
Jun 3, 2025
Merged

std.Build: Demote errors for exceeding max_rss to warnings.#23525
alexrp merged 2 commits intoziglang:masterfrom
alexrp:ci-max-rss

Conversation

@alexrp
Copy link
Member

@alexrp alexrp commented Apr 10, 2025

We have no control over memory usage on arbitrary systems in the wild. But we would still like to get the warnings so we can adjust the values based on observations in the official ZSF CI.

Closes #23254.
Closes #23638.

@alexrp alexrp marked this pull request as draft April 10, 2025 15:10
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Please do not add a ci option to the build script. The CI is there to make sure things work, with no exceptions or affordances made for it.

If someone is experiencing the max rss being incorrect, then we need to troubleshoot why sometimes the peak memory usage is much higher than expected. Or we need to decide that it is in fact expected and bump the max rss. Or perhaps we learn that it is target-dependent and decide that is acceptable.

@alexrp
Copy link
Member Author

alexrp commented Apr 10, 2025

Just to make sure I understand where you're at: In your view, what is the purpose of setting max_rss in our build.zig? FWIW, my thinking here is that, because this is the official Zig CI, we want to know when the memory usage of the compiler or test suite grows so that we can decide whether to accept that or fix what's causing it.

On the other hand, I don't see any clear value in enforcing max_rss on end-user machines. There are too many targets that the Zig compiler can be built on, and we only cover 5 in CI. Worse, we have extremely limited coverage for different page sizes, which is perhaps the bigger problem here.

Obviously we can just bump the values when users complain, but in my view, that isn't actually serving users well. Being greeted with a failed build due to a max_rss value that we picked, but which doesn't apply to the system that the user is building on, is confusing at best and pretty annoying at worst when it happens in an automated build environment. Even if we maintain target-specific max_rss values, if it's for a target we don't have CI for, that still means an end-user has to run into the limit and go open an issue or PR. Think about the implications of all this: It basically means that any given Zig release potentially needs downstream packagers to patch build.zig to get a successful build, because realistically we're not going to be testing the max_rss values on every conceivable system.

I think max_rss makes a lot of sense in CI because it's a tightly controlled environment, but not so much anywhere else.

@andrewrk
Copy link
Member

One more factor that you might not be aware of is that the max rss value is used by the build runner when scheduling tasks, to avoid running too many tasks at the same time that use a lot of memory. This avoids triggering OOM when using zig build - not only on the CI but also on users' machines.

So this change would cause zig build to start running into OOM when run locally.

@alexrp
Copy link
Member Author

alexrp commented Apr 10, 2025

That's a fair concern, but I think max_rss is a bit of a blunt instrument for dealing with that problem.

What do you think about an estimated_rss option of sorts? The build runner could use that to limit concurrency based on total system memory when max_rss isn't specified. We'd continue to set max_rss in CI, but users would just get our max_rss values as estimated_rss. From what I've seen, there isn't a huge difference between our declared values and what users observe, so I think this might strike a good balance in practice.

alexrp added 2 commits June 2, 2025 20:33
We have no control over memory usage on arbitrary systems in the wild. But we
would still like to get the warnings so we can adjust the values based on
observations in the official ZSF CI.

Closes #23254.
Closes #23638.
@alexrp alexrp changed the title Only enable max_rss options in CI std.Build: Demote errors for exceeding max_rss to warnings. Jun 2, 2025
@alexrp alexrp requested a review from andrewrk June 2, 2025 18:59
@alexrp
Copy link
Member Author

alexrp commented Jun 2, 2025

cc @Jan200101 @xtexx

@alexrp alexrp marked this pull request as ready for review June 2, 2025 19:27
@alexrp alexrp enabled auto-merge June 2, 2025 20:17
@alexrp alexrp merged commit f4f4460 into ziglang:master Jun 3, 2025
9 checks passed
@alexrp alexrp deleted the ci-max-rss branch June 4, 2025 03:15
BratishkaErik added a commit to BratishkaErik/gentoo that referenced this pull request Oct 23, 2025
Upstream changed behavior so that the "max_rss" field
in one line of their build.zig is now required:
ziglang/zig#25402

Drop the command removing it, as it is no longer needed.
"max_rss" errors were downgraded to warnings in:
ziglang/zig#23525

Upstream suggestion:
ziglang/zig#25659

Closes: https://bugs.gentoo.org/964953
Signed-off-by: Eric Joldasov <bratishkaerik@landless-city.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants