* Don't create thunks for simple constants (integers, strings, paths)

  and allocate them only once.
* Move Value and related functions into value.hh.

5 files changed, 193 insertions, 133 deletions

diff --git a/src/libexpr/Makefile.am b/src/libexpr/Makefile.am
index c15975777..9ca4b2448 100644
--- a/src/libexpr/Makefile.am
+++ b/src/libexpr/Makefile.am
@@ -8,7 +8,7 @@ libexpr_la_SOURCES = \
 pkginclude_HEADERS = \
  nixexpr.hh eval.hh lexer-tab.hh parser-tab.hh \
  get-drvs.hh attr-path.hh value-to-xml.hh common-opts.hh \
- names.hh symbol-table.hh
+ names.hh symbol-table.hh value.hh
 
 libexpr_la_LIBADD = ../libutil/libutil.la ../libstore/libstore.la \
  ../boost/format/libformat.la @BDW_GC_LIBS@
diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc
index c58b95126..1e3f42edc 100644
--- a/src/libexpr/eval.cc
+++ b/src/libexpr/eval.cc
@@ -283,9 +283,7 @@ LocalNoInline(void addErrorPrefix(Error & e, const char * s, const string & s2,
 
 void mkString(Value & v, const char * s)
 {
-    v.type = tString;
-    v.string.s = GC_STRDUP(s);
-    v.string.context = 0;
+    mkStringNoCopy(v, GC_STRDUP(s));
 }
 
 
@@ -303,19 +301,9 @@ void mkString(Value & v, const string & s, const PathSet & context)
 }
 
 
-void mkString(Value & v, const Symbol & s)
-{
-    v.type = tString;
-    v.string.s = ((string) s).c_str();
-    v.string.context = 0;
-}
-
-
 void mkPath(Value & v, const char * s)
 {
-    clearValue(v);
-    v.type = tPath;
-    v.path = GC_STRDUP(s);
+    mkPathNoCopy(v, GC_STRDUP(s));
 }
 
 
@@ -426,6 +414,26 @@ Value * ExprVar::maybeThunk(EvalState & state, Env & env)
 }
 
 
+Value * ExprString::maybeThunk(EvalState & state, Env & env)
+{
+    nrAvoided++;
+    return &v;
+}
+
+Value * ExprInt::maybeThunk(EvalState & state, Env & env)
+{
+    nrAvoided++;
+    return &v;
+}
+
+Value * ExprPath::maybeThunk(EvalState & state, Env & env)
+{
+    nrAvoided++;
+    return &v;
+}
+
+
+
 void EvalState::evalFile(const Path & path, Value & v)
 {
     FileEvalCache::iterator i = fileEvalCache.find(path);
@@ -454,7 +462,7 @@ struct RecursionCounter
             state.maxRecursionDepth = state.recursionDepth;
     }
     ~RecursionCounter()
-    {   
+    {
         state.recursionDepth--;
     }
 };
@@ -512,19 +520,19 @@ void Expr::eval(EvalState & state, Env & env, Value & v)
 
 void ExprInt::eval(EvalState & state, Env & env, Value & v)
 {
-    mkInt(v, n);
+    v = this->v;
 }
 
 
 void ExprString::eval(EvalState & state, Env & env, Value & v)
 {
-    mkString(v, s);
+    v = this->v;
 }
 
 
 void ExprPath::eval(EvalState & state, Env & env, Value & v)
 {
-    mkPath(v, s.c_str());
+    v = this->v;
 }
 
 
diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh
index 75438f5eb..91004fe4c 100644
--- a/src/libexpr/eval.hh
+++ b/src/libexpr/eval.hh
@@ -1,6 +1,7 @@
 #ifndef __EVAL_H
 #define __EVAL_H
 
+#include "value.hh"
 #include "nixexpr.hh"
 #include "symbol-table.hh"
 #include "hash.hh"
@@ -16,8 +17,6 @@ namespace nix {
 
 
 class EvalState;
-struct Env;
-struct Value;
 struct Attr;
 
 
@@ -38,23 +37,6 @@ public:
 };
 
 
