Anton (Jun 23 2025 at 12:13): Anton (Jun 23 2025 at 12:13):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 :)
Brendan Hansknecht (Jun 23 2025 at 14:51):Yeah, zigs gpa in debug mode should cache a ton of things, but it never hurts to test with valgrind as well
Anton (Jun 28 2025 at 12:46):code run to generate snapshots is now checked by valgrind https://github.com/roc-lang/roc/pull/7893
Luke Boswell (Jul 09 2025 at 04:32):@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:
Joshua Warner (Jul 10 2025 at 21:08):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
Joshua Warner (Jul 10 2025 at 21:09):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.
Joshua Warner (Jul 10 2025 at 21:18):These seem to all be cases where something deep in the allocator is reading theoretically uninitialized memory
Joshua Warner (Jul 10 2025 at 21:29):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.
Joshua Warner (Jul 10 2025 at 21:48):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
Joshua Warner (Jul 10 2025 at 22:00):Calling all valgrind experts?
Joshua Warner (Jul 10 2025 at 22:03):I suppose as a temporary workaround I can switch to using couple arena allocators or something
Brendan Hansknecht (Jul 11 2025 at 02:48):Yeah, I was gonna guess either allocator change or some weird multithreaded complaint.
Brendan Hansknecht (Jul 11 2025 at 02:49):Out of curiousity does it have an issue if we run valgrind in the single threaded mode (probably, but worth double checking)
Brendan Hansknecht (Jul 11 2025 at 02:49):If not, I think we run valgrind on debug builds, so could always use gpa in debug builds.
Brendan Hansknecht (Jul 11 2025 at 02:49):Otherwise, I'm sure @Anton or I can take a more serious look.
Anton (Jul 11 2025 at 10:29):so could always use gpa in debug builds.
I think we currently do this @Joshua Warner?
Anton (Jul 11 2025 at 10:35):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?
Joshua Warner (Jul 11 2025 at 14:53):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.
Joshua Warner (Jul 11 2025 at 14:53):Based on local testing, arena allocators are something like 20% slower than the C allocator :/
Anton (Jul 11 2025 at 15:10):Ok, I'll check out the valgrind errors
Brendan Hansknecht (Jul 11 2025 at 15:30):We probably should make the main compiler use gpa in debug
Brendan Hansknecht (Jul 11 2025 at 15:30):Turn on the flags to help catch leaks and bugs and stuff
Anton (Jul 11 2025 at 15:41):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?
Brendan Hansknecht (Jul 11 2025 at 16:01):Yeah
Last updated: Jul 26 2025 at 12:14 UTC