Use hybrid C / Pascal strings in the evaluator #14442

glittershark · 2025-11-01T18:45:25Z

Motivation

This is a draft PR to replace the null-terminated C-style strings in Value with pascal strings, where the length is stored in the allocation before the data. This should be considered basically an experiment right now - this ended up being a big optimization for tvix, but it's unclear how much of a win it'd be for cppnix

Context

Depends on #14444
Depends on #14470

Ericson2314 · 2025-11-01T19:04:10Z

CC @Radvendii

Ericson2314 · 2025-11-01T19:27:01Z

OK I am taking a look at the sanitzer failure.

xokdvium · 2025-11-01T19:31:12Z

Would to see this through :)

Ericson2314 · 2025-11-01T22:38:25Z

 eval-nixos> error:
eval-nixos>        … while evaluating the attribute 'python310Packages'
eval-nixos>          at /nix/store/8sm7wycza5fl6lnsmw11sn4vqkvw4xyh-source/pkgs/top-level/all-packages.nix:13740:3:
eval-nixos>         13739|   python39Packages = recurseIntoAttrs python39.pkgs;
eval-nixos>         13740|   python310Packages = recurseIntoAttrs python310.pkgs;
eval-nixos>              |   ^
eval-nixos>         13741|   python311Packages = python311.pkgs;
eval-nixos> 
eval-nixos>        … while evaluating the attribute 'h5py-mpi'
eval-nixos>          at /nix/store/8sm7wycza5fl6lnsmw11sn4vqkvw4xyh-source/pkgs/top-level/python-packages.nix:3594:3:
eval-nixos>          3593|
eval-nixos>          3594|   h5py-mpi = self.h5py.override {
eval-nixos>              |   ^
eval-nixos>          3595|     hdf5 = pkgs.hdf5-mpi;
eval-nixos> 
eval-nixos>        (stack trace truncated; use '--show-trace' to show the full, detailed trace)
eval-nixos> 
eval-nixos>        error: store path '-I/nix/store/j8kxvbz8qnfpbcdq3gjnbzagd3mqr4wv-libffi-3.4.2-dev/include' contains illegal base-32 character '-'

it would be lovely to minimize this from the VM test!

edolstra · 2025-11-03T16:52:52Z

What sort of performance improvement can we expect from this? (Memory, speed?)

glittershark · 2025-11-03T17:38:00Z

What sort of performance improvement can we expect from this? (Memory, speed?)

I think it's hard to say. For tvix the big performance win was saving a word of cache footprint by converting rust fat pointers to thin pointers. For C++nix the value representation already uses only one word for the pointer (and another word for context iirc) so the win is less clear. Plausibly length gets cheaper (I have definitely worked in systems where strlen is really costly!) but ime length doesn't end up being that hot in Nix evaluations.

On the other hand, tvix had to incur the cost of an extra pointer indirection to find string length, so maybe the delta in this case is more clearly beneficial, since c++nix already has to chase a pointer to look up the length of a string.

As is always the case, we should benchmark.

Ericson2314 · 2025-11-03T18:04:37Z

In the meeting (going on right now) we also talked about how parsers going through strings character by character were needlessly quadratic. This might not be in our test suite today, but @tomberek can easily add such an example.

I would expect that since this dramatically includes that use-case, even if it is doesn't improve other use-cases (but also doesn't make them worse) the PR should overall be worth it.

In short, the plan to me is:

Benchmark
If seems worth it, merge
Otherwise, add parser test case
Benchmark again
Almost certainly will be worth it (unless we made a mistake in the implementation)
Merge

So all paths lead to merging, but we do make sure we have evidence first.

Radvendii · 2025-11-03T18:22:33Z

I am also getting the "store path contains illegal base-32 character" error locally, but it looks like a normal store path in this case, and it's complaining about the path separator "/"

