From 474695975dde60f582ca0b2fb72c17f664e22876 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 26 Apr 2022 14:01:21 +0200 Subject: [PATCH 1/2] EvalCache: Revert to using symbols in getAttr() --- src/libcmd/installables.cc | 2 +- src/libexpr/eval-cache.cc | 38 +++++++++++++++++++++++++------------- src/libexpr/eval-cache.hh | 8 ++++++-- src/nix/app.cc | 6 +++--- src/nix/flake.cc | 2 +- src/nix/search.cc | 2 +- 6 files changed, 37 insertions(+), 21 deletions(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 693b5e2a2..e3210d18d 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -600,7 +600,7 @@ std::tuple InstallableF auto drvInfo = DerivationInfo { std::move(drvPath), - attr->getAttr("outputName")->getString() + attr->getAttr(state->sOutputName)->getString() }; return {attrPath, getLockedFlake()->flake.lockedRef, std::move(drvInfo)}; diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc index 00e80ae3b..e90c9bff5 100644 --- a/src/libexpr/eval-cache.cc +++ b/src/libexpr/eval-cache.cc @@ -420,8 +420,10 @@ Suggestions AttrCursor::getSuggestionsForAttr(Symbol name) return Suggestions::bestMatches(strAttrNames, root->state.symbols[name]); } -std::shared_ptr AttrCursor::maybeGetAttr(std::string_view name, bool forceErrors) +std::shared_ptr AttrCursor::maybeGetAttr(Symbol name, bool forceErrors) { + auto nameS = root->state.symbols[name]; + if (root->db) { if (!cachedValue) cachedValue = root->db->getAttr(getKey(), root->state.symbols); @@ -429,11 +431,11 @@ std::shared_ptr AttrCursor::maybeGetAttr(std::string_view name, bool if (cachedValue) { if (auto attrs = std::get_if>(&cachedValue->second)) { for (auto & attr : *attrs) - if (root->state.symbols[attr] == name) + if (attr == name) return std::make_shared(root, std::make_pair(shared_from_this(), attr)); return nullptr; } else if (std::get_if(&cachedValue->second)) { - auto attr = root->db->getAttr({cachedValue->first, std::string(name)}, root->state.symbols); + auto attr = root->db->getAttr({cachedValue->first, nameS}, root->state.symbols); if (attr) { if (std::get_if(&attr->second)) return nullptr; @@ -441,10 +443,10 @@ std::shared_ptr AttrCursor::maybeGetAttr(std::string_view name, bool if (forceErrors) debug("reevaluating failed cached attribute '%s'"); else - throw CachedEvalError("cached failure of attribute '%s'", getAttrPathStr(root->state.symbols.create(name))); + throw CachedEvalError("cached failure of attribute '%s'", getAttrPathStr(name)); } else return std::make_shared(root, - std::make_pair(shared_from_this(), root->state.symbols.create(name)), nullptr, std::move(attr)); + std::make_pair(shared_from_this(), name), nullptr, std::move(attr)); } // Incomplete attrset, so need to fall thru and // evaluate to see whether 'name' exists @@ -465,13 +467,13 @@ std::shared_ptr AttrCursor::maybeGetAttr(std::string_view name, bool root->db->setPlaceholder({cachedValue->first, root->state.symbols[attr.name]}); } - auto attr = v.attrs->get(root->state.symbols.create(name)); + auto attr = v.attrs->get(name); if (!attr) { if (root->db) { if (!cachedValue) cachedValue = {root->db->setPlaceholder(getKey()), placeholder_t()}; - root->db->setMissing({cachedValue->first, std::string(name)}); + root->db->setMissing({cachedValue->first, nameS}); } return nullptr; } @@ -480,26 +482,36 @@ std::shared_ptr AttrCursor::maybeGetAttr(std::string_view name, bool if (root->db) { if (!cachedValue) cachedValue = {root->db->setPlaceholder(getKey()), placeholder_t()}; - cachedValue2 = {root->db->setPlaceholder({cachedValue->first, std::string(name)}), placeholder_t()}; + cachedValue2 = {root->db->setPlaceholder({cachedValue->first, nameS}), placeholder_t()}; } return make_ref( - root, std::make_pair(shared_from_this(), root->state.symbols.create(name)), attr->value, std::move(cachedValue2)); + root, std::make_pair(shared_from_this(), name), attr->value, std::move(cachedValue2)); } -ref AttrCursor::getAttr(std::string_view name, bool forceErrors) +std::shared_ptr AttrCursor::maybeGetAttr(std::string_view name) +{ + return maybeGetAttr(root->state.symbols.create(name)); +} + +ref AttrCursor::getAttr(Symbol name, bool forceErrors) { auto p = maybeGetAttr(name, forceErrors); if (!p) - throw Error("attribute '%s' does not exist", getAttrPathStr(root->state.symbols.create(name))); + throw Error("attribute '%s' does not exist", getAttrPathStr(name)); return ref(p); } +ref AttrCursor::getAttr(std::string_view name) +{ + return getAttr(root->state.symbols.create(name)); +} + OrSuggestions> AttrCursor::findAlongAttrPath(const std::vector & attrPath, bool force) { auto res = shared_from_this(); for (auto & attr : attrPath) { - auto child = res->maybeGetAttr(root->state.symbols[attr], force); + auto child = res->maybeGetAttr(attr, force); if (!child) { auto suggestions = res->getSuggestionsForAttr(attr); return OrSuggestions>::failed(suggestions); @@ -627,7 +639,7 @@ bool AttrCursor::isDerivation() StorePath AttrCursor::forceDerivation() { - auto aDrvPath = getAttr("drvPath", true); + auto aDrvPath = getAttr(root->state.sDrvPath, true); auto drvPath = root->state.store->parseStorePath(aDrvPath->getString()); if (!root->state.store->isValidPath(drvPath) && !settings.readOnlyMode) { /* The eval cache contains 'drvPath', but the actual path has diff --git a/src/libexpr/eval-cache.hh b/src/libexpr/eval-cache.hh index 288ecd7e3..0ba2e4ed0 100644 --- a/src/libexpr/eval-cache.hh +++ b/src/libexpr/eval-cache.hh @@ -96,9 +96,13 @@ public: Suggestions getSuggestionsForAttr(Symbol name); - std::shared_ptr maybeGetAttr(std::string_view name, bool forceErrors = false); + std::shared_ptr maybeGetAttr(Symbol name, bool forceErrors = false); - ref getAttr(std::string_view name, bool forceErrors = false); + std::shared_ptr maybeGetAttr(std::string_view name); + + ref getAttr(Symbol name, bool forceErrors = false); + + ref getAttr(std::string_view name); /* Get an attribute along a chain of attrsets. Note that this does not auto-call functors or functions. */ diff --git a/src/nix/app.cc b/src/nix/app.cc index eec53ad7c..cce84d026 100644 --- a/src/nix/app.cc +++ b/src/nix/app.cc @@ -85,9 +85,9 @@ UnresolvedApp Installable::toApp(EvalState & state) else if (type == "derivation") { auto drvPath = cursor->forceDerivation(); - auto outPath = cursor->getAttr("outPath")->getString(); - auto outputName = cursor->getAttr("outputName")->getString(); - auto name = cursor->getAttr("name")->getString(); + auto outPath = cursor->getAttr(state.sOutPath)->getString(); + auto outputName = cursor->getAttr(state.sOutputName)->getString(); + auto name = cursor->getAttr(state.sName)->getString(); auto aPname = cursor->maybeGetAttr("pname"); auto aMeta = cursor->maybeGetAttr("meta"); auto aMainProgram = aMeta ? aMeta->maybeGetAttr("mainProgram") : nullptr; diff --git a/src/nix/flake.cc b/src/nix/flake.cc index 2c4a64c85..040c1c7af 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -1012,7 +1012,7 @@ struct CmdFlakeShow : FlakeCommand, MixJSON auto showDerivation = [&]() { - auto name = visitor.getAttr("name")->getString(); + auto name = visitor.getAttr(state->sName)->getString(); if (json) { std::optional description; if (auto aMeta = visitor.maybeGetAttr("meta")) { diff --git a/src/nix/search.cc b/src/nix/search.cc index 6febc0a55..76451f810 100644 --- a/src/nix/search.cc +++ b/src/nix/search.cc @@ -156,7 +156,7 @@ struct CmdSearch : InstallableCommand, MixJSON recurse(); else if (attrPathS[0] == "legacyPackages" && attrPath.size() > 2) { - auto attr = cursor.maybeGetAttr("recurseForDerivations"); + auto attr = cursor.maybeGetAttr(state->sRecurseForDerivations); if (attr && attr->getBool()) recurse(); } From b12c33510cb09f7d8300d7f3c762a84b8688780f Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 26 Apr 2022 14:16:20 +0200 Subject: [PATCH 2/2] EvalCache AttrKey: Use Symbol instead of std::string --- src/libexpr/eval-cache.cc | 69 +++++++++++++++++++------------------ src/libexpr/eval-cache.hh | 2 +- src/libexpr/symbol-table.hh | 2 ++ 3 files changed, 39 insertions(+), 34 deletions(-) diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc index e90c9bff5..6fb4bf266 100644 --- a/src/libexpr/eval-cache.cc +++ b/src/libexpr/eval-cache.cc @@ -35,9 +35,15 @@ struct AttrDb std::unique_ptr> _state; - AttrDb(const Store & cfg, const Hash & fingerprint) + SymbolTable & symbols; + + AttrDb( + const Store & cfg, + const Hash & fingerprint, + SymbolTable & symbols) : cfg(cfg) , _state(std::make_unique>()) + , symbols(symbols) { auto state(_state->lock()); @@ -92,7 +98,6 @@ struct AttrDb AttrId setAttrs( AttrKey key, - const SymbolTable & symbols, const std::vector & attrs) { return doSQLite([&]() @@ -101,7 +106,7 @@ struct AttrDb state->insertAttribute.use() (key.first) - (key.second) + (symbols[key.second]) (AttrType::FullAttrs) (0, false).exec(); @@ -136,14 +141,14 @@ struct AttrDb } state->insertAttributeWithContext.use() (key.first) - (key.second) + (symbols[key.second]) (AttrType::String) (s) (ctx).exec(); } else { state->insertAttribute.use() (key.first) - (key.second) + (symbols[key.second]) (AttrType::String) (s).exec(); } @@ -162,7 +167,7 @@ struct AttrDb state->insertAttribute.use() (key.first) - (key.second) + (symbols[key.second]) (AttrType::Bool) (b ? 1 : 0).exec(); @@ -178,7 +183,7 @@ struct AttrDb state->insertAttribute.use() (key.first) - (key.second) + (symbols[key.second]) (AttrType::Placeholder) (0, false).exec(); @@ -194,7 +199,7 @@ struct AttrDb state->insertAttribute.use() (key.first) - (key.second) + (symbols[key.second]) (AttrType::Missing) (0, false).exec(); @@ -210,7 +215,7 @@ struct AttrDb state->insertAttribute.use() (key.first) - (key.second) + (symbols[key.second]) (AttrType::Misc) (0, false).exec(); @@ -226,7 +231,7 @@ struct AttrDb state->insertAttribute.use() (key.first) - (key.second) + (symbols[key.second]) (AttrType::Failed) (0, false).exec(); @@ -234,13 +239,11 @@ struct AttrDb }); } - std::optional> getAttr( - AttrKey key, - SymbolTable & symbols) + std::optional> getAttr(AttrKey key) { auto state(_state->lock()); - auto queryAttribute(state->queryAttribute.use()(key.first)(key.second)); + auto queryAttribute(state->queryAttribute.use()(key.first)(symbols[key.second])); if (!queryAttribute.next()) return {}; auto rowId = (AttrType) queryAttribute.getInt(0); @@ -278,10 +281,13 @@ struct AttrDb } }; -static std::shared_ptr makeAttrDb(const Store & cfg, const Hash & fingerprint) +static std::shared_ptr makeAttrDb( + const Store & cfg, + const Hash & fingerprint, + SymbolTable & symbols) { try { - return std::make_shared(cfg, fingerprint); + return std::make_shared(cfg, fingerprint, symbols); } catch (SQLiteError &) { ignoreException(); return nullptr; @@ -292,7 +298,7 @@ EvalCache::EvalCache( std::optional> useCache, EvalState & state, RootLoader rootLoader) - : db(useCache ? makeAttrDb(*state.store, *useCache) : nullptr) + : db(useCache ? makeAttrDb(*state.store, *useCache, state.symbols) : nullptr) , state(state) , rootLoader(rootLoader) { @@ -326,13 +332,12 @@ AttrCursor::AttrCursor( AttrKey AttrCursor::getKey() { if (!parent) - return {0, {""}}; + return {0, root->state.sEpsilon}; if (!parent->first->cachedValue) { - parent->first->cachedValue = root->db->getAttr( - parent->first->getKey(), root->state.symbols); + parent->first->cachedValue = root->db->getAttr(parent->first->getKey()); assert(parent->first->cachedValue); } - return {parent->first->cachedValue->first, root->state.symbols[parent->second]}; + return {parent->first->cachedValue->first, parent->second}; } Value & AttrCursor::getValue() @@ -422,11 +427,9 @@ Suggestions AttrCursor::getSuggestionsForAttr(Symbol name) std::shared_ptr AttrCursor::maybeGetAttr(Symbol name, bool forceErrors) { - auto nameS = root->state.symbols[name]; - if (root->db) { if (!cachedValue) - cachedValue = root->db->getAttr(getKey(), root->state.symbols); + cachedValue = root->db->getAttr(getKey()); if (cachedValue) { if (auto attrs = std::get_if>(&cachedValue->second)) { @@ -435,7 +438,7 @@ std::shared_ptr AttrCursor::maybeGetAttr(Symbol name, bool forceErro return std::make_shared(root, std::make_pair(shared_from_this(), attr)); return nullptr; } else if (std::get_if(&cachedValue->second)) { - auto attr = root->db->getAttr({cachedValue->first, nameS}, root->state.symbols); + auto attr = root->db->getAttr({cachedValue->first, name}); if (attr) { if (std::get_if(&attr->second)) return nullptr; @@ -464,7 +467,7 @@ std::shared_ptr AttrCursor::maybeGetAttr(Symbol name, bool forceErro for (auto & attr : *v.attrs) { if (root->db) - root->db->setPlaceholder({cachedValue->first, root->state.symbols[attr.name]}); + root->db->setPlaceholder({cachedValue->first, attr.name}); } auto attr = v.attrs->get(name); @@ -473,7 +476,7 @@ std::shared_ptr AttrCursor::maybeGetAttr(Symbol name, bool forceErro if (root->db) { if (!cachedValue) cachedValue = {root->db->setPlaceholder(getKey()), placeholder_t()}; - root->db->setMissing({cachedValue->first, nameS}); + root->db->setMissing({cachedValue->first, name}); } return nullptr; } @@ -482,7 +485,7 @@ std::shared_ptr AttrCursor::maybeGetAttr(Symbol name, bool forceErro if (root->db) { if (!cachedValue) cachedValue = {root->db->setPlaceholder(getKey()), placeholder_t()}; - cachedValue2 = {root->db->setPlaceholder({cachedValue->first, nameS}), placeholder_t()}; + cachedValue2 = {root->db->setPlaceholder({cachedValue->first, name}), placeholder_t()}; } return make_ref( @@ -525,7 +528,7 @@ std::string AttrCursor::getString() { if (root->db) { if (!cachedValue) - cachedValue = root->db->getAttr(getKey(), root->state.symbols); + cachedValue = root->db->getAttr(getKey()); if (cachedValue && !std::get_if(&cachedValue->second)) { if (auto s = std::get_if(&cachedValue->second)) { debug("using cached string attribute '%s'", getAttrPathStr()); @@ -547,7 +550,7 @@ string_t AttrCursor::getStringWithContext() { if (root->db) { if (!cachedValue) - cachedValue = root->db->getAttr(getKey(), root->state.symbols); + cachedValue = root->db->getAttr(getKey()); if (cachedValue && !std::get_if(&cachedValue->second)) { if (auto s = std::get_if(&cachedValue->second)) { bool valid = true; @@ -580,7 +583,7 @@ bool AttrCursor::getBool() { if (root->db) { if (!cachedValue) - cachedValue = root->db->getAttr(getKey(), root->state.symbols); + cachedValue = root->db->getAttr(getKey()); if (cachedValue && !std::get_if(&cachedValue->second)) { if (auto b = std::get_if(&cachedValue->second)) { debug("using cached Boolean attribute '%s'", getAttrPathStr()); @@ -602,7 +605,7 @@ std::vector AttrCursor::getAttrs() { if (root->db) { if (!cachedValue) - cachedValue = root->db->getAttr(getKey(), root->state.symbols); + cachedValue = root->db->getAttr(getKey()); if (cachedValue && !std::get_if(&cachedValue->second)) { if (auto attrs = std::get_if>(&cachedValue->second)) { debug("using cached attrset attribute '%s'", getAttrPathStr()); @@ -626,7 +629,7 @@ std::vector AttrCursor::getAttrs() }); if (root->db) - cachedValue = {root->db->setAttrs(getKey(), root->state.symbols, attrs), attrs}; + cachedValue = {root->db->setAttrs(getKey(), attrs), attrs}; return attrs; } diff --git a/src/libexpr/eval-cache.hh b/src/libexpr/eval-cache.hh index 0ba2e4ed0..b0709ebc2 100644 --- a/src/libexpr/eval-cache.hh +++ b/src/libexpr/eval-cache.hh @@ -51,7 +51,7 @@ struct missing_t {}; struct misc_t {}; struct failed_t {}; typedef uint64_t AttrId; -typedef std::pair AttrKey; +typedef std::pair AttrKey; typedef std::pair string_t; typedef std::variant< diff --git a/src/libexpr/symbol-table.hh b/src/libexpr/symbol-table.hh index 63fb25d73..288c15602 100644 --- a/src/libexpr/symbol-table.hh +++ b/src/libexpr/symbol-table.hh @@ -72,12 +72,14 @@ private: ChunkedVector store{16}; public: + Symbol create(std::string_view s) { // Most symbols are looked up more than once, so we trade off insertion performance // for lookup performance. // TODO: could probably be done more efficiently with transparent Hash and Equals // on the original implementation using unordered_set + // FIXME: make this thread-safe. auto it = symbols.find(s); if (it != symbols.end()) return Symbol(it->second.second + 1);