-typedef enum {
-    tInt = 1,
-    tBool,
-    tString,
-    tPath,
-    tNull,
-    tAttrs,
-    tList,
-    tThunk,
-    tApp,
-    tLambda,
-    tBlackhole,
-    tPrimOp,
-    tPrimOpApp,
-} ValueType;
-
-
 typedef void (* PrimOpFun) (EvalState & state, Value * * args, Value & v);
 
 
@@ -68,64 +50,6 @@ struct PrimOp
 };
 
 
-struct Value
-{
-    ValueType type;
-    union 
-    {
-        int integer;
-        bool boolean;
-        
-        /* Strings in the evaluator carry a so-called `context' (the
-           ATermList) which is a list of strings representing store
-           paths.  This is to allow users to write things like
-
-             "--with-freetype2-library=" + freetype + "/lib"
-
-           where `freetype' is a derivation (or a source to be copied
-           to the store).  If we just concatenated the strings without
-           keeping track of the referenced store paths, then if the
-           string is used as a derivation attribute, the derivation
-           will not have the correct dependencies in its inputDrvs and
-           inputSrcs.
-
-           The semantics of the context is as follows: when a string
-           with context C is used as a derivation attribute, then the
-           derivations in C will be added to the inputDrvs of the
-           derivation, and the other store paths in C will be added to
-           the inputSrcs of the derivations.
-
-           For canonicity, the store paths should be in sorted order. */
-        struct {
-            const char * s;
-            const char * * context; // must be in sorted order
-        } string;
-        
-        const char * path;
-        Bindings * attrs;
-        struct {
-            unsigned int length;
-            Value * * elems;
-        } list;
-        struct {
-            Env * env;
-            Expr * expr;
-        } thunk;
-        struct {
-            Value * left, * right;
-        } app;
-        struct {
-            Env * env;
-            ExprLambda * fun;
-        } lambda;
-        PrimOp * primOp;
-        struct {
-            Value * left, * right;
-        } primOpApp;
-    };
-};
-
-
 struct Env
 {
     Env * up;
@@ -149,41 +73,7 @@ struct Attr
 };
 
 
-/* After overwriting an app node, be sure to clear pointers in the
-   Value to ensure that the target isn't kept alive unnecessarily. */
-static inline void clearValue(Value & v)
-{
-    v.app.right = 0;
-}
-
-
-static inline void mkInt(Value & v, int n)
-{
-    clearValue(v);
-    v.type = tInt;
-    v.integer = n;
-}
-
-
-static inline void mkBool(Value & v, bool b)
-{
-    clearValue(v);
-    v.type = tBool;
-    v.boolean = b;
-}
-
-
-static inline void mkApp(Value & v, Value & left, Value & right)
-{
-    v.type = tApp;
-    v.app.left = &left;
-    v.app.right = &right;
-}
-
-
-void mkString(Value & v, const char * s);
 void mkString(Value & v, const string & s, const PathSet & context = PathSet());
-void mkPath(Value & v, const char * s);
 
 void copyContext(const Value & v, PathSet & context);
 
diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh
index 1c5cc07af..6eb771a72 100644
--- a/src/libexpr/nixexpr.hh
+++ b/src/libexpr/nixexpr.hh
@@ -1,6 +1,7 @@
 #ifndef __NIXEXPR_H
 #define __NIXEXPR_H
 
+#include "value.hh"
 #include "symbol-table.hh"
 
 #include <map>
@@ -66,15 +67,19 @@ std::ostream & operator << (std::ostream & str, Expr & e);
 struct ExprInt : Expr
 {
     int n;
-    ExprInt(int n) : n(n) { };
+    Value v;
+    ExprInt(int n) : n(n) { mkInt(v, n); };
     COMMON_METHODS
+    Value * maybeThunk(EvalState & state, Env & env);
 };
 
 struct ExprString : Expr
 {
     Symbol s;
-    ExprString(const Symbol & s) : s(s) { };
+    Value v;
+    ExprString(const Symbol & s) : s(s) { mkString(v, s); };
     COMMON_METHODS
+    Value * maybeThunk(EvalState & state, Env & env);
 };
 
 /* Temporary class used during parsing of indented strings. */
