Skip to content

Commit

Permalink
Make the evaluator multi-threaded
Browse files Browse the repository at this point in the history
This adds basic support for multi-threaded evaluation. Currently, only
"nix search" uses this to evaluate packages in parallel.

The implementation is based on Harris et al., "Haskell on a
Shared-Memory Multiprocessor" (Haskell Workshop '05) (sans the
blackholing for now), which describes letting threads evaluate the
same thunk in parallel without locking. Since we're in a purely
functionaly language, this may lead to work duplication (if two thread
hit the same thunk concurrently), but is semantically safe. We just
need to ensure that thunk overwriting doesn't cause problems.

Thus, the "thunk" and "app" cases of the Value union are now stored
separately so that they're never overwritten. This does increase
memory consumption by 16 bytes per value, but it might be possible to
optimize this a bit. Another worry is garbage collection
effectiveness. Probably the collector will need to be taught to ignore
"thunk" and "app" for non-thunk values.

Similarly, the "with" attribute set is now stored in a separate field
of Env, rather than in values[0], to account for the case where two
threads evaluate the "with" set concurrently.

There are probably still quite a few races like this one in elemAt:

  state.forceValue(*list.listElems()[n]);
  v = *list.listElems()[n];

This is now unsafe, because there is a slight possibility ‘list’ is
overwritten by another thread (by a semantically equivalent value), so
that ‘list.listElems()[n]’ no longer refers to the value forced in the
first line. So we now need to write:

  auto & v2 = *list.listElems()[n];
  state.forceValue(v2);
  v = v2;

Also, a lot of code updates thunks like this:

  v.type = tInt;
  v.int = 123;

This is now unsafe; it should be the other way around, with some
memory ordering to prevent the compiler from swapping them back. I
worked around this for now by having forceValue() use a temporary
value for the thunk evaluation result.
  • Loading branch information
edolstra committed Aug 29, 2016
1 parent 9cdac0e commit 3ad4128
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 41 deletions.
24 changes: 8 additions & 16 deletions src/libexpr/eval-inline.hh
Expand Up @@ -27,23 +27,15 @@ LocalNoInlineNoReturn(void throwTypeError(const char * s, const Value & v, const
void EvalState::forceValue(Value & v, const Pos & pos)
{
if (v.type == tThunk) {
Env * env = v.thunk.env;
Expr * expr = v.thunk.expr;
try {
v.type = tBlackhole;
//checkInterrupt();
expr->eval(*this, *env, v);
} catch (Error & e) {
v.type = tThunk;
v.thunk.env = env;
v.thunk.expr = expr;
throw;
}
Value vTmp;
v.thunk.expr->eval(*this, *v.thunk.env, vTmp);
v = vTmp;
}
else if (v.type == tApp) {
Value vTmp;
callFunction(*v.app.left, *v.app.right, vTmp, noPos);
v = vTmp;
}
else if (v.type == tApp)
callFunction(*v.app.left, *v.app.right, v, noPos);
else if (v.type == tBlackhole)
throwEvalError("infinite recursion encountered, at %1%", pos);
}


Expand Down
21 changes: 9 additions & 12 deletions src/libexpr/eval.cc
Expand Up @@ -492,12 +492,13 @@ inline Value * EvalState::lookupVar(Env * env, const ExprVar & var, bool noEval)
if (!var.fromWith) return env->values[var.displ];

while (1) {
if (!env->haveWithAttrs) {
Expr * withAttrs = env->withAttrs;
if (withAttrs) {
if (noEval) return 0;
Value * v = allocValue();
evalAttrs(*env->up, (Expr *) env->values[0], *v);
evalAttrs(*env->up, withAttrs, *v);
env->values[0] = v;
env->haveWithAttrs = true;
env->withAttrs = 0;
}
Bindings::iterator j = env->values[0]->attrs->find(var.name);
if (j != env->values[0]->attrs->end()) {
Expand Down Expand Up @@ -906,7 +907,7 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v)
if (state.countCalls && pos2) state.attrSelects[*pos2]++;
}

state.forceValue(*vAttrs, ( pos2 != NULL ? *pos2 : this->pos ) );
state.forceValue(*vAttrs, pos2 ? *pos2 : this->pos);

} catch (Error & e) {
if (pos2 && pos2->file != state.sDerivationNix)
Expand All @@ -928,6 +929,7 @@ void ExprOpHasAttr::eval(EvalState & state, Env & env, Value & v)

for (auto & i : attrPath) {
state.forceValue(*vAttrs);
assert(vAttrs->type != tAttrs || vAttrs->attrs != 0);
Bindings::iterator j;
Symbol name = getName(i, state, env);
if (vAttrs->type != tAttrs ||
Expand Down Expand Up @@ -1140,8 +1142,7 @@ void ExprWith::eval(EvalState & state, Env & env, Value & v)
Env & env2(state.allocEnv(1));
env2.up = &env;
env2.prevWith = prevWith;
env2.haveWithAttrs = false;
env2.values[0] = (Value *) attrs;
env2.withAttrs = attrs;

body->eval(state, env2, v);
}
Expand Down Expand Up @@ -1416,12 +1417,8 @@ void EvalState::forceFunction(Value & v, const Pos & pos)
string EvalState::forceString(Value & v, const Pos & pos)
{
forceValue(v, pos);
if (v.type != tString) {
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);
}
if (v.type != tString)
throwTypeError("value is %1% while a string was expected, at %2%", v, pos);
return string(v.string.s);
}

Expand Down
4 changes: 2 additions & 2 deletions src/libexpr/eval.hh
Expand Up @@ -37,8 +37,8 @@ struct Env
{
Env * up;
unsigned short size; // used by ‘valueSize’
unsigned short prevWith:15; // nr of levels up to next `with' environment
unsigned short haveWithAttrs:1;
unsigned short prevWith; // nr of levels up to next `with' environment
Expr * withAttrs;
Value * values[0];
};

Expand Down
6 changes: 3 additions & 3 deletions src/libexpr/primops.cc
Expand Up @@ -1231,8 +1231,9 @@ static void elemAt(EvalState & state, const Pos & pos, Value & list, int n, Valu
state.forceList(list, pos);
if (n < 0 || (unsigned int) n >= list.listSize())
throw Error(format("list index %1% is out of bounds, at %2%") % n % pos);
state.forceValue(*list.listElems()[n]);
v = *list.listElems()[n];
auto & v2 = *list.listElems()[n];
state.forceValue(v2);
v = v2;
}


Expand Down Expand Up @@ -1426,7 +1427,6 @@ static void prim_sort(EvalState & state, const Pos & pos, Value * * args, Value
v.listElems()[n] = args[1]->listElems()[n];
}


auto comparator = [&](Value * a, Value * b) {
/* Optimization: if the comparator is lessThan, bypass
callFunction. */
Expand Down
37 changes: 29 additions & 8 deletions src/libexpr/value.hh
Expand Up @@ -90,7 +90,21 @@ std::ostream & operator << (std::ostream & str, const ExternalValueBase & v);

struct Value
{
// FIXME: should be std::atomic, with readers using
// memory_order_relaxed and writers using memory_order_release.
ValueType type;

union
{
struct {
Env * env;
Expr * expr;
} thunk;
struct {
Value * left, * right;
} app;
};

union
{
NixInt integer;
Expand Down Expand Up @@ -128,13 +142,6 @@ struct Value
Value * * elems;
} bigList;
Value * smallList[2];
struct {
Env * env;
Expr * expr;
} thunk;
struct {
Value * left, * right;
} app;
struct {
Env * env;
ExprLambda * fun;
Expand All @@ -147,6 +154,20 @@ struct Value
NixFloat fpoint;
};

Value()
{
}

Value(const Value & v) = delete;

Value & operator = (const Value & v)
{
lambda.env = v.lambda.env;
lambda.fun = v.lambda.fun;
type = v.type; // FIXME: memory_order_release
return *this;
}

bool isList() const
{
return type == tList1 || type == tList2 || type == tListN;
Expand All @@ -173,7 +194,7 @@ struct Value
Value to ensure that the target isn't kept alive unnecessarily. */
static inline void clearValue(Value & v)
{
v.app.left = v.app.right = 0;
//v.app.left = v.app.right = 0;
}


Expand Down

0 comments on commit 3ad4128

Please sign in to comment.