Store short strings in Values

The vast majority of strings are < 16 bytes, and so can be stored
directly in a Value. This saves a heap allocation and an indirection.

src/libexpr/eval.cc

@@ -51,9 +51,10 @@ static void printValue(std::ostream & str, std::set<const Value *> & active, con
     case tBool:
         str << (v.boolean ? "true" : "false");
         break;
-    case tString:
+    case tShortString:
+    case tLongString:
         str << "\"";
-        for (const char * i = v.string.s; *i; i++)
+        for (const char * i = v.getString(); *i; i++)
             if (*i == '\"' || *i == '\\') str << "\\" << *i;
             else if (*i == '\n') str << "\\n";
             else if (*i == '\r') str << "\\r";
@@ -140,7 +141,8 @@ string showType(const Value & v)
     switch (v.type) {
         case tInt: return "an integer";
         case tBool: return "a boolean";
-        case tString: return v.string.context ? "a string with context" : "a string";
+        case tShortString: return "a string";
+        case tLongString: return v.string.context ? "a string with context" : "a string";
         case tPath: return "a path";
         case tNull: return "null";
         case tAttrs: return "a set";
@@ -170,8 +172,7 @@ static Symbol getName(const AttrName & name, EvalState & state, Env & env)
     } else {
         Root<Value> nameValue;
         name.expr->eval(state, env, nameValue);
-        state.forceStringNoCtx(nameValue);
+        return state.symbols.create(state.forceStringNoCtx(nameValue));
-        return state.symbols.create(nameValue->string.s);
     }
 }
 
@@ -495,14 +496,23 @@ LocalNoInline(void addErrorPrefix(Error & e, const char * s, const string & s2,
 
 void mkString(Value & v, const char * s)
 {
-    mkStringNoCopy(v, dupString(s));
+    auto len = strlen(s); // FIXME: only need to know if > short
+    if (len < WORD_SIZE * 2) {
+        strcpy((char *) &v.string, s);
+        v.type = tShortString;
+    } else
+        mkStringNoCopy(v, dupString(s));
 }
 
 
 Value & mkString(Value & v, const string & s, const PathSet & context)
 {
-    mkString(v, s.c_str());
+    if (context.empty())
-    v.setContext(context);
+        mkString(v, s.c_str());
+    else {
+        mkStringNoCopy(v, dupString(s.c_str()));
+        v.setContext(context);
+    }
     return v;
 }
 
@@ -840,8 +850,7 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v)
         state.forceValue(nameVal, i.pos);
         if (nameVal->type == tNull)
             continue;
-        state.forceStringNoCtx(nameVal);
+        Symbol nameSym = state.symbols.create(state.forceStringNoCtx(nameVal));
-        Symbol nameSym = state.symbols.create(nameVal->string.s);
         Bindings::iterator j = v.attrs->find(nameSym);
         if (j != v.attrs->end())
             throwEvalError("dynamic attribute '%1%' at %2% already defined at %3%", nameSym, i.pos, *j->pos);
@@ -1313,7 +1322,7 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
     NixFloat nf = 0;
 
     bool first = !forceString;
-    Tag firstType = tString;
+    Tag firstType = tLongString;
 
     auto vTmp = state.allocValue();
 
@@ -1325,7 +1334,7 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
            since paths are copied when they are used in a derivation),
            and none of the strings are allowed to have contexts. */
         if (first) {
-            firstType = vTmp->type;
+            firstType = vTmp->isString() ? tLongString : vTmp->type;
             first = false;
         }
 
@@ -1347,7 +1356,7 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
             } else
                 throwEvalError("cannot add %1% to a float, at %2%", showType(vTmp), pos);
         } else
-            s << state.coerceToString(pos, vTmp, context, false, firstType == tString);
+            s << state.coerceToString(pos, vTmp, context, false, firstType == tLongString);
     }
 
     if (firstType == tInt)