$ result/bin/nix eval /path/to/my/nixos/flake#nixosConfigurations.smudge.config.system.build.toplevel
warning: Git tree '/path/to/my/nixos' is dirty
error:
       … while calling the 'head' builtin
         at «github:NixOS/nixpkgs/5b09dc45f24cf32316283e62aec81ffee3c3e376?narHash=sha256-Q/I2xJn/j1wpkGhWkQnm20nShYnG7TI99foDBpXm1SY%3D»/lib/attrsets.nix:1534:13:
         1533|           if length values == 1 || pred here (elemAt values 1) (head values) then
         1534|             head values
             |             ^
         1535|           else

       … while evaluating the attribute 'value'
         at «github:NixOS/nixpkgs/5b09dc45f24cf32316283e62aec81ffee3c3e376?narHash=sha256-Q/I2xJn/j1wpkGhWkQnm20nShYnG7TI99foDBpXm1SY%3D»/lib/modules.nix:1085:7:
         1084|     // {
         1085|       value = addErrorContext "while evaluating the option `${showOption loc}':" value;
             |       ^
         1086|       inherit (res.defsFinal') highestPrio;

       … while evaluating the option `system.build.toplevel':

       … while evaluating definitions from `/nix/store/4danq4f7rxpsax7bx18709c533v9krii-source/nixos/modules/system/activation/top-level.nix':

       … while evaluating the option `warnings':

       … while evaluating definitions from `/nix/store/4danq4f7rxpsax7bx18709c533v9krii-source/nixos/modules/system/boot/systemd/tmpfiles.nix':

       … while evaluating the option `systemd.tmpfiles.settings.graphics-driver."/run/opengl-driver-32"."L+".argument':

       … while evaluating definitions from `/nix/store/4danq4f7rxpsax7bx18709c533v9krii-source/nixos/modules/hardware/graphics.nix':

       (stack trace truncated; use '--show-trace' to show the full, detailed trace)

       error: store path '/nix/store/6yd4sqywxnh3wjr0s14l68szs6jnqqs1-replace-distutils.patch.drv' contains illegal base-32 character '/'

Radvendii · 2025-11-03T18:25:28Z

Invoking it a different way I get this, which distinctly does not look like a store path.

$ ./build/src/nix/nix eval /path/to/my/nixos/flake#nixosConfigurations.smudge.config.system.build.toplevel
warning: Git tree '/path/to/my/nixos/flake' is dirty
error:
       … while calling the 'head' builtin
         at «github:NixOS/nixpkgs/5b09dc45f24cf32316283e62aec81ffee3c3e376?narHash=sha256-Q/I2xJn/j1wpkGhWkQnm20nShYnG7TI99foDBpXm1SY%3D»/lib/attrsets.nix:1534:13:
         1533|           if length values == 1 || pred here (elemAt values 1) (head values) then
         1534|             head values
             |             ^
         1535|           else

       … while evaluating the attribute 'value'
         at «github:NixOS/nixpkgs/5b09dc45f24cf32316283e62aec81ffee3c3e376?narHash=sha256-Q/I2xJn/j1wpkGhWkQnm20nShYnG7TI99foDBpXm1SY%3D»/lib/modules.nix:1085:7:
         1084|     // {
         1085|       value = addErrorContext "while evaluating the option `${showOption loc}':" value;
             |       ^
         1086|       inherit (res.defsFinal') highestPrio;

       … while evaluating the option `system.build.toplevel':

       … while evaluating definitions from `/nix/store/4danq4f7rxpsax7bx18709c533v9krii-source/nixos/modules/system/activation/top-level.nix':

       … while evaluating the option `warnings':

       … while evaluating definitions from `/nix/store/4danq4f7rxpsax7bx18709c533v9krii-source/nixos/modules/system/boot/systemd/tmpfiles.nix':

       … while evaluating the option `systemd.tmpfiles.settings.graphics-driver."/run/opengl-driver-32"."L+".argument':

       … while evaluating definitions from `/nix/store/4danq4f7rxpsax7bx18709c533v9krii-source/nixos/modules/hardware/graphics.nix':

       (stack trace truncated; use '--show-trace' to show the full, detailed trace)

       error: store path 'https://github.com/sphinx-doc/alabaster/archive/refs/tags/1.0.0.tar.gz' contains illegal base-32 character 't'

glittershark · 2025-11-03T18:26:16Z

I would really not be surprised if this issue is just UB - the original implementation was pretty messy, and I never got around to combing through it to find all the ways I was holding C++ wrong.

dpulls · 2025-11-03T18:44:04Z

🎉 All dependencies have been resolved !

xokdvium · 2025-11-03T19:21:47Z

src/libexpr/include/nix/expr/value.hh

+        constexpr operator const StringData &() const &
+        {
+            return *std::bit_cast<const StringData *>(this);
+        }


This is very much UB. We need a better approach for static strings. Doing it in a well-defined manner with FAMs with constexpr isn't going to be possible.

yeah, static strings are by far the hardest part of this change.

I am surprised this is UB. I would think that casting between types that have compatible layouts is not UB.

(if bit_cast is the wrong type of cast, sure, but I thought there was some way to cast along these lines.)

Technically you would be able to do a reinterpret_cast if flexible array members were a standard thing and they would form a common initial sequence. char m_data[] is by all means non-standard but is just widely supported as a language extension. It's been added in C99, which was after C++98. I'm pretty sure the official status of whether this is well-formed is "YOLO".

Honestly that might be the best bet. It's certainly in the realm of non-standard language extensions but that should work out ok in practice at least for clang and gcc.

what might be our best bet? changing std::bit_cast to reinterpret_cast does not solve the problem.

I moved Static to another file for less bad recompilation hits, and I added some static casts that I hope make for a safer YOLO :).

Radvendii · 2025-11-03T19:52:04Z

src/libexpr/include/nix/expr/symbol-table.hh

    bool empty() const noexcept
    {
-        return s->size_ == 0;
+        auto * p = &s->string_data();
+        // Save a dereference in the sentinal value case
+        if (p == &""_sds)
+            return true;
+        return p->size() == 0;
    }

    [[gnu::always_inline]]
    size_t size() const noexcept
    {
-        return s->size_;
+        return s->string_data().size();
    }


Does this help us avoid more dereferences?

Suggested change

bool empty() const noexcept

{

return s->size_ == 0;

auto * p = &s->string_data();

// Save a dereference in the sentinal value case

if (p == &""_sds)

return true;

return p->size() == 0;

}

[[gnu::always_inline]]

size_t size() const noexcept

{

return s->size_;

return s->string_data().size();

}

bool empty() const noexcept

{

return size() == 0;

}

[[gnu::always_inline]]

size_t size() const noexcept

{

auto * p = &s->string_data();

// Save a dereference in the sentinal value case

if (p == &""_sds)

return 0;

return p->size();

}

Radvendii · 2025-11-03T20:22:40Z

src/libexpr/eval.cc

-// When there's no need to write to the string, we can optimize away empty
-// string allocations.
-// This function handles makeImmutableString(std::string_view()) by returning
-// the empty string.
-static const char * makeImmutableString(std::string_view s)
+StringData & StringData::make(std::string_view s)
 {
-    const size_t size = s.size();
-    if (size == 0)
-        return "";
-    auto t = allocString(size + 1);
-    memcpy(t, s.data(), size);
-    t[size] = '\0';
-    return t;
+    auto & res = alloc(s.size());
+    memcpy(&res.m_data, s.data(), s.size());
+    res.m_data[s.size()] = '\0';
+    return res;
 }


We can still handle the empty string by returning ""_sds, right?

Radvendii · 2025-11-03T22:38:03Z

This is what I've found:

the problem has to do with invalid context strings.
it seems to be getting set to another string, rather than turning into a nullpointer or something.
that context string is not getting set to the invalid state when it's initialized via mkPath
the problem goes away when the GC is disabled

Radvendii · 2025-11-03T22:43:03Z

Oh! Could it be that boehm is confused because we're holding onto pointers but they don't point at the beginning of the allocation?

So then it concludes that there's no reference to it and frees it?

hmm... presumably boehm is smarter than that...

Radvendii · 2025-11-03T23:31:33Z

Well well well, boehm is that stupid.

I don't think I can push to the branch because I'm not on the nix team, but here's my diff that solves the problem for me

commit 4364dd6d3170471966ec805073a027e8e38d0b92
Author: Taeer Bar-Yam <[email protected]>
Date:   Tue Nov 4 00:25:09 2025 +0100

    store StringData in context

diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc
index de74d2143..5d8e031e4 100644
--- a/src/libexpr/eval-cache.cc
+++ b/src/libexpr/eval-cache.cc
@@ -136,17 +136,17 @@ struct AttrDb
         });
     }
 
