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

support building with --enable-gc=no #6741

Merged
merged 1 commit into from
Dec 13, 2022
Merged

Conversation

Mindavi
Copy link
Contributor

@Mindavi Mindavi commented Jun 29, 2022

Some minor changes fixing the build without boehm.
Fixes #6250.

Would be nice to test this (building without boehm) somehow (in hydra maybe), but haven't looked at that yet and IMO that can be added separately. Could help prevent or reduce regressions on this configuration.

I'm interested in this to possibly make ASAN builds more reliable. Not sure if this will really help, but since I was looking at it anyway and the fixes are low-hanging fruit, I've submitted it 😄 .

Not 100% sure about the AllocCache members, but in the cc file there's a path for both boehm enabled and disabled where the members are needed, so I just assumed that that was correct and this corresponds to that.

@Mindavi Mindavi marked this pull request as draft June 29, 2022 22:21
@Mindavi
Copy link
Contributor Author

Mindavi commented Jun 29, 2022

Hmm, not sure if this makes sense. The valueAllocCache only seems to be used when HAVE_BOEHMGC is defined. So the expression in the cc file should be fixed rather than this in the header :). Will revisit later (feel free to pick it up if I forget about it).

@thufschmitt
Copy link
Member

The valueAllocCache only seems to be used when HAVE_BOEHMGC is defined. So the expression in the cc file should be fixed rather than this in the header :)

Yes, these alloc caches are only used to batch the allocations with the GC because allocating a lot of small values is fairly slow. That being said, it probably doesn't hurt to keep the exact same logic when not using the GC to keep things simple.

Would be nice to test this (building without boehm) somehow (in hydra maybe)

Absolutely. A hydra job for that sounds like the best solution.

Some minor changes fixing the build without boehm.
Fixes NixOS#6250
Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

Thanks, let's merge

@thufschmitt thufschmitt merged commit 6e31d27 into NixOS:master Dec 13, 2022
@Mindavi Mindavi deleted the nix-no-gc branch December 14, 2022 07:02
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.

build failure without boehm GC
2 participants