@@ -1448,13 +1457,13 @@ void EvalState::forceFunction(Value & v, const Pos & pos)
 string EvalState::forceString(Value & v, const Pos & pos)
 {
     forceValue(v, pos);
-    if (v.type != tString) {
+    if (!v.isString()) {
         if (pos)
             throwTypeError("value is %1% while a string was expected, at %2%", v, pos);
         else
             throwTypeError("value is %1% while a string was expected", v);
     }
-    return string(v.string.s);
+    return string(v.getString()); // FIXME: don't copy
 }
 
 
@@ -1469,15 +1478,15 @@ string EvalState::forceString(Value & v, PathSet & context, const Pos & pos)
 string EvalState::forceStringNoCtx(Value & v, const Pos & pos)
 {
     string s = forceString(v, pos);
-    if (v.string.context) {
+    if (v.type == tLongString && v.string.context) {
         PathSet context;
         v.getContext(context);
         if (pos)
             throwEvalError("the string '%1%' is not allowed to refer to a store path (such as '%2%'), at %3%",
-                v.string.s, *context.begin(), pos);
+                v.string._s, *context.begin(), pos);
         else
             throwEvalError("the string '%1%' is not allowed to refer to a store path (such as '%2%')",
-                v.string.s, *context.begin());
+                v.string._s, *context.begin());
     }
     return s;
 }
@@ -1489,8 +1498,8 @@ bool EvalState::isDerivation(Value & v)
     Bindings::iterator i = v.attrs->find(sType);
     if (i == v.attrs->end()) return false;
     forceValue(*i->value);
-    if (i->value->type != tString) return false;
+    if (!i->value->isString()) return false;
-    return strcmp(i->value->string.s, "derivation") == 0;
+    return strcmp(i->value->getString(), "derivation") == 0;
 }
 
 
