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

add std.heap.AutoAllocator #23432

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

Conversation

gooncreeper
Copy link
Contributor

Provides a convenient abstraction for this relatively complex logic.

Additionally, utilizes it in src/main.zig.

Provides a convenient abstraction for this relatively complex logic.

Additionally, utilizes it in src/main.zig.
@alexrp
Copy link
Member

alexrp commented Apr 2, 2025

This will probably need an init() method accepting a struct of options that gets passed to the underlying allocator.

@alexrp
Copy link
Member

alexrp commented Apr 2, 2025

Test failures happening because build_options.debug_gpa is no longer respected in the compiler.

@gooncreeper gooncreeper changed the title add std.heap.DefaultAllocator add std.heap.AutoAllocator Apr 2, 2025
@tensorush
Copy link
Contributor

tensorush commented Apr 5, 2025

If I can throw my two cents in here, could we bring the GeneralPurposeAllocator name back for this?

Auto and Allocator side-by-side might be misinterpreted as "an allocator that auto-manages it's allocations", instead of "an allocator that auto-initializes an allocator instance for the general case".

Also, gpa is a pretty nice and already established convention for naming such general-purpose allocator instances.

Basically, I'd love to have this code working as is again:

var gpa_state: std.heap.GeneralPurposeAllocator(.{}) = .init;
const gpa = gpa_state.allocator();
defer if (gpa_state.deinit() == .leak) {
    @panic("Memory leak has occurred!");
};

@alexrp
Copy link
Member

alexrp commented Apr 5, 2025

FWIW, the name that was suggested on Zulip was DefaultAllocator.

@gooncreeper
Copy link
Contributor Author

gooncreeper commented Apr 5, 2025

I think both can work. I changed it to AutoAllocator since it is similar to AutoHashMap in the sense it automatically chooses the implementation, and it also leads to a bit more friction over choosing an allocator yourself. Though, I can change it back if you think DefaultAllocator makes more sense.

@gooncreeper
Copy link
Contributor Author

@alexrp The CI timeout is due to the compiler now defaulting to the DebugAllocator for debug builds. Based on empirical testing, it seems that it is 70% slower for x86_64-linux-debug (this was also the case before the patch if you passed -Ddebug-allocator), meaning it would take a bit over 8.5 hours to complete (on aarch-linux-debug it took a 30% hit and aarch-macos-debug a 20% hit.) I can change it back to using a faster allocator unless you think it makes sense to bump the CI time.

@alexrp
Copy link
Member

alexrp commented Apr 7, 2025

I can change it back to using a faster allocator

That would probably be best.

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.

4 participants