From a4fa93d469bf250f001ee28872e86d211d4d47d0 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Tue, 3 Dec 2024 20:38:41 +0100 Subject: [PATCH] libexpr: simplify EvalErrorBuilder interface if we hold the error being built behind a pointer we no longer need to allocate the error builder itself to avoid impacting eval performance. adding && ref qualifiers to builder functions and adding the nodiscard attribute to the class itself also makes the nodiscard attributes kept on the builder functions unnecessary. we do keep the noinlines though, removing them still regresses eval performance even after this change. Change-Id: Ibc14a66955ac32142d97fb3680b0a7e14db250cd --- lix/libexpr/eval-error.cc | 59 +++++++++++++++++---------------------- lix/libexpr/eval-error.hh | 34 ++++++++++------------ lix/libexpr/eval.hh | 7 ++--- 3 files changed, 44 insertions(+), 56 deletions(-) diff --git a/lix/libexpr/eval-error.cc b/lix/libexpr/eval-error.cc index 671b515e1..0da05eac1 100644 --- a/lix/libexpr/eval-error.cc +++ b/lix/libexpr/eval-error.cc @@ -5,45 +5,45 @@ namespace nix { template -EvalErrorBuilder & EvalErrorBuilder::withExitStatus(unsigned int exitStatus) +EvalErrorBuilder EvalErrorBuilder::withExitStatus(unsigned int exitStatus) && { - error.withExitStatus(exitStatus); - return *this; + error->withExitStatus(exitStatus); + return std::move(*this); } template -EvalErrorBuilder & EvalErrorBuilder::atPos(PosIdx pos) +EvalErrorBuilder EvalErrorBuilder::atPos(PosIdx pos) && { - error.err.pos = state.positions[pos]; - return *this; + error->err.pos = state.positions[pos]; + return std::move(*this); } template -EvalErrorBuilder & EvalErrorBuilder::atPos(Value & value, PosIdx fallback) +EvalErrorBuilder EvalErrorBuilder::atPos(Value & value, PosIdx fallback) && { - return atPos(value.determinePos(fallback)); + return std::move(*this).atPos(value.determinePos(fallback)); } template -EvalErrorBuilder & EvalErrorBuilder::withTrace(PosIdx pos, const std::string_view text) +EvalErrorBuilder EvalErrorBuilder::withTrace(PosIdx pos, const std::string_view text) && { - error.err.traces.push_front( + error->err.traces.push_front( Trace{.pos = state.positions[pos], .hint = HintFmt(std::string(text))}); - return *this; + return std::move(*this); } template -EvalErrorBuilder & EvalErrorBuilder::withSuggestions(Suggestions & s) +EvalErrorBuilder EvalErrorBuilder::withSuggestions(Suggestions & s) && { - error.err.suggestions = s; - return *this; + error->err.suggestions = s; + return std::move(*this); } template -EvalErrorBuilder & EvalErrorBuilder::withFrame(const Env & env, const Expr & expr) +EvalErrorBuilder EvalErrorBuilder::withFrame(const Env & env, const Expr & expr) && { if (state.debug) { - error.frame = state.debug->addTrace(DebugTrace{ + error->frame = state.debug->addTrace(DebugTrace{ .pos = state.positions[expr.getPos()], .expr = expr, .env = env, @@ -51,45 +51,38 @@ EvalErrorBuilder & EvalErrorBuilder::withFrame(const Env & env, const Expr .isError = true }).entry; } - return *this; + return std::move(*this); } template -EvalErrorBuilder & EvalErrorBuilder::addTrace(PosIdx pos, HintFmt hint) +EvalErrorBuilder EvalErrorBuilder::addTrace(PosIdx pos, HintFmt hint) && { - error.addTrace(state.positions[pos], hint); - return *this; + error->addTrace(state.positions[pos], hint); + return std::move(*this); } template template -EvalErrorBuilder & -EvalErrorBuilder::addTrace(PosIdx pos, std::string_view formatString, const Args &... formatArgs) +EvalErrorBuilder +EvalErrorBuilder::addTrace(PosIdx pos, std::string_view formatString, const Args &... formatArgs) && { addTrace(state.positions[pos], HintFmt(std::string(formatString), formatArgs...)); - return *this; + return std::move(*this); } template -void EvalErrorBuilder::debugThrow() +void EvalErrorBuilder::debugThrow() && { if (state.debug) { if (auto last = state.debug->traces().next()) { const Env * env = &(*last)->env; const Expr * expr = &(*last)->expr; - state.debug->onEvalError(&error, *env, *expr); + state.debug->onEvalError(error.get(), *env, *expr); } } - // `EvalState` is the only class that can construct an `EvalErrorBuilder`, - // and it does so in dynamic storage. This is the final method called on - // any such instance and must delete itself before throwing the underlying - // error. - auto error = std::move(this->error); - delete this; - - throw error; + throw *error; } template class EvalErrorBuilder; diff --git a/lix/libexpr/eval-error.hh b/lix/libexpr/eval-error.hh index b05feb827..77513943c 100644 --- a/lix/libexpr/eval-error.hh +++ b/lix/libexpr/eval-error.hh @@ -1,6 +1,7 @@ #pragma once ///@file +#include "lix/libutil/box_ptr.hh" #include "lix/libutil/error.hh" #include "lix/libutil/types.hh" #include "lix/libexpr/pos-idx.hh" @@ -51,13 +52,8 @@ public: } }; -/** - * `EvalErrorBuilder`s may only be constructed by `EvalState`. The `debugThrow` - * method must be the final method in any such `EvalErrorBuilder` usage, and it - * handles deleting the object. - */ template -class EvalErrorBuilder final +class [[nodiscard]] EvalErrorBuilder final { friend class EvalState; @@ -65,35 +61,35 @@ class EvalErrorBuilder final template explicit EvalErrorBuilder(EvalState & state, const Args &... args) - : state(state), error(T(args...)) + : state(state), error(make_box_ptr(args...)) { } public: - T error; + box_ptr error; - [[nodiscard, gnu::noinline]] EvalErrorBuilder & withExitStatus(unsigned int exitStatus); + [[gnu::noinline]] EvalErrorBuilder withExitStatus(unsigned int exitStatus) &&; - [[nodiscard, gnu::noinline]] EvalErrorBuilder & atPos(PosIdx pos); + [[gnu::noinline]] EvalErrorBuilder atPos(PosIdx pos) &&; - [[nodiscard, gnu::noinline]] EvalErrorBuilder & atPos(Value & value, PosIdx fallback = noPos); + [[gnu::noinline]] EvalErrorBuilder atPos(Value & value, PosIdx fallback = noPos) &&; - [[nodiscard, gnu::noinline]] EvalErrorBuilder & withTrace(PosIdx pos, const std::string_view text); + [[gnu::noinline]] EvalErrorBuilder withTrace(PosIdx pos, const std::string_view text) &&; - [[nodiscard, gnu::noinline]] EvalErrorBuilder & withSuggestions(Suggestions & s); + [[gnu::noinline]] EvalErrorBuilder withSuggestions(Suggestions & s) &&; - [[nodiscard, gnu::noinline]] EvalErrorBuilder & withFrame(const Env & e, const Expr & ex); + [[gnu::noinline]] EvalErrorBuilder withFrame(const Env & e, const Expr & ex) &&; - [[nodiscard, gnu::noinline]] EvalErrorBuilder & addTrace(PosIdx pos, HintFmt hint); + [[gnu::noinline]] EvalErrorBuilder addTrace(PosIdx pos, HintFmt hint) &&; template - [[nodiscard, gnu::noinline]] EvalErrorBuilder & - addTrace(PosIdx pos, std::string_view formatString, const Args &... formatArgs); + [[gnu::noinline]] EvalErrorBuilder + addTrace(PosIdx pos, std::string_view formatString, const Args &... formatArgs) &&; /** - * Delete the `EvalErrorBuilder` and throw the underlying exception. + * Throw the underlying exception. */ - [[gnu::noinline, gnu::noreturn]] void debugThrow(); + [[gnu::noinline, gnu::noreturn]] void debugThrow() &&; }; } diff --git a/lix/libexpr/eval.hh b/lix/libexpr/eval.hh index 4906975ae..e35ec35b1 100644 --- a/lix/libexpr/eval.hh +++ b/lix/libexpr/eval.hh @@ -391,10 +391,9 @@ public: std::unique_ptr debug; template - [[nodiscard, gnu::noinline]] - EvalErrorBuilder & error(const Args & ... args) { - // `EvalErrorBuilder::debugThrow` performs the corresponding `delete`. - return *new EvalErrorBuilder(*this, args...); + [[gnu::noinline]] + EvalErrorBuilder error(const Args & ... args) { + return EvalErrorBuilder(*this, args...); } private: