bench: add benchmarks to show off string perf improvements

Change-Id: I6d032706e1e8b6fa39502276ac2018cb69de6ce5

bench/bench.sh

@@ -6,7 +6,13 @@ shopt -s inherit_errexit
 scriptdir=$(cd "$(dirname -- "$0")" ; pwd -P)
 cd "$scriptdir/.."
 
-if [[ $# -lt 2 ]]; then
+benchSepIx=0
+for arg in "${@}"; do
+    if [[ "$arg" == "--" ]]; then break; fi
+    benchSepIx=$((benchSepIx+1))
+done
+
+if [[ $# -lt 2 ]] || [[ $benchSepIx -lt 2 ]]; then
     # FIXME(jade): it is a reasonable use case to want to run a benchmark run
     # on just one build. However, since we are using hyperfine in comparison
     # mode, we would have to combine the JSON ourselves to support that, which
@@ -14,7 +20,7 @@ if [[ $# -lt 2 ]]; then
     # not-bash.
     echo "Fewer than two result dirs given, nothing to compare!" >&2
     echo "Pass some directories (with names indicating which alternative they are) with bin/nix in them" >&2
-    echo "Usage: ./bench/bench.sh result-1 result-2 [result-3...]" >&2
+    echo "Usage: ./bench/bench.sh result-1 result-2 [result-3...] [--] [bench-1...]" >&2
     exit 1
 fi
 
@@ -28,7 +34,8 @@ export NIX_REMOTE="$(mktemp -d)"
 _exit='rm -rfv "$NIX_REMOTE"; $_exit'
 export NIX_PATH="nixpkgs=bench/nixpkgs:nixos-config=bench/configuration.nix"
 
-builds=("$@")
+builds=("${@:1:$benchSepIx}")
+benchsFromArgs=("${@:$((benchSepIx+2))}")
 
 flake_args="--extra-experimental-features 'nix-command flakes'"
 
@@ -43,15 +50,22 @@ cases=(
     [rebuild]="{BUILD}/bin/nix $flake_args eval --raw --impure --expr 'with import <nixpkgs/nixos> {}; system'"
     [rebuild-lh]="GC_INITIAL_HEAP_SIZE=10g {BUILD}/bin/nix eval $flake_args --raw --impure --expr 'with import <nixpkgs/nixos> {}; system'"
     [parse]="{BUILD}/bin/nix $flake_args eval -f bench/nixpkgs/pkgs/development/haskell-modules/hackage-packages.nix"
+    [substr100k]="{BUILD}/bin/nix eval --no-eval-cache -f bench/substring.nix --apply 'f: f.bench 100000'"
+    [substr200k]="{BUILD}/bin/nix eval --no-eval-cache -f bench/substring.nix --apply 'f: f.bench 200000'"
+    [substr300k]="{BUILD}/bin/nix eval --no-eval-cache -f bench/substring.nix --apply 'f: f.bench 300000'"
+    [substr400k]="{BUILD}/bin/nix eval --no-eval-cache -f bench/substring.nix --apply 'f: f.bench 400000'"
 )
 
-benches=(
+defaultBenches=(
     rebuild
     rebuild-lh
     search
     parse
 )
 
+
+benches=("${benchsFromArgs[@]:-${defaultBenches[@]}}")
+
 for k in "${benches[@]}"; do
     taskset -c 2,3 \
         chrt -f 50 \

bench/substring.nix

@@ -0,0 +1,19 @@
+# Tests stringLength and substring performance
+with builtins;
+{
+  bench =
+    baseStrLen:
+    let
+      # a big string
+      baseStr = concatStringsSep "" (genList (_: "x") baseStrLen);
+      # a way to force the values
+      sum = ns: foldl' (acc: n: acc + n) 0 ns;
+      forceOpTimes =
+        range: # Int
+        op: # Int -> Int
+        sum (genList (ix: op ix) range);
+
+      benchOp = ix: stringLength (substring ix 1 baseStr);
+    in
+    forceOpTimes baseStrLen benchOp;
+}

src/libcmd/repl.cc

@@ -674,7 +674,7 @@ ProcessLineResult NixRepl::processLine(std::string line)
         Value v;
         evalString(arg, v);
         if (v.type() == nString) {
-            std::cout << v.string.s;
+            std::cout << v.c_str();
         } else {
             printValue(std::cout, v);
         }

src/libexpr/eval-cache.cc

@@ -440,8 +440,8 @@ Value & AttrCursor::forceValue()
 
     if (root->db && (!cachedValue || std::get_if<placeholder_t>(&cachedValue->second))) {
         if (v.type() == nString)
-            cachedValue = {root->db->setString(getKey(), v.string.s, v.string.context),
-                           string_t{v.string.s, {}}};
+            cachedValue = {root->db->setString(getKey(), v.c_str(), v.stringContext()),
+                           string_t{v.c_str(), {}}};
         else if (v.type() == nPath) {
             auto path = v.path().path;
             cachedValue = {root->db->setString(getKey(), path.abs()), string_t{path.abs(), {}}};
@@ -580,7 +580,7 @@ std::string AttrCursor::getString()
         root->state.error<TypeError>("'%s' is not a string but %s", getAttrPathStr(), v.type()).debugThrow();
     }
 
-    return v.type() == nString ? v.string.s : v.path().to_string();
+    return v.type() == nString ? v.c_str() : v.path().to_string();
 }
 
 string_t AttrCursor::getStringWithContext()
@@ -622,7 +622,7 @@ string_t AttrCursor::getStringWithContext()
     if (v.type() == nString) {
         NixStringContext context;
         copyContext(v, context);
-        return {v.string.s, std::move(context)};
+        return {v.c_str(), std::move(context)};
     } else if (v.type() == nPath) {
         return {v.path().to_string(), {}};
     } else {

src/libexpr/eval.cc

@@ -94,6 +94,14 @@ static const char * makeImmutableString(std::string_view s)
     return t;
 }
 
+static Value::StringMeta * makeStringMeta(size_t size, const char * * context)
+{
+    auto meta = (Value::StringMeta *) allocBytes(sizeof(Value::StringMeta));
+    meta->size = size;
+    meta->context = context;
+    return meta;
+}
+
 
 RootValue allocRootValue(Value * v)
 {
@@ -157,7 +165,10 @@ std::string showType(const Value & v)
     #pragma GCC diagnostic push
     #pragma GCC diagnostic ignored "-Wswitch-enum"
     switch (v.internalType) {
-        case tString: return v.string.context ? "a string with context" : "a string";
+        case tStringEmpty: return v.stringContext() ? "a string with context" : "a string";
+        case tStringUnknownSize: return "a string";
+        case tStringKnownSize: return "a string";
+        case tStringWithContext: return "a string with context";
         case tPrimOp:
             return fmt("the built-in function '%s'", std::string(v.primOp->name));
         case tPrimOpApp:
@@ -253,7 +264,7 @@ static Symbol getName(const AttrName & name, EvalState & state, Env & env)
         Value nameValue;
         name.expr->eval(state, env, nameValue);
         state.forceStringNoCtx(nameValue, name.expr->getPos(), "while evaluating an attribute name");
-        return state.symbols.create(nameValue.string.s);
+        return state.symbols.create(nameValue.str());
     }
 }
 
@@ -877,34 +888,41 @@ DebugTraceStacker::DebugTraceStacker(EvalState & evalState, DebugTrace t)
         evalState.runDebugRepl(nullptr, trace.env, trace.expr);
 }
 
-void Value::mkString(std::string_view s)
+void Value::mkString(const char * s, size_t size, const char * * context)
 {
-    mkString(makeImmutableString(s));
+
+    if (size == 0) {
+        mkEmptyString(context);
+    } else if (! context) {
+        mkStringKnownSize(s, size);
+    } else {
+        mkStringWithContext(s, makeStringMeta(size, context));
+    }
 }
 
-
-static void copyContextToValue(Value & v, const NixStringContext & context)
+void Value::mkString(std::string_view s)
 {
-    if (!context.empty()) {
-        size_t n = 0;
-        v.string.context = (const char * *)
-            allocBytes((context.size() + 1) * sizeof(char *));
-        for (auto & i : context)
-            v.string.context[n++] = dupString(i.to_string().c_str());
-        v.string.context[n] = 0;
-    }
+    mkString(makeImmutableString(s), s.size());
 }
 
 void Value::mkString(std::string_view s, const NixStringContext & context)
 {
-    mkString(s);
-    copyContextToValue(*this, context);
+    mkStringMove(makeImmutableString(s), s.size(), context);
 }
 
-void Value::mkStringMove(const char * s, const NixStringContext & context)
+void Value::mkStringMove(const char * s, size_t size, const NixStringContext & context)
 {
-    mkString(s);
-    copyContextToValue(*this, context);
+    if (context.empty())
+        mkString(s, size);
+    else {
+        auto vContext = (const char * *) allocBytes((context.size() + 1) * sizeof(char *));
+        mkString(s, size, vContext);
+
+        size_t n = 0;
+        for (auto & i : context)
+            vContext[n++] = dupString(i.to_string().c_str());
+        vContext[n] = 0;
+    }
 }
 
 
@@ -1330,7 +1348,7 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v)
         if (nameVal.type() == nNull)
             continue;
         state.forceStringNoCtx(nameVal, i.pos, "while evaluating the name of a dynamic attribute");
-        auto nameSym = state.symbols.create(nameVal.string.s);
+        auto nameSym = state.symbols.create(nameVal.str());
         Bindings::iterator j = v.attrs->find(nameSym);
         if (j != v.attrs->end())
             state.error<EvalError>("dynamic attribute '%1%' already defined at %2%", state.symbols[nameSym], state.positions[j->pos]).atPos(i.pos).withFrame(env, *this).debugThrow();
@@ -2091,8 +2109,9 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
         if (!context.empty())
             state.error<EvalError>("a string that refers to a store path cannot be appended to a path").atPos(pos).withFrame(env, *this).debugThrow();
         v.mkPath(CanonPath(canonPath(str())));
-    } else
-        v.mkStringMove(c_str(), context);
+    } else {
+        v.mkStringMove(c_str(), sSize, context);
+    }
 }
 
 
@@ -2256,7 +2275,7 @@ std::string_view EvalState::forceString(Value & v, const PosIdx pos, std::string
                 showType(v),
                 ValuePrinter(*this, v, errorPrintOptions)
             ).atPos(pos).debugThrow();
-        return v.string.s;
+        return v.str();
     } catch (Error & e) {
         e.addTrace(positions[pos], errorCtx);
         throw;
@@ -2266,8 +2285,8 @@ std::string_view EvalState::forceString(Value & v, const PosIdx pos, std::string
 
 void copyContext(const Value & v, NixStringContext & context)
 {
-    if (v.string.context)
-        for (const char * * p = v.string.context; *p; ++p)
+    if (v.stringContext())
+        for (const char * * p = v.stringContext(); *p; ++p)
             context.insert(NixStringContextElem::parse(*p));
 }
 
@@ -2283,8 +2302,8 @@ std::string_view EvalState::forceString(Value & v, NixStringContext & context, c
 std::string_view EvalState::forceStringNoCtx(Value & v, const PosIdx pos, std::string_view errorCtx)
 {
     auto s = forceString(v, pos, errorCtx);
-    if (v.string.context) {
-        error<EvalError>("the string '%1%' is not allowed to refer to a store path (such as '%2%')", v.string.s, v.string.context[0]).withTrace(pos, errorCtx).debugThrow();
+    if (v.stringContext()) {
+        error<EvalError>("the string '%1%' is not allowed to refer to a store path (such as '%2%')", v.c_str(), v.stringContext()[0]).withTrace(pos, errorCtx).debugThrow();
     }
     return s;
 }
@@ -2297,7 +2316,7 @@ bool EvalState::isDerivation(Value & v)
     if (i == v.attrs->end()) return false;
     forceValue(*i->value, i->pos);
     if (i->value->type() != nString) return false;
-    return strcmp(i->value->string.s, "derivation") == 0;
+    return strcmp(i->value->c_str(), "derivation") == 0;
 }
 
 
@@ -2329,7 +2348,7 @@ BackedStringView EvalState::coerceToString(
 
     if (v.type() == nString) {
         copyContext(v, context);
-        return std::string_view(v.string.s);
+        return v.str();
     }
 
     if (v.type() == nPath) {
@@ -2534,7 +2553,7 @@ bool EvalState::eqValues(Value & v1, Value & v2, const PosIdx pos, std::string_v
             return v1.boolean == v2.boolean;
 
         case nString:
-            return strcmp(v1.string.s, v2.string.s) == 0;
+            return strcmp(v1.c_str(), v2.c_str()) == 0;
 
         case nPath:
             return strcmp(v1._path, v2._path) == 0;

src/libexpr/flake/flake.cc

@@ -113,7 +113,7 @@ static FlakeInput parseFlakeInput(EvalState & state,
         try {
             if (attr.name == sUrl) {
                 expectType(state, nString, *attr.value, attr.pos);
-                url = attr.value->string.s;
+                url = attr.value->c_str();
                 attrs.emplace("url", *url);
             } else if (attr.name == sFlake) {
                 expectType(state, nBool, *attr.value, attr.pos);
@@ -122,7 +122,7 @@ static FlakeInput parseFlakeInput(EvalState & state,
                 input.overrides = parseFlakeInputs(state, attr.value, attr.pos, baseDir, lockRootPath, depth + 1);
             } else if (attr.name == sFollows) {
                 expectType(state, nString, *attr.value, attr.pos);
-                auto follows(parseInputPath(attr.value->string.s));
+                auto follows(parseInputPath(attr.value->str()));
                 follows.insert(follows.begin(), lockRootPath.begin(), lockRootPath.end());
                 input.follows = follows;
             } else {
@@ -131,7 +131,7 @@ static FlakeInput parseFlakeInput(EvalState & state,
                 #pragma GCC diagnostic ignored "-Wswitch-enum"
                 switch (attr.value->type()) {
                     case nString:
-                        attrs.emplace(state.symbols[attr.name], attr.value->string.s);
+                        attrs.emplace(state.symbols[attr.name], attr.value->c_str());
                         break;
                     case nBool:
                         attrs.emplace(state.symbols[attr.name], Explicit<bool> { attr.value->boolean });
@@ -238,7 +238,7 @@ static Flake getFlake(
 
     if (auto description = vInfo.attrs->get(state.sDescription)) {
         expectType(state, nString, *description->value, description->pos);
-        flake.description = description->value->string.s;
+        flake.description = description->value->c_str();
     }
 
     auto sInputs = state.symbols.create("inputs");

src/libexpr/get-drvs.cc

@@ -156,7 +156,7 @@ DrvInfo::Outputs DrvInfo::queryOutputs(bool withPaths, bool onlyOutputsToInstall
         Outputs result;
         for (auto elem : outTI->listItems()) {
             if (elem->type() != nString) throw errMsg;
-            auto out = outputs.find(elem->string.s);
+            auto out = outputs.find(elem->c_str());
             if (out == outputs.end()) throw errMsg;
             result.insert(*out);
         }
@@ -230,7 +230,7 @@ std::string DrvInfo::queryMetaString(const std::string & name)
 {
     Value * v = queryMeta(name);
     if (!v || v->type() != nString) return "";
-    return v->string.s;
+    return v->c_str();
 }
 
 
@@ -242,7 +242,7 @@ NixInt DrvInfo::queryMetaInt(const std::string & name, NixInt def)
     if (v->type() == nString) {
         /* Backwards compatibility with before we had support for
            integer meta fields. */
-        if (auto n = string2Int<NixInt>(v->string.s))
+        if (auto n = string2Int<NixInt>(v->c_str()))
             return *n;
     }
     return def;
@@ -256,7 +256,7 @@ NixFloat DrvInfo::queryMetaFloat(const std::string & name, NixFloat def)
     if (v->type() == nString) {
         /* Backwards compatibility with before we had support for
            float meta fields. */
-        if (auto n = string2Float<NixFloat>(v->string.s))
+        if (auto n = string2Float<NixFloat>(v->c_str()))
             return *n;
     }
     return def;
@@ -271,8 +271,8 @@ bool DrvInfo::queryMetaBool(const std::string & name, bool def)
     if (v->type() == nString) {
         /* Backwards compatibility with before we had support for
            Boolean meta fields. */
-        if (strcmp(v->string.s, "true") == 0) return true;
-        if (strcmp(v->string.s, "false") == 0) return false;
+        if (strcmp(v->c_str(), "true") == 0) return true;
+        if (strcmp(v->c_str(), "false") == 0) return false;
     }
     return def;
 }

src/libexpr/primops.cc

@@ -594,7 +594,7 @@ struct CompareValues
                 case nFloat:
                     return v1->fpoint < v2->fpoint;
                 case nString:
-                    return strcmp(v1->string.s, v2->string.s) < 0;
+                    return strcmp(v1->c_str(), v2->c_str()) < 0;
                 case nPath:
                     return strcmp(v1->_path, v2->_path) < 0;
                 case nList:
@@ -977,7 +977,7 @@ static void prim_trace(EvalState & state, const PosIdx pos, Value * * args, Valu
 {
     state.forceValue(*args[0], pos);
     if (args[0]->type() == nString)
-        printError("trace: %1%", args[0]->string.s);
+        printError("trace: %1%", args[0]->c_str());
     else
         printError("trace: %1%", ValuePrinter(state, *args[0]));
     if (evalSettings.builtinsTraceDebugger && state.debugRepl && !state.debugTraces.empty()) {
@@ -2400,7 +2400,7 @@ static void prim_attrNames(EvalState & state, const PosIdx pos, Value * * args,
         (v.listElems()[n++] = state.allocValue())->mkString(state.symbols[i.name]);
 
     std::sort(v.listElems(), v.listElems() + n,
-              [](Value * v1, Value * v2) { return strcmp(v1->string.s, v2->string.s) < 0; });
+              [](Value * v1, Value * v2) { return strcmp(v1->c_str(), v2->c_str()) < 0; });
 }
 
 static RegisterPrimOp primop_attrNames({
@@ -2590,7 +2590,7 @@ static void prim_removeAttrs(EvalState & state, const PosIdx pos, Value * * args
     names.reserve(args[1]->listSize());
     for (auto elem : args[1]->listItems()) {
         state.forceStringNoCtx(*elem, pos, "while evaluating the values of the second argument passed to builtins.removeAttrs");
-        names.emplace_back(state.symbols.create(elem->string.s), nullptr);
+        names.emplace_back(state.symbols.create(elem->str()), nullptr);
     }
     std::sort(names.begin(), names.end());
 
@@ -3740,7 +3740,7 @@ static void prim_substring(EvalState & state, const PosIdx pos, Value * * args,
     if (len == 0) {
         state.forceValue(*args[2], pos);
         if (args[2]->type() == nString) {
-            v.mkString("", args[2]->string.context);
+            v.mkEmptyString(args[2]->stringContext());
             return;
         }
     }

src/libexpr/primops/fetchClosure.cc

@@ -133,7 +133,7 @@ static void prim_fetchClosure(EvalState & state, const PosIdx pos, Value * * arg
 
         else if (attrName == "toPath") {
             state.forceValue(*attr.value, attr.pos);
-            bool isEmptyString = attr.value->type() == nString && attr.value->string.s == std::string("");
+            bool isEmptyString = attr.value->type() == nString && attr.value->c_str() == std::string("");
             if (isEmptyString) {
                 toPath = StorePathOrGap {};
             }

src/libexpr/print-ambiguous.cc

@@ -28,7 +28,7 @@ void printAmbiguous(
         printLiteralBool(str, v.boolean);
         break;
     case nString:
-        escapeString(str, v.string.s);
+        escapeString(str, v.str());
         break;
     case nPath:
         str << v.path().to_string(); // !!! escaping?

src/libexpr/print.cc

@@ -200,7 +200,7 @@ private:
     {
         escapeString(
             output,
-            v.string.s,
+            v.str(),
             {
                 .maxLength = options.maxStringLength,
                 .outputAnsiColors = options.ansiColors,

src/libexpr/value-to-json.cc

@@ -32,7 +32,7 @@ json printValueAsJSON(EvalState & state, bool strict,
 
         case nString:
             copyContext(v, context);
-            out = v.string.s;
+            out = v.c_str();
             break;
 
         case nPath:

src/libexpr/value-to-xml.cc

@@ -75,7 +75,7 @@ static void printValueAsXML(EvalState & state, bool strict, bool location,
         case nString:
             /* !!! show the context? */
             copyContext(v, context);
-            doc.writeEmptyElement("string", singletonAttrs("value", v.string.s));
+            doc.writeEmptyElement("string", singletonAttrs("value", v.c_str()));
             break;
 
         case nPath:
@@ -97,14 +97,14 @@ static void printValueAsXML(EvalState & state, bool strict, bool location,
                 if (a != v.attrs->end()) {
                     if (strict) state.forceValue(*a->value, a->pos);
                     if (a->value->type() == nString)
-                        xmlAttrs["drvPath"] = drvPath = a->value->string.s;
+                        xmlAttrs["drvPath"] = drvPath = a->value->c_str();
                 }
 
                 a = v.attrs->find(state.sOutPath);
                 if (a != v.attrs->end()) {
                     if (strict) state.forceValue(*a->value, a->pos);
                     if (a->value->type() == nString)
-                        xmlAttrs["outPath"] = a->value->string.s;
+                        xmlAttrs["outPath"] = a->value->c_str();
                 }
 
                 XMLOpenElement _(doc, "derivation", xmlAttrs);

src/libexpr/value.hh

@@ -23,7 +23,10 @@ class BindingsBuilder;
 typedef enum {
     tInt = 1,
     tBool,
-    tString,
+    tStringEmpty,
+    tStringUnknownSize,
+    tStringKnownSize,
+    tStringWithContext,
     tPath,
     tNull,
     tAttrs,
@@ -159,6 +162,13 @@ public:
     inline bool isPrimOp() const { return internalType == tPrimOp; };
     inline bool isPrimOpApp() const { return internalType == tPrimOpApp; };
 
+    // Widening Value kills eval performace so we use an extra indirection
+    // to carry more metadata.
+    struct StringMeta {
+        size_t size;
+        const char * * context; // must be in sorted order, see note below
+    };
+
     union
     {
         NixInt integer;
@@ -186,10 +196,29 @@ public:
 
          * For canonicity, the store paths should be in sorted order.
          */
+
+        // When a string is empty we can store the context directly.
+        struct {
+            const char * * context;
+        } emptyString;
+
+        // When the context is empty, we can use the InternalType
+        // to be lazy about calculating the size of the string.
         struct {
             const char * s;
-            const char * * context; // must be in sorted order
-        } string;
+        } stringUnknownSize;
+        struct {
+            const char * s;
+            size_t size;
+        } stringKnownSize;
+
+        // We happen to always have a size available when we're
+        // constucting a string with context. If this changes
+        // use the same trick as for strings without context.
+        struct {
+            const char * s;
+            const StringMeta * meta;
+        } stringWithContext;
 
         const char * _path;
         Bindings * attrs;
@@ -229,7 +258,7 @@ public:
         switch (internalType) {
             case tInt: return nInt;
             case tBool: return nBool;
-            case tString: return nString;
+            case tStringEmpty: case tStringUnknownSize: case tStringKnownSize: case tStringWithContext: return nString;
             case tPath: return nPath;
             case tNull: return nNull;
             case tAttrs: return nAttrs;
@@ -268,22 +297,55 @@ public:
         boolean = b;
     }
 
-    inline void mkString(const char * s, const char * * context = 0)
+    inline void mkEmptyString(const char * * context = 0)
     {
-        internalType = tString;
-        string.s = s;
-        string.context = context;
+        clearValue();
+        internalType = tStringEmpty;
+        emptyString.context = context;
+    }
+
+    inline void mkStringUnknownSize(const char * s)
+    {
+        clearValue();
+        internalType = tStringUnknownSize;
+        stringUnknownSize.s = s;
+    }
+
+    inline void mkStringKnownSize(const char * s, size_t size)
+    {
+        internalType = tStringKnownSize;
+        stringKnownSize.s = s;
+        stringKnownSize.size = size;
+    }
+
+    inline void mkStringWithContext(const char * s, StringMeta * meta)
+    {
+        internalType = tStringWithContext;
+        stringWithContext.s = s;
+        stringWithContext.meta = meta;
+    }
+
+    void mkString(const char * s, size_t size, const char * * context = 0);
+
+    // Don't change this proto. You'll get upcast to string_view and kill the gc.
+    inline void mkString(const char * s)
+    {
+        if (s[0] == '\0')
+            mkEmptyString();
+        else
+            mkStringUnknownSize(s);
     }
 
     void mkString(std::string_view s);
 
     void mkString(std::string_view s, const NixStringContext & context);
 
-    void mkStringMove(const char * s, const NixStringContext & context);
+    void mkStringMove(const char * s, size_t size, const NixStringContext & context);
 
-    inline void mkString(const Symbol & s)
+    inline void mkString(const Symbol & sym)
     {
-        mkString(((const std::string &) s).c_str());
+        auto s = (const std::string &) sym;
+        mkString(s.c_str(), s.size());
     }
 
     void mkPath(const SourcePath & path);
@@ -437,11 +499,46 @@ public:
         return SourcePath{CanonPath(_path)};
     }
 
-    std::string_view str() const
+// Allow selecting a subset of enum values
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wswitch-enum"
+
+    const char * c_str() const
     {
-        assert(internalType == tString);
-        return std::string_view(string.s);
+        switch(internalType) {
+            case tStringEmpty: return "";
+            case tStringUnknownSize: return stringUnknownSize.s;
+            case tStringKnownSize: return stringKnownSize.s;
+            case tStringWithContext: return stringWithContext.s;
+            default: abort();
+        }
     }
+
+    std::string_view str()
+    {
+        switch(internalType) {
+            case tStringEmpty: return std::string_view("");
+            case tStringUnknownSize:
+                mkStringKnownSize(stringUnknownSize.s, strlen(stringUnknownSize.s));
+                return std::string_view(stringKnownSize.s, stringKnownSize.size);
+            case tStringKnownSize: return std::string_view(stringKnownSize.s, stringKnownSize.size);
+            case tStringWithContext: return std::string_view(stringWithContext.s, stringWithContext.meta->size);
+            default: abort();
+        }
+    }
+
+    const char * * stringContext() const
+    {
+        switch(internalType) {
+            case tStringEmpty: return emptyString.context;
+            case tStringUnknownSize: return 0;
+            case tStringKnownSize: return 0;
+            case tStringWithContext: return stringWithContext.meta->context;
+            default: abort();
+        }
+    }
+
+#pragma GCC diagnostic pop
 };
 
 

src/nix-env/nix-env.cc

@@ -1233,7 +1233,7 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs)
                         else {
                             if (v->type() == nString) {
                                 attrs2["type"] = "string";
-                                attrs2["value"] = v->string.s;
+                                attrs2["value"] = v->c_str();
                                 xml.writeEmptyElement("meta", attrs2);
                             } else if (v->type() == nInt) {
                                 attrs2["type"] = "int";
@@ -1253,7 +1253,7 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs)
                                 for (auto elem : v->listItems()) {
                                     if (elem->type() != nString) continue;
                                     XMLAttrs attrs3;
-                                    attrs3["value"] = elem->string.s;
+                                    attrs3["value"] = elem->c_str();
                                     xml.writeEmptyElement("string", attrs3);
                                 }
                             } else if (v->type() == nAttrs) {
@@ -1265,7 +1265,7 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs)
                                     if(a.value->type() != nString) continue;
                                     XMLAttrs attrs3;
                                     attrs3["type"] = globals.state->symbols[i.name];
-                                    attrs3["value"] = a.value->string.s;
+                                    attrs3["value"] = a.value->c_str();
                                     xml.writeEmptyElement("string", attrs3);
                             }
                             }

src/nix/eval.cc

@@ -86,7 +86,7 @@ struct CmdEval : MixJSON, InstallableValueCommand, MixReadOnlyOption
                 state->forceValue(v, pos);
                 if (v.type() == nString)
                     // FIXME: disallow strings with contexts?
-                    writeFile(path, v.string.s);
+                    writeFile(path, v.c_str());
                 else if (v.type() == nAttrs) {
                     if (mkdir(path.c_str(), 0777) == -1)
                         throw SysError("creating directory '%s'", path);

tests/unit/libexpr-support/tests/libexpr.hh

@@ -71,7 +71,7 @@ namespace nix {
         if (arg.type() != nString) {
             return false;
         }
-        return std::string_view(arg.string.s) == std::string_view(s);
+        return std::string_view(arg.c_str()) == std::string_view(s);
     }
 
     MATCHER_P(IsIntEq, v, fmt("The string is equal to \"%1%\"", v)) {
@@ -107,7 +107,7 @@ namespace nix {
                 *result_listener << "Expected a path got " << arg.type();
                 return false;
             } else if (std::string_view(arg._path) != p) {
-                *result_listener << "Expected a path that equals \"" << p << "\" but got: " << arg.string.s;
+                *result_listener << "Expected a path that equals \"" << p << "\" but got: " << arg.c_str();
                 return false;
             }
             return true;

tests/unit/libexpr/primops.cc

@@ -713,14 +713,14 @@ namespace nix {
         // FIXME: add a test that verifies the string context is as expected
         auto v = eval("builtins.replaceStrings [\"oo\" \"a\"] [\"a\" \"i\"] \"foobar\"");
         ASSERT_EQ(v.type(), nString);
-        ASSERT_EQ(v.string.s, std::string_view("fabir"));
+        ASSERT_EQ(v.c_str(), std::string_view("fabir"));
     }
 
     TEST_F(PrimOpTest, concatStringsSep) {
         // FIXME: add a test that verifies the string context is as expected
         auto v = eval("builtins.concatStringsSep \"%\" [\"foo\" \"bar\" \"baz\"]");
         ASSERT_EQ(v.type(), nString);
-        ASSERT_EQ(std::string_view(v.string.s), "foo%bar%baz");
+        ASSERT_EQ(std::string_view(v.c_str()), "foo%bar%baz");
     }
 
     TEST_F(PrimOpTest, split1) {