From cbcb434cb3eb9b647b7f0e8c22dbb526f5599849 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Sun, 31 Aug 2025 13:55:11 +0200 Subject: [PATCH] libexpr: Convert Symbol comparisons to switch statements Now that Symbols are statically allocated at compile time with known IDs, we can use switch statements instead of if-else chains for Symbol comparisons. This provides better performance through compiler optimizations like jump tables. Changes: - Add public getId() method to Symbol class to access the internal ID - Convert if-else chains comparing Symbol values to switch statements in primops.cc's derivationStrictInternal function - Simplify control flow by removing the 'handled' flag and moving the default attribute handling into the switch's default case The static and runtime Symbol IDs are guaranteed to match by the copyIntoSymbolTable implementation which asserts this invariant. Co-authored-by: John Ericson --- src/libexpr/include/nix/expr/symbol-table.hh | 9 ++ src/libexpr/primops.cc | 99 +++++++++++++------- 2 files changed, 75 insertions(+), 33 deletions(-) diff --git a/src/libexpr/include/nix/expr/symbol-table.hh b/src/libexpr/include/nix/expr/symbol-table.hh index ff98077ca..9a9cbae61 100644 --- a/src/libexpr/include/nix/expr/symbol-table.hh +++ b/src/libexpr/include/nix/expr/symbol-table.hh @@ -61,6 +61,15 @@ public: return id > 0; } + /** + * The ID is a private implementation detail that should generally not be observed. However, we expose here just for + * sake of `switch...case`, which needs to dispatch on numbers. */ + [[gnu::always_inline]] + constexpr uint32_t getId() const noexcept + { + return id; + } + constexpr auto operator<=>(const Symbol & other) const noexcept = default; friend class std::hash; diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 264f3d155..c6cdf09a1 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -1454,19 +1454,22 @@ static void derivationStrictInternal(EvalState & state, std::string_view drvName continue; } - if (i->name == state.s.contentAddressed && state.forceBool(*i->value, pos, context_below)) { - contentAddressed = true; - experimentalFeatureSettings.require(Xp::CaDerivations); - } - - else if (i->name == state.s.impure && state.forceBool(*i->value, pos, context_below)) { - isImpure = true; - experimentalFeatureSettings.require(Xp::ImpureDerivations); - } - + switch (i->name.getId()) { + case EvalState::s.contentAddressed.getId(): + if (state.forceBool(*i->value, pos, context_below)) { + contentAddressed = true; + experimentalFeatureSettings.require(Xp::CaDerivations); + } + break; + case EvalState::s.impure.getId(): + if (state.forceBool(*i->value, pos, context_below)) { + isImpure = true; + experimentalFeatureSettings.require(Xp::ImpureDerivations); + } + break; /* The `args' attribute is special: it supplies the command-line arguments to the builder. */ - else if (i->name == state.s.args) { + case EvalState::s.args.getId(): state.forceList(*i->value, pos, context_below); for (auto elem : i->value->listView()) { auto s = state @@ -1475,11 +1478,10 @@ static void derivationStrictInternal(EvalState & state, std::string_view drvName .toOwned(); drv.args.push_back(s); } - } - + break; /* All other attributes are passed to the builder through the environment. */ - else { + default: if (jsonObject) { @@ -1488,49 +1490,69 @@ static void derivationStrictInternal(EvalState & state, std::string_view drvName jsonObject->structuredAttrs.emplace(key, printValueAsJSON(state, true, *i->value, pos, context)); - if (i->name == state.s.builder) + switch (i->name.getId()) { + case EvalState::s.builder.getId(): drv.builder = state.forceString(*i->value, context, pos, context_below); - else if (i->name == state.s.system) + break; + case EvalState::s.system.getId(): drv.platform = state.forceStringNoCtx(*i->value, pos, context_below); - else if (i->name == state.s.outputHash) + break; + case EvalState::s.outputHash.getId(): outputHash = state.forceStringNoCtx(*i->value, pos, context_below); - else if (i->name == state.s.outputHashAlgo) + break; + case EvalState::s.outputHashAlgo.getId(): outputHashAlgo = parseHashAlgoOpt(state.forceStringNoCtx(*i->value, pos, context_below)); - else if (i->name == state.s.outputHashMode) + break; + case EvalState::s.outputHashMode.getId(): handleHashMode(state.forceStringNoCtx(*i->value, pos, context_below)); - else if (i->name == state.s.outputs) { - /* Require ‘outputs’ to be a list of strings. */ + break; + case EvalState::s.outputs.getId(): { + /* Require 'outputs' to be a list of strings. */ state.forceList(*i->value, pos, context_below); Strings ss; for (auto elem : i->value->listView()) ss.emplace_back(state.forceStringNoCtx(*elem, pos, context_below)); handleOutputs(ss); + break; + } + default: + break; } - if (i->name == state.s.allowedReferences) + switch (i->name.getId()) { + case EvalState::s.allowedReferences.getId(): warn( "In a derivation named '%s', 'structuredAttrs' disables the effect of the derivation attribute 'allowedReferences'; use 'outputChecks..allowedReferences' instead", drvName); - if (i->name == state.s.allowedRequisites) + break; + case EvalState::s.allowedRequisites.getId(): warn( "In a derivation named '%s', 'structuredAttrs' disables the effect of the derivation attribute 'allowedRequisites'; use 'outputChecks..allowedRequisites' instead", drvName); - if (i->name == state.s.disallowedReferences) + break; + case EvalState::s.disallowedReferences.getId(): warn( "In a derivation named '%s', 'structuredAttrs' disables the effect of the derivation attribute 'disallowedReferences'; use 'outputChecks..disallowedReferences' instead", drvName); - if (i->name == state.s.disallowedRequisites) + break; + case EvalState::s.disallowedRequisites.getId(): warn( "In a derivation named '%s', 'structuredAttrs' disables the effect of the derivation attribute 'disallowedRequisites'; use 'outputChecks..disallowedRequisites' instead", drvName); - if (i->name == state.s.maxSize) + break; + case EvalState::s.maxSize.getId(): warn( "In a derivation named '%s', 'structuredAttrs' disables the effect of the derivation attribute 'maxSize'; use 'outputChecks..maxSize' instead", drvName); - if (i->name == state.s.maxClosureSize) + break; + case EvalState::s.maxClosureSize.getId(): warn( "In a derivation named '%s', 'structuredAttrs' disables the effect of the derivation attribute 'maxClosureSize'; use 'outputChecks..maxClosureSize' instead", drvName); + break; + default: + break; + } } else { auto s = state.coerceToString(pos, *i->value, context, context_below, true).toOwned(); @@ -1541,20 +1563,31 @@ static void derivationStrictInternal(EvalState & state, std::string_view drvName drv.structuredAttrs = StructuredAttrs::parse(s); } else { drv.env.emplace(key, s); - if (i->name == state.s.builder) + switch (i->name.getId()) { + case EvalState::s.builder.getId(): drv.builder = std::move(s); - else if (i->name == state.s.system) + break; + case EvalState::s.system.getId(): drv.platform = std::move(s); - else if (i->name == state.s.outputHash) + break; + case EvalState::s.outputHash.getId(): outputHash = std::move(s); - else if (i->name == state.s.outputHashAlgo) + break; + case EvalState::s.outputHashAlgo.getId(): outputHashAlgo = parseHashAlgoOpt(s); - else if (i->name == state.s.outputHashMode) + break; + case EvalState::s.outputHashMode.getId(): handleHashMode(s); - else if (i->name == state.s.outputs) + break; + case EvalState::s.outputs.getId(): handleOutputs(tokenizeString(s)); + break; + default: + break; + } } } + break; } } catch (Error & e) {