@@ -87,8 +92,10 @@ struct ExprIndStr : Expr
 struct ExprPath : Expr
 {
     string s;
-    ExprPath(const string & s) : s(s) { };
+    Value v;
+    ExprPath(const string & s) : s(s) { mkPathNoCopy(v, this->s.c_str()); };
     COMMON_METHODS
+    Value * maybeThunk(EvalState & state, Env & env);
 };
 
 struct VarRef
diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh
new file mode 100644
index 000000000..41512d40b
--- /dev/null
+++ b/src/libexpr/value.hh
@@ -0,0 +1,155 @@
+#ifndef __VALUE_H
+#define __VALUE_H
+
+#include "symbol-table.hh"
+
+namespace nix {
+
+
+typedef enum {
+    tInt = 1,
+    tBool,
+    tString,
+    tPath,
+    tNull,
+    tAttrs,
+    tList,
+    tThunk,
+    tApp,
+    tLambda,
+    tBlackhole,
+    tPrimOp,
+    tPrimOpApp,
+} ValueType;
+
+
+struct Bindings;
+struct Env;
+struct Expr;
+struct ExprLambda;
+struct PrimOp;
+struct PrimOp;
+struct Symbol;
+
+
+struct Value
+{
+    ValueType type;
+    union 
+    {
+        int integer;
+        bool boolean;
+        
+        /* Strings in the evaluator carry a so-called `context' (the
+           ATermList) which is a list of strings representing store
+           paths.  This is to allow users to write things like
+
+             "--with-freetype2-library=" + freetype + "/lib"
+
+           where `freetype' is a derivation (or a source to be copied
+           to the store).  If we just concatenated the strings without
+           keeping track of the referenced store paths, then if the
+           string is used as a derivation attribute, the derivation
+           will not have the correct dependencies in its inputDrvs and
+           inputSrcs.
+
+           The semantics of the context is as follows: when a string
+           with context C is used as a derivation attribute, then the
+           derivations in C will be added to the inputDrvs of the
+           derivation, and the other store paths in C will be added to
+           the inputSrcs of the derivations.
+
+           For canonicity, the store paths should be in sorted order. */
+        struct {
+            const char * s;
+            const char * * context; // must be in sorted order
+        } string;
+        
+        const char * path;
+        Bindings * attrs;
+        struct {
+            unsigned int length;
+            Value * * elems;
+        } list;
+        struct {
+            Env * env;
+            Expr * expr;
+        } thunk;
+        struct {
+            Value * left, * right;
+        } app;
+        struct {
+            Env * env;
+            ExprLambda * fun;
+        } lambda;
+        PrimOp * primOp;
+        struct {
+            Value * left, * right;
+        } primOpApp;
+    };
+};
+
+
+/* After overwriting an app node, be sure to clear pointers in the
+   Value to ensure that the target isn't kept alive unnecessarily. */
+static inline void clearValue(Value & v)
+{
+    v.app.right = 0;
+}
+
+
+static inline void mkInt(Value & v, int n)
+{
+    clearValue(v);
+    v.type = tInt;
+    v.integer = n;
+}
+
+
+static inline void mkBool(Value & v, bool b)
+{
+    clearValue(v);
+    v.type = tBool;
+    v.boolean = b;
+}
+
+
+static inline void mkApp(Value & v, Value & left, Value & right)
+{
+    v.type = tApp;
+    v.app.left = &left;
+    v.app.right = &right;
+}
+
+
+static inline void mkStringNoCopy(Value & v, const char * s)
+{
+    v.type = tString;
+    v.string.s = s;
+    v.string.context = 0;
+}
+
+
+static inline void mkString(Value & v, const Symbol & s)
+{
+    mkStringNoCopy(v, ((string) s).c_str());
+}
+
+
+void mkString(Value & v, const char * s);
+
+
+static inline void mkPathNoCopy(Value & v, const char * s)
+{
+    clearValue(v);
+    v.type = tPath;
+    v.path = s;
+}
+
+
+void mkPath(Value & v, const char * s);
+
+
+}
+
+#endif /* !__VALUE_H */