Skip to content

Fix stack overflow in filter#9430

Merged
edolstra merged 1 commit intoNixOS:masterfrom
hercules-ci:remove-vlas
Nov 30, 2023
Merged

Fix stack overflow in filter#9430
edolstra merged 1 commit intoNixOS:masterfrom
hercules-ci:remove-vlas

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
@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