Is now a good time to add some valgrind tests for the new compiler? It's always best to add these things early. I know zig allocators can prevent issues as well, but maybe we should still test with valgrind? Please share your wisdom, experienced zig programmers :)
Yeah, zigs gpa in debug mode should cache a ton of things, but it never hurts to test with valgrind as well
code run to generate snapshots is now checked by valgrind https://github.com/roc-lang/roc/pull/7893
@Anton adding this to CI has been very helpful. It picked up a few bugs in the module caching implementation that I couldn't on my mac. Thanks for setting this up :smiley:
I'm really struggling to make heads or tails of the current valgrind failures on this PR: https://github.com/roc-lang/roc/pull/7987
These look a hell of a lot like false-positives that need to be suppressed, but I'm rather confused as to why they would be appearing on this PR and not before.
These seem to all be cases where something deep in the allocator is reading theoretically uninitialized memory
It looks for all the world like this is a misconfiguration in zig / valgrind, perhaps one that's only triggered if we ever have a workload that ends up re-using memory that was freed previously.
Hmm, apparently this is an issue in main, and it bisects to:
82346e49b1e946b201ae3d7b2ea5a252f5d4429d is the first bad commit
commit 82346e49b1e946b201ae3d7b2ea5a252f5d4429d
Author: Joshua Warner <joshuawarner32@gmail.com>
Date: Wed Jul 9 09:57:25 2025 -0700
Switch to using the c_allocator by default for snapshots
This leads to a 4-6x speed increase in snapshot generation
.github/workflows/ci_zig.yml | 4 ++--
src/snapshot.zig | 26 +++++++++++++++++++-------
2 files changed, 21 insertions(+), 9 deletions(-)
I feel pretty firmly that that's NOT an indication of an actual bug, but rather some sort of misconfiguration in valgrind or zig
Calling all valgrind experts?
I suppose as a temporary workaround I can switch to using couple arena allocators or something
Yeah, I was gonna guess either allocator change or some weird multithreaded complaint.
Out of curiousity does it have an issue if we run valgrind in the single threaded mode (probably, but worth double checking)
If not, I think we run valgrind on debug builds, so could always use gpa in debug builds.
Otherwise, I'm sure @Anton or I can take a more serious look.
so could always use gpa in debug builds.
I think we currently do this @Joshua Warner?
Joshua Warner said:
I suppose as a temporary workaround I can switch to using couple arena allocators or something
I see we went with arena allocators, that may be even faster, why should we only use it temporarily?
So technically what's in main right now has us using the c allocator for arg parsing and such regardless - then we switch to the GPA for actual snapshot processing.
Based on local testing, arena allocators are something like 20% slower than the C allocator :/
Ok, I'll check out the valgrind errors
We probably should make the main compiler use gpa in debug
Turn on the flags to help catch leaks and bugs and stuff
Brendan Hansknecht said:
We probably should make the main compiler use gpa in debug
So, using e.g. comptime if (builtin.mode == .Debug) {
to set the allocator?
Yeah
Last updated: Jul 26 2025 at 12:14 UTC