-    AttrId setString(AttrKey key, std::string_view s, const char ** context = nullptr)
+    AttrId setString(AttrKey key, std::string_view s, const StringData ** context = nullptr)
     {
         return doSQLite([&]() {
             auto state(_state->lock());
 
             if (context) {
                 std::string ctx;
-                for (const char ** p = context; *p; ++p) {
+                for (const StringData ** p = context; *p; ++p) {
                     if (p != context)
                         ctx.push_back(' ');
-                    ctx.append(*p);
+                    ctx.append((*p)->view());
                 }
                 state->insertAttributeWithContext.use()(key.first)(symbols[key.second])(AttrType::String) (s) (ctx)
                     .exec();
diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc
index 392022e39..428f52d46 100644
--- a/src/libexpr/eval.cc
+++ b/src/libexpr/eval.cc
@@ -818,13 +818,13 @@ void Value::mkString(std::string_view s)
     mkStringNoCopy(StringData::make(s));
 }
 
-static const char ** encodeContext(const NixStringContext & context)
+static const StringData ** encodeContext(const NixStringContext & context)
 {
     if (!context.empty()) {
         size_t n = 0;
-        auto ctx = (const char **) allocBytes((context.size() + 1) * sizeof(char *));
+        auto ctx = (const StringData **) allocBytes((context.size() + 1) * sizeof(StringData *));
         for (auto & i : context) {
-            ctx[n++] = StringData::make({i.to_string()}).view().data();
+            ctx[n++] = &StringData::make({i.to_string()});
         }
         ctx[n] = nullptr;
         return ctx;
@@ -2288,8 +2288,8 @@ std::string_view EvalState::forceString(Value & v, const PosIdx pos, std::string
 void copyContext(const Value & v, NixStringContext & context, const ExperimentalFeatureSettings & xpSettings)
 {
     if (v.context())
-        for (const char ** p = v.context(); *p; ++p)
-            context.insert(NixStringContextElem::parse(*p, xpSettings));
+        for (const StringData ** p = v.context(); *p; ++p)
+            context.insert(NixStringContextElem::parse((*p)->view().data(), xpSettings));
 }
 
 std::string_view EvalState::forceString(
diff --git a/src/libexpr/include/nix/expr/value.hh b/src/libexpr/include/nix/expr/value.hh
index aa275eda2..3dee1be02 100644
--- a/src/libexpr/include/nix/expr/value.hh
+++ b/src/libexpr/include/nix/expr/value.hh
@@ -264,7 +264,7 @@ public:
 
         constexpr operator const StringData &() const &
         {
-            return *std::bit_cast<const StringData *>(this);
+            return *reinterpret_cast<const StringData *>(this);
         }
 
         constexpr operator std::string_view() const &
@@ -335,7 +335,7 @@ struct ValueBase
     struct StringWithContext
     {
         const StringData * str;
-        const char ** context; // must be in sorted order
+        const StringData ** context; // must be in sorted order
     };
 
     struct Path
@@ -1106,7 +1106,7 @@ public:
         setStorage(b);
     }
 
-    void mkStringNoCopy(const StringData & s, const char ** context = 0) noexcept
+    void mkStringNoCopy(const StringData & s, const StringData ** context = nullptr) noexcept
     {
         setStorage(StringWithContext{.str = &s, .context = context});
     }
@@ -1234,7 +1234,7 @@ public:
         return string_data().view();
     }
 
-    const char ** context() const noexcept
+    const StringData ** context() const noexcept
     {
         return getStorage<StringWithContext>().context;
     }

This will make it easier to change this type in the future. See new TODO on naming. The thing we already so-named is a builder type for string contexts, not the on-heap type. Co-Authored-By: Taeer Bar-Yam <[email protected]

Replace the null-terminated C-style strings in Value with hybrid C / Pascal strings, where the length is stored in the allocation before the data, and there is still a null byte at the end for the sake of C interopt. Co-Authored-By: Taeer Bar-Yam <[email protected]>

Ericson2314 · 2025-11-04T00:26:46Z

Applied the fix, and factored out a new prep PR of #14470 to keep this smaller.

glittershark · 2025-11-04T15:27:42Z

boehm delenda est

xokdvium · 2025-11-04T16:19:12Z

So then it concludes that there's no reference to it and frees it?

Well, it's not suprising consdering that we have interior pointer detection only for the first 8 bytes and only on 64 bit systems to make that work with low-bit tagging.

Radvendii · 2025-11-04T23:14:29Z

boehm delenda est

This might be my next project.

to make that work with low-bit tagging.

I was wondering how that worked!

github-actions bot added new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority c api Nix as a C library with a stable interface labels Nov 1, 2025

Ericson2314 force-pushed the pascal-strings branch 2 times, most recently from dda5c2f to 15a9f93 Compare November 1, 2025 19:03

Ericson2314 force-pushed the pascal-strings branch from 15a9f93 to 6f82634 Compare November 1, 2025 19:10

Ericson2314 changed the title ~~Pascal strings~~ Use hybrid C / Pascal strings in the evaluator Nov 1, 2025

xokdvium self-assigned this Nov 1, 2025

Ericson2314 force-pushed the pascal-strings branch 2 times, most recently from d5a1e45 to f89ef40 Compare November 1, 2025 20:18

This was referenced Nov 1, 2025

Inline only-used-once closures in ExprConcatStrings::eval #14443

Merged

Use less c_str() in the evaluator, and other cleanups #14444

Merged

Ericson2314 force-pushed the pascal-strings branch 2 times, most recently from 91905f3 to 2f15843 Compare November 1, 2025 21:41

Ericson2314 force-pushed the pascal-strings branch from 2f15843 to 8e7cd91 Compare November 2, 2025 04:42

xokdvium requested changes Nov 3, 2025

View reviewed changes

Radvendii reviewed Nov 3, 2025

View reviewed changes

Ericson2314 force-pushed the pascal-strings branch from 8e7cd91 to ea6ba08 Compare November 3, 2025 23:05

Ericson2314 and others added 2 commits November 3, 2025 19:23

Factor out type alias for string contexts on the heap

f029b14

This will make it easier to change this type in the future. See new TODO on naming. The thing we already so-named is a builder type for string contexts, not the on-heap type. Co-Authored-By: Taeer Bar-Yam <[email protected]

Ericson2314 force-pushed the pascal-strings branch from ea6ba08 to 874640c Compare November 4, 2025 00:24

Ericson2314 marked this pull request as ready for review November 4, 2025 17:04

Ericson2314 requested a review from edolstra as a code owner November 4, 2025 17:04

Uh oh!

Use hybrid C / Pascal strings in the evaluator #14442

Are you sure you want to change the base?

Use hybrid C / Pascal strings in the evaluator #14442

Uh oh!

Conversation

glittershark commented Nov 1, 2025 • edited by Ericson2314 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Context

Uh oh!

Ericson2314 commented Nov 1, 2025

Uh oh!

Ericson2314 commented Nov 1, 2025

Uh oh!

xokdvium commented Nov 1, 2025

Uh oh!

Ericson2314 commented Nov 1, 2025

Uh oh!

edolstra commented Nov 3, 2025

Uh oh!

glittershark commented Nov 3, 2025

Uh oh!

Ericson2314 commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Radvendii commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Radvendii commented Nov 3, 2025

Uh oh!

glittershark commented Nov 3, 2025

Uh oh!

dpulls bot commented Nov 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xokdvium Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Radvendii commented Nov 3, 2025

Uh oh!

Radvendii commented Nov 3, 2025

Uh oh!

Radvendii commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ericson2314 commented Nov 4, 2025

Uh oh!

glittershark commented Nov 4, 2025

Uh oh!

xokdvium commented Nov 4, 2025

Uh oh!

Radvendii commented Nov 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

glittershark commented Nov 1, 2025 •

edited by Ericson2314

Loading

Ericson2314 commented Nov 3, 2025 •

edited

Loading

Radvendii commented Nov 3, 2025 •

edited

Loading

xokdvium Nov 3, 2025 •

edited

Loading

Radvendii commented Nov 3, 2025 •

edited

Loading