@@ -1501,9 +1510,9 @@ string EvalState::coerceToString(const Pos & pos, Value & v, PathSet & context,
 
     string s;
 
-    if (v.type == tString) {
+    if (v.isString()) {
         v.getContext(context);
-        return v.string.s;
+        return v.getString();
     }
 
     if (v.type == tPath) {
@@ -1603,6 +1612,9 @@ bool EvalState::eqValues(Value & v1, Value & v2)
     if (v1.type == tFloat && v2.type == tInt)
         return v1.fpoint == v2.integer;
 
+    if (v1.isString())
+        return v2.isString() && strcmp(v1.getString(), v2.getString()) == 0;
+
     // All other types are not compatible with each other.
     if (v1.type != v2.type) return false;
 
@@ -1614,9 +1626,6 @@ bool EvalState::eqValues(Value & v1, Value & v2)
         case tBool:
             return v1.boolean == v2.boolean;
 
-        case tString:
-            return strcmp(v1.string.s, v2.string.s) == 0;
-
         case tPath:
             return strcmp(v1.path, v2.path) == 0;
 
@@ -1799,8 +1808,8 @@ size_t valueSize(Value & v)
         size_t sz = sizeof(Value);
 
         switch (v.type) {
-        case tString:
+        case tLongString:
-            sz += doString(v.string.s);
+            sz += doString(v.string._s);
             break;
         case tPath:
             sz += doString(v.path);

src/libexpr/gc.cc

@@ -106,9 +106,10 @@ void GC::gc()
         case tNull:
         case tList0:
         case tFloat:
+        case tShortString:
             break;
 
-        case tString: {
+        case tLongString: {
             auto obj2 = (Value *) obj;
             // FIXME: GC string
             // See setContext().

src/libexpr/gc.hh

@@ -25,7 +25,8 @@ enum Tag {
     // Value tags
     tInt,
     tBool,
-    tString,
+    tShortString,
+    tLongString,
     tPath,
     tNull,
     tAttrs,

src/libexpr/get-drvs.cc

@@ -122,8 +122,8 @@ DrvInfo::Outputs DrvInfo::queryOutputs(bool onlyOutputsToInstall)
     if (!outTI->isList()) throw errMsg;
     Outputs result;
     for (auto i = outTI->listElems(); i != outTI->listElems() + outTI->listSize(); ++i) {
-        if ((*i)->type != tString) throw errMsg;
+        if (!(*i)->isString()) throw errMsg;
-        auto out = outputs.find((*i)->string.s);
+        auto out = outputs.find((*i)->getString());
         if (out == outputs.end()) throw errMsg;
         result.insert(*out);
     }
@@ -178,8 +178,7 @@ bool DrvInfo::checkMeta(Value & v)
             if (!checkMeta(*i.value)) return false;
         return true;
     }
-    else return v.type == tInt || v.type == tBool || v.type == tString ||
+    else return v.type == tInt || v.type == tBool || v.isString() || v.type == tFloat;
-                v.type == tFloat;
 }
 
 
@@ -195,8 +194,8 @@ Value * DrvInfo::queryMeta(const string & name)
 string DrvInfo::queryMetaString(const string & name)
 {
     auto v = queryMeta(name);
-    if (!v || v->type != tString) return "";
+    if (!v || !v->isString()) return "";
-    return v->string.s;
+    return v->getString();
 }
 
 
@@ -205,11 +204,11 @@ NixInt DrvInfo::queryMetaInt(const string & name, NixInt def)
     auto v = queryMeta(name);
     if (!v) return def;
     if (v->type == tInt) return v->integer;
-    if (v->type == tString) {
+    if (v->isString()) {
         /* Backwards compatibility with before we had support for
            integer meta fields. */
         NixInt n;
-        if (string2Int(v->string.s, n)) return n;
+        if (string2Int(v->getString(), n)) return n;
     }
     return def;
 }
@@ -219,11 +218,11 @@ NixFloat DrvInfo::queryMetaFloat(const string & name, NixFloat def)
     auto v = queryMeta(name);
     if (!v) return def;
     if (v->type == tFloat) return v->fpoint;
-    if (v->type == tString) {
+    if (v->isString()) {
         /* Backwards compatibility with before we had support for
            float meta fields. */
         NixFloat n;
-        if (string2Float(v->string.s, n)) return n;
+        if (string2Float(v->getString(), n)) return n;
     }
     return def;
 }
@@ -234,11 +233,12 @@ bool DrvInfo::queryMetaBool(const string & name, bool def)
     auto v = queryMeta(name);
     if (!v) return def;
     if (v->type == tBool) return v->boolean;
-    if (v->type == tString) {
+    if (v->isString()) {
         /* Backwards compatibility with before we had support for
            Boolean meta fields. */
-        if (strcmp(v->string.s, "true") == 0) return true;
+        auto s = v->getString();
-        if (strcmp(v->string.s, "false") == 0) return false;
+        if (strcmp(s, "true") == 0) return true;
+        if (strcmp(s, "false") == 0) return false;
     }
     return def;
 }

src/libexpr/primops.cc

@@ -238,7 +238,9 @@ static void prim_typeOf(EvalState & state, const Pos & pos, Value * * args, Valu
     switch (args[0]->type) {
         case tInt: t = "int"; break;
         case tBool: t = "bool"; break;
-        case tString: t = "string"; break;
+        case tShortString:
+        case tLongString:
+            t = "string"; break;
         case tPath: t = "path"; break;
         case tNull: t = "null"; break;
         case tAttrs: t = "set"; break;
@@ -305,7 +307,7 @@ static void prim_isFloat(EvalState & state, const Pos & pos, Value * * args, Val
 static void prim_isString(EvalState & state, const Pos & pos, Value * * args, Value & v)
 {
     state.forceValue(*args[0]);
-    mkBool(v, args[0]->type == tString);
+    mkBool(v, args[0]->isString());
 }
 
 
@@ -331,6 +333,8 @@ struct CompareValues
             return v1->fpoint < v2->integer;
         if (v1->type == tInt && v2->type == tFloat)
             return v1->integer < v2->fpoint;
+        if (v1->isString() && v2->isString())
+            return strcmp(v1->getString(), v2->getString()) < 0;
         if (v1->type != v2->type)
             throw EvalError(format("cannot compare %1% with %2%") % showType(*v1) % showType(*v2));
         switch (v1->type) {
@@ -338,8 +342,6 @@ struct CompareValues
                 return v1->integer < v2->integer;
             case tFloat:
                 return v1->fpoint < v2->fpoint;
-            case tString:
-                return strcmp(v1->string.s, v2->string.s) < 0;
             case tPath:
                 return strcmp(v1->path, v2->path) < 0;
             default:
@@ -495,10 +497,10 @@ static void prim_deepSeq(EvalState & state, const Pos & pos, Value * * args, Val
 static void prim_trace(EvalState & state, const Pos & pos, Value * * args, Value & v)
 {
     state.forceValue(*args[0]);
-    if (args[0]->type == tString)
+    if (args[0]->isString())
-        printError(format("trace: %1%") % args[0]->string.s);
+        printError("trace: %s", args[0]->getString());
     else
-        printError(format("trace: %1%") % *args[0]);
+        printError("trace: %s", *args[0]);
     state.forceValue(*args[1]);
     v = *args[1];
 }
@@ -1134,7 +1136,7 @@ static void prim_attrNames(EvalState & state, const Pos & pos, Value * * args, V
         mkString(*(v.listElems()[n++] = state.allocValue()), 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->getString(), v2->getString()) < 0; });
 }
 
 
@@ -1211,10 +1213,9 @@ static void prim_removeAttrs(EvalState & state, const Pos & pos, Value * * args,
 
     /* Get the attribute names to be removed. */
     std::set<Symbol> names;
-    for (unsigned int i = 0; i < args[1]->listSize(); ++i) {
+    for (unsigned int i = 0; i < args[1]->listSize(); ++i)
-        state.forceStringNoCtx(*args[1]->listElems()[i], pos);
+        names.insert(state.symbols.create(
-        names.insert(state.symbols.create(args[1]->listElems()[i]->string.s));
+                state.forceStringNoCtx(*args[1]->listElems()[i], pos)));
-    }
 
     /* Copy all attributes not in that set.  Note that we don't need
        to sort v.attrs because it's a subset of an already sorted

src/libexpr/value-to-json.cc

@@ -26,9 +26,10 @@ void printValueAsJSON(EvalState & state, bool strict,
             out.write(v.boolean);
             break;
 
-        case tString:
+        case tShortString:
+        case tLongString:
             v.getContext(context);
-            out.write(v.string.s);
+            out.write(v.getString());
             break;
 
         case tPath:

src/libexpr/value-to-xml.cc

@@ -68,10 +68,11 @@ static void printValueAsXML(EvalState & state, bool strict, bool location,
             doc.writeEmptyElement("bool", singletonAttrs("value", v.boolean ? "true" : "false"));
             break;
 
-        case tString:
+        case tShortString:
+        case tLongString:
             /* !!! show the context? */
             v.getContext(context);
-            doc.writeEmptyElement("string", singletonAttrs("value", v.string.s));
+            doc.writeEmptyElement("string", singletonAttrs("value", v.getString()));
             break;
 
         case tPath:
@@ -92,15 +93,15 @@ static void printValueAsXML(EvalState & state, bool strict, bool location,
                 a = v.attrs->find(state.sDrvPath);
                 if (a != v.attrs->end()) {
                     if (strict) state.forceValue(*a->value);
-                    if (a->value->type == tString)
+                    if (a->value->isString())
-                        xmlAttrs["drvPath"] = drvPath = a->value->string.s;
+                        xmlAttrs["drvPath"] = drvPath = a->value->getString();
                 }
 
                 a = v.attrs->find(state.sOutPath);
                 if (a != v.attrs->end()) {
                     if (strict) state.forceValue(*a->value);
-                    if (a->value->type == tString)
+                    if (a->value->isString())
-                        xmlAttrs["outPath"] = a->value->string.s;
+                        xmlAttrs["outPath"] = a->value->getString();
                 }
 
                 XMLOpenElement _(doc, "derivation", xmlAttrs);

src/libexpr/value.hh

@@ -114,7 +114,7 @@ struct Value : Object
            derivation, and the other store paths in C will be added to
            the inputSrcs of the derivations. */
         struct {
-            const char * s;
+            const char * _s;
             Context * context;
         } string;
 
@@ -186,7 +186,7 @@ public:
 
     void getContext(PathSet & context)
     {
-        if (string.context) {
+        if (type == tLongString && string.context) {
             if (((ptrdiff_t) string.context) & 1) {
                 auto s = (const std::string *) (((ptrdiff_t) string.context) & ~1UL);
                 context.insert(*s);
@@ -197,6 +197,19 @@ public:
             }
         }
     }
+
+    bool isString() const
+    {
+        return type == tShortString || type == tLongString;
+    }
+
+    const char * getString() const
+    {
+        if (type == tShortString)
+            return (const char *) &string;
+        else
+            return string._s;
+    }
 };
 
 
@@ -245,21 +258,22 @@ static inline void mkPrimOpApp(Value & v, Value & left, Value & right)
 
 static inline void mkStringNoCopy(Value & v, const char * s)
 {
-    v.type = tString;
+    // FIXME: copy short strings?
-    v.string.s = s;
+    v.type = tLongString;
+    v.string._s = s;
     v.string.context = 0;
 }
 
 
+void mkString(Value & v, const char * s);
+
+
 static inline void mkString(Value & v, const Symbol & s)
 {
-    mkStringNoCopy(v, ((const string &) s).c_str());
+    mkString(v, ((const string &) s).c_str());
 }
 
 
-void mkString(Value & v, const char * s);
-
-
 static inline void mkPathNoCopy(Value & v, const char * s)
 {
     v.type = tPath;

src/nix-env/nix-env.cc

@@ -1116,9 +1116,9 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs)
                             if (!v)
                                 printError("derivation '%s' has invalid meta attribute '%s'", i.queryName(), j);
                             else {
-                                if (v->type == tString) {
+                                if (v->isString()) {
                                     attrs2["type"] = "string";
-                                    attrs2["value"] = v->string.s;
+                                    attrs2["value"] = v->getString();
                                     xml.writeEmptyElement("meta", attrs2);
                                 } else if (v->type == tInt) {
                                     attrs2["type"] = "int";
@@ -1136,9 +1136,9 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs)
                                     attrs2["type"] = "strings";
                                     XMLOpenElement m(xml, "meta", attrs2);
                                     for (unsigned int j = 0; j < v->listSize(); ++j) {
-                                        if (v->listElems()[j]->type != tString) continue;
+                                        if (!v->listElems()[j]->isString()) continue;
                                         XMLAttrs attrs3;
-                                        attrs3["value"] = v->listElems()[j]->string.s;
+                                        attrs3["value"] = v->listElems()[j]->getString();
                                         xml.writeEmptyElement("string", attrs3);
                                     }
                               } else if (v->type == tAttrs) {
@@ -1147,10 +1147,10 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs)
                                   Bindings & attrs = *v->attrs;
                                   for (auto &i : attrs) {
                                       Attr & a(*attrs.find(i.name));
-                                      if(a.value->type != tString) continue;
+                                      if (!a.value->isString()) continue;
                                       XMLAttrs attrs3;
                                       attrs3["type"] = i.name;
-                                      attrs3["value"] = a.value->string.s;
+                                      attrs3["value"] = a.value->getString();
                                       xml.writeEmptyElement("string", attrs3);
                                 }
                               }

src/nix/repl.cc

@@ -606,9 +606,10 @@ std::ostream & NixRepl::printValue(std::ostream & str, Value & v, unsigned int m
         str << ESC_CYA << (v.boolean ? "true" : "false") << ESC_END;
         break;
 
-    case tString:
+    case tShortString:
+    case tLongString:
         str << ESC_YEL;
-        printStringValue(str, v.string.s);
+        printStringValue(str, v.getString());
         str << ESC_END;
         break;