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

Fix stack overflow in filter #9430

Merged
merged 1 commit into from
Nov 30, 2023
Merged

Conversation

roberth
Copy link
Member

@roberth roberth commented Nov 21, 2023

Motivation

Remove VLAs, attempt 2 with traceable allocator.
Fixes stack overflow in builtins.filter on large lists.

Context

Requires a trivial patch in bdwgc (patch included in the flake)

Priorities

Add 👍 to pull requests you find important.

@roberth roberth requested a review from edolstra as a code owner November 21, 2023 19:56
@edolstra
Copy link
Member

To be honest, I'm really not convinced this is worth it the effort. VLAs work, they have worked for decades, and they're not suddenly going to stop working - and if they do, then we can get rid of them.

@roberth roberth changed the title Use boost::container::small_vector in place of VLAs Fix stack overflow in filter Nov 22, 2023
@Ericson2314
Copy link
Member

I don't really have anything against VLAs, but being able to fall back on heap allocations when the stack is filling up seems good.

VLA dynamic sized small vec (do some sort of stack check to decide VLA or regular heap vector) works, but unconditional VLA doesn't satisfy the above.

@roberth
Copy link
Member Author

roberth commented Nov 22, 2023

Indeed I'm not doing this for the VLAs but for the stack overflows. The PR only had that as a title for continuity with @thufschmitt's work.

VLA dynamic sized small vec

This would seem optimal but leads to somewhat more complicated code because the VLAs can't be put into a type. Also this suggests a small performance penalty with VLAs:

It's also incidentally consistently 1% faster on the benchmarks.

In fact I'm quite ok with VLAs, as long as they're not chocolate flavored.
It's just that we have a big reason (stack overflow) and two minor reasons to avoid them.

@edolstra edolstra merged commit cb7f258 into NixOS:master Nov 30, 2023
11 checks passed
@fricklerhandwerk fricklerhandwerk added bug language The Nix expression language; parser, interpreter, primops, evaluation, etc labels Jan 8, 2024
tebowy pushed a commit to tebowy/nix that referenced this pull request Jul 11, 2024
Fix stack overflow in `filter`

(cherry picked from commit cb7f258)
Change-Id: Ib90f97a9805bbb4d0e2741551d490f054fc0a675
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug language The Nix expression language; parser, interpreter, primops, evaluation, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants