From 1b978596b575bda096a9e896a5cc3502943e5a60 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 26 Feb 2022 02:30:11 +0100 Subject: Value::mkString: Avoid crash from null string_view --- src/libexpr/eval.cc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'src/libexpr/eval.cc') diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 2d7309738..b954e7bd9 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -805,7 +805,13 @@ LocalNoInline(void addErrorTrace(Error & e, const Pos & pos, const char * s, con void Value::mkString(std::string_view s) { - mkString(dupStringWithLen(s.data(), s.size())); + if (s.size() == 0) { + // s.data() may not be valid and we don't need to allocate. + mkString(""); + } + else { + mkString(dupStringWithLen(s.data(), s.size())); + } } -- cgit v1.2.3 From bbf55383e7a9914cf79b8879e234f444cc774bfa Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 26 Feb 2022 02:30:29 +0100 Subject: Value::mkPath: Avoid potential crash from null string_view --- src/libexpr/eval.cc | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) (limited to 'src/libexpr/eval.cc') diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index b954e7bd9..7f82aa5f4 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -842,7 +842,14 @@ void Value::mkStringMove(const char * s, const PathSet & context) void Value::mkPath(std::string_view s) { - mkPath(dupStringWithLen(s.data(), s.size())); + if (s.size() == 0) { + // Pathological, but better than crashing in dupStringWithLen, as + // s.data() may be null. + mkPath(""); + } + else { + mkPath(dupStringWithLen(s.data(), s.size())); + } } -- cgit v1.2.3 From da260f579d5f4a66d6d0d0e1297b69666d049d03 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 27 Feb 2022 12:50:18 +0100 Subject: dupStringWithLen -> makeImmutableString Refactor the `size == 0` logic into a new helper function that replaces dupStringWithLen. The name had to change, because unlike a `dup`-function, it does not always allocate a new string. --- src/libexpr/eval.cc | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) (limited to 'src/libexpr/eval.cc') diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 7f82aa5f4..5bf161cc0 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -63,9 +63,15 @@ static char * dupString(const char * s) } -static char * dupStringWithLen(const char * s, size_t size) +// When there's no need to write to the string, we can optimize away empty +// string allocations. +// This function handles makeImmutableStringWithLen(null, 0) by returning the +// empty string. +static const char * makeImmutableStringWithLen(const char * s, size_t size) { char * t; + if (size == 0) + return ""; #if HAVE_BOEHMGC t = GC_STRNDUP(s, size); #else @@ -75,6 +81,10 @@ static char * dupStringWithLen(const char * s, size_t size) return t; } +static inline const char * makeImmutableString(std::string_view s) { + return makeImmutableStringWithLen(s.data(), s.size()); +} + RootValue allocRootValue(Value * v) { @@ -805,13 +815,7 @@ LocalNoInline(void addErrorTrace(Error & e, const Pos & pos, const char * s, con void Value::mkString(std::string_view s) { - if (s.size() == 0) { - // s.data() may not be valid and we don't need to allocate. - mkString(""); - } - else { - mkString(dupStringWithLen(s.data(), s.size())); - } + mkString(makeImmutableString(s)); } @@ -842,14 +846,7 @@ void Value::mkStringMove(const char * s, const PathSet & context) void Value::mkPath(std::string_view s) { - if (s.size() == 0) { - // Pathological, but better than crashing in dupStringWithLen, as - // s.data() may be null. - mkPath(""); - } - else { - mkPath(dupStringWithLen(s.data(), s.size())); - } + mkPath(makeImmutableString(s)); } -- cgit v1.2.3 From 0c6e46e34965ba0db4e0b755ee473868a4fef21f Mon Sep 17 00:00:00 2001 From: regnat Date: Tue, 8 Mar 2022 06:16:51 +0100 Subject: Add some suggestions to the evaluator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make the evaluator show some suggestions when trying to access an invalid field from an attrset. ```console $ nix eval --expr '{ foo = 1; }.foa' error: attribute 'foa' missing at «string»:1:1: 1| { foo = 1; }.foa | ^ Did you mean foo? ``` --- src/libexpr/eval.cc | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) (limited to 'src/libexpr/eval.cc') diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 5bf161cc0..1d88e8709 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -727,6 +727,15 @@ LocalNoInlineNoReturn(void throwEvalError(const char * s, const std::string & s2 throw EvalError(s, s2); } +LocalNoInlineNoReturn(void throwEvalError(const Pos & pos, const Suggestions & suggestions, const char * s, const std::string & s2)) +{ + throw EvalError({ + .msg = hintfmt(s, s2), + .errPos = pos, + .suggestions = suggestions, + }); +} + LocalNoInlineNoReturn(void throwEvalError(const Pos & pos, const char * s, const std::string & s2)) { throw EvalError({ @@ -1281,8 +1290,15 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v) } } else { state.forceAttrs(*vAttrs, pos); - if ((j = vAttrs->attrs->find(name)) == vAttrs->attrs->end()) - throwEvalError(pos, "attribute '%1%' missing", name); + if ((j = vAttrs->attrs->find(name)) == vAttrs->attrs->end()) { + std::set allAttrNames; + for (auto & attr : *vAttrs->attrs) + allAttrNames.insert(attr.name); + throwEvalError( + pos, + Suggestions::bestMatches(allAttrNames, name), + "attribute '%1%' missing", name); + } } vAttrs = j->value; pos2 = j->pos; -- cgit v1.2.3 From 33b7514035a967df2ab61ab9770627157aa4f5c5 Mon Sep 17 00:00:00 2001 From: regnat Date: Tue, 8 Mar 2022 16:07:17 +0100 Subject: Try and make the darwin build happy --- src/libexpr/eval.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/libexpr/eval.cc') diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 1d88e8709..3bfb82b16 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -729,7 +729,7 @@ LocalNoInlineNoReturn(void throwEvalError(const char * s, const std::string & s2 LocalNoInlineNoReturn(void throwEvalError(const Pos & pos, const Suggestions & suggestions, const char * s, const std::string & s2)) { - throw EvalError({ + throw EvalError(ErrorInfo { .msg = hintfmt(s, s2), .errPos = pos, .suggestions = suggestions, @@ -738,7 +738,7 @@ LocalNoInlineNoReturn(void throwEvalError(const Pos & pos, const Suggestions & s LocalNoInlineNoReturn(void throwEvalError(const Pos & pos, const char * s, const std::string & s2)) { - throw EvalError({ + throw EvalError(ErrorInfo { .msg = hintfmt(s, s2), .errPos = pos }); -- cgit v1.2.3 From f6078e474d5fc41c8a7f683865d60490bf0c7040 Mon Sep 17 00:00:00 2001 From: regnat Date: Tue, 8 Mar 2022 16:20:01 +0100 Subject: Also display some suggestions for invalid formal arguments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ```console $ nix eval --expr '({ foo ? 1 }: foo) { fob = 2; }' error: anonymous function at (string):1:2 called with unexpected argument 'fob' at «string»:1:1: 1| ({ foo ? 1 }: foo) { fob = 2; } | ^ Did you mean foo? ``` Not that because Nix will first check for _missing_ arguments before checking for extra arguments, `({ foo }: foo) { fob = 1; }` will complain about the missing `foo` argument (rather than extra `fob`) and so won’t display a suggestion. --- src/libexpr/eval.cc | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) (limited to 'src/libexpr/eval.cc') diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 3bfb82b16..a5e9dc286 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -782,6 +782,16 @@ LocalNoInlineNoReturn(void throwTypeError(const Pos & pos, const char * s, const }); } +LocalNoInlineNoReturn(void throwTypeError(const Pos & pos, const Suggestions & suggestions, const char * s, const ExprLambda & fun, const Symbol & s2)) +{ + throw TypeError(ErrorInfo { + .msg = hintfmt(s, fun.showNamePos(), s2), + .errPos = pos, + .suggestions = suggestions, + }); +} + + LocalNoInlineNoReturn(void throwTypeError(const char * s, const Value & v)) { throw TypeError(s, showType(v)); @@ -1414,8 +1424,17 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & /* Nope, so show the first unexpected argument to the user. */ for (auto & i : *args[0]->attrs) - if (!lambda.formals->has(i.name)) - throwTypeError(pos, "%1% called with unexpected argument '%2%'", lambda, i.name); + if (!lambda.formals->has(i.name)) { + std::set formalNames; + for (auto & formal : lambda.formals->formals) + formalNames.insert(formal.name); + throwTypeError( + pos, + Suggestions::bestMatches(formalNames, i.name), + "%1% called with unexpected argument '%2%'", + lambda, + i.name); + } abort(); // can't happen } } -- cgit v1.2.3 From 4b2b0d3a5528133898a15f3210c79162ec993823 Mon Sep 17 00:00:00 2001 From: pennae Date: Wed, 29 Dec 2021 01:28:58 +0100 Subject: remove GC_PTR_STORE_AND_DIRTY MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit turns out it's only necessary for MANUAL_VDB, which nix doesn't use. omitting them gives a slight performance improvement on eval. before: Benchmark 1: nix flakes search --no-eval-cache --offline ../nixpkgs hello Time (mean ± σ): 6.988 s ± 0.061 s [User: 5.935 s, System: 0.845 s] Range (min … max): 6.865 s … 7.075 s 20 runs Benchmark 2: nix flakes eval -f ../nixpkgs/pkgs/development/haskell-modules/hackage-packages.nix Time (mean ± σ): 332.6 ms ± 3.9 ms [User: 299.6 ms, System: 32.9 ms] Range (min … max): 328.1 ms … 339.1 ms 20 runs Benchmark 3: nix eval --raw --impure --expr 'with import {}; system' Time (mean ± σ): 2.681 s ± 0.049 s [User: 2.382 s, System: 0.228 s] Range (min … max): 2.607 s … 2.776 s 20 runs after: Benchmark 1: nix flakes search --no-eval-cache --offline ../nixpkgs hello Time (mean ± σ): 6.946 s ± 0.041 s [User: 5.875 s, System: 0.835 s] Range (min … max): 6.834 s … 7.005 s 20 runs Benchmark 2: nix flakes eval -f ../nixpkgs/pkgs/development/haskell-modules/hackage-packages.nix Time (mean ± σ): 330.3 ms ± 2.5 ms [User: 299.2 ms, System: 30.9 ms] Range (min … max): 327.5 ms … 337.7 ms 20 runs Benchmark 3: nix eval --raw --impure --expr 'with import {}; system' Time (mean ± σ): 2.671 s ± 0.035 s [User: 2.370 s, System: 0.232 s] Range (min … max): 2.597 s … 2.749 s 20 runs --- src/libexpr/eval.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/libexpr/eval.cc') diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 193358161..777f6a4ec 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -879,7 +879,7 @@ Value * EvalState::allocValue() /* GC_NEXT is a convenience macro for accessing the first word of an object. Take the first list item, advance the list to the next item, and clear the next pointer. */ void * p = *valueAllocCache; - GC_PTR_STORE_AND_DIRTY(&*valueAllocCache, GC_NEXT(p)); + *valueAllocCache = GC_NEXT(p); GC_NEXT(p) = nullptr; nrValues++; -- cgit v1.2.3 From 60ed4e908a59f258f82356a9dd47e43361d39f2f Mon Sep 17 00:00:00 2001 From: pennae Date: Sun, 26 Dec 2021 19:32:08 +0100 Subject: cache singleton Envs just like Values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit vast majority of envs is this size. before: Benchmark 1: nix flakes search --no-eval-cache --offline ../nixpkgs hello Time (mean ± σ): 6.946 s ± 0.041 s [User: 5.875 s, System: 0.835 s] Range (min … max): 6.834 s … 7.005 s 20 runs Benchmark 2: nix flakes eval -f ../nixpkgs/pkgs/development/haskell-modules/hackage-packages.nix Time (mean ± σ): 330.3 ms ± 2.5 ms [User: 299.2 ms, System: 30.9 ms] Range (min … max): 327.5 ms … 337.7 ms 20 runs Benchmark 3: nix eval --raw --impure --expr 'with import {}; system' Time (mean ± σ): 2.671 s ± 0.035 s [User: 2.370 s, System: 0.232 s] Range (min … max): 2.597 s … 2.749 s 20 runs after: Benchmark 1: nix flakes search --no-eval-cache --offline ../nixpkgs hello Time (mean ± σ): 6.935 s ± 0.052 s [User: 5.852 s, System: 0.853 s] Range (min … max): 6.808 s … 7.026 s 20 runs Benchmark 2: nix flakes eval -f ../nixpkgs/pkgs/development/haskell-modules/hackage-packages.nix Time (mean ± σ): 329.8 ms ± 2.7 ms [User: 299.0 ms, System: 30.8 ms] Range (min … max): 326.6 ms … 336.5 ms 20 runs Benchmark 3: nix flakes eval --raw --impure --file expr.nix Time (mean ± σ): 2.655 s ± 0.038 s [User: 2.364 s, System: 0.220 s] Range (min … max): 2.574 s … 2.737 s 20 runs --- src/libexpr/eval.cc | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) (limited to 'src/libexpr/eval.cc') diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 777f6a4ec..ddbbdeaa6 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -438,8 +438,10 @@ EvalState::EvalState( , regexCache(makeRegexCache()) #if HAVE_BOEHMGC , valueAllocCache(std::allocate_shared(traceable_allocator(), nullptr)) + , env1AllocCache(std::allocate_shared(traceable_allocator(), nullptr)) #else , valueAllocCache(std::make_shared(nullptr)) + , env1AllocCache(std::make_shared(nullptr)) #endif , baseEnv(allocEnv(128)) , staticBaseEnv(false, 0) @@ -892,7 +894,24 @@ Env & EvalState::allocEnv(size_t size) { nrEnvs++; nrValuesInEnvs += size; - Env * env = (Env *) allocBytes(sizeof(Env) + size * sizeof(Value *)); + + Env * env; + + if (size != 1) + env = (Env *) allocBytes(sizeof(Env) + size * sizeof(Value *)); + else { + /* see allocValue for explanations. */ + if (!*env1AllocCache) { + *env1AllocCache = GC_malloc_many(sizeof(Env) + sizeof(Value *)); + if (!*env1AllocCache) throw std::bad_alloc(); + } + + void * p = *env1AllocCache; + *env1AllocCache = GC_NEXT(p); + GC_NEXT(p) = nullptr; + env = (Env *) p; + } + env->type = Env::Plain; /* We assume that env->values has been cleared by the allocator; maybeThunk() and lookupVar fromWith expect this. */ -- cgit v1.2.3 From c96460f3520862d52b7bf3108f609e20384878e7 Mon Sep 17 00:00:00 2001 From: pennae Date: Tue, 28 Dec 2021 19:18:17 +0100 Subject: force-inline a few much-used functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit these functions are called a whole lot, and they're all comparatively small. always inlining them gives ~0.7% performance boost on eval. before: Benchmark 1: nix flakes search --no-eval-cache --offline ../nixpkgs hello Time (mean ± σ): 6.935 s ± 0.052 s [User: 5.852 s, System: 0.853 s] Range (min … max): 6.808 s … 7.026 s 20 runs Benchmark 2: nix flakes eval -f ../nixpkgs/pkgs/development/haskell-modules/hackage-packages.nix Time (mean ± σ): 329.8 ms ± 2.7 ms [User: 299.0 ms, System: 30.8 ms] Range (min … max): 326.6 ms … 336.5 ms 20 runs Benchmark 3: nix flakes eval --raw --impure --file expr.nix Time (mean ± σ): 2.655 s ± 0.038 s [User: 2.364 s, System: 0.220 s] Range (min … max): 2.574 s … 2.737 s 20 runs after: Benchmark 1: nix flakes search --no-eval-cache --offline ../nixpkgs hello Time (mean ± σ): 6.912 s ± 0.036 s [User: 5.823 s, System: 0.856 s] Range (min … max): 6.849 s … 6.980 s 20 runs Benchmark 2: nix flakes eval -f ../nixpkgs/pkgs/development/haskell-modules/hackage-packages.nix Time (mean ± σ): 325.1 ms ± 2.5 ms [User: 293.2 ms, System: 31.8 ms] Range (min … max): 322.2 ms … 332.8 ms 20 runs Benchmark 3: nix flakes eval --raw --impure --file expr.nix Time (mean ± σ): 2.636 s ± 0.024 s [User: 2.352 s, System: 0.226 s] Range (min … max): 2.574 s … 2.681 s 20 runs --- src/libexpr/eval.cc | 53 ----------------------------------------------------- 1 file changed, 53 deletions(-) (limited to 'src/libexpr/eval.cc') diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index ddbbdeaa6..038b6bb7c 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -867,59 +867,6 @@ inline Value * EvalState::lookupVar(Env * env, const ExprVar & var, bool noEval) } -Value * EvalState::allocValue() -{ - /* We use the boehm batch allocator to speed up allocations of Values (of which there are many). - GC_malloc_many returns a linked list of objects of the given size, where the first word - of each object is also the pointer to the next object in the list. This also means that we - have to explicitly clear the first word of every object we take. */ - if (!*valueAllocCache) { - *valueAllocCache = GC_malloc_many(sizeof(Value)); - if (!*valueAllocCache) throw std::bad_alloc(); - } - - /* GC_NEXT is a convenience macro for accessing the first word of an object. - Take the first list item, advance the list to the next item, and clear the next pointer. */ - void * p = *valueAllocCache; - *valueAllocCache = GC_NEXT(p); - GC_NEXT(p) = nullptr; - - nrValues++; - auto v = (Value *) p; - return v; -} - - -Env & EvalState::allocEnv(size_t size) -{ - nrEnvs++; - nrValuesInEnvs += size; - - Env * env; - - if (size != 1) - env = (Env *) allocBytes(sizeof(Env) + size * sizeof(Value *)); - else { - /* see allocValue for explanations. */ - if (!*env1AllocCache) { - *env1AllocCache = GC_malloc_many(sizeof(Env) + sizeof(Value *)); - if (!*env1AllocCache) throw std::bad_alloc(); - } - - void * p = *env1AllocCache; - *env1AllocCache = GC_NEXT(p); - GC_NEXT(p) = nullptr; - env = (Env *) p; - } - - env->type = Env::Plain; - - /* We assume that env->values has been cleared by the allocator; maybeThunk() and lookupVar fromWith expect this. */ - - return *env; -} - - void EvalState::mkList(Value & v, size_t size) { v.mkList(size); -- cgit v1.2.3