From ea6f2214e646cdbcacb2b53a9b8ccbf7b99a2e8f Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Tue, 25 Feb 2025 22:13:19 +0100 Subject: [PATCH] libstore: remove Store::addMultipleToStore(Source&, ...) only RemoteStore and the daemon loop ever call this method, and the remote store call site isn't even particularly well done. we do not have to create a bitstream, check whether our remote understands it at all, only to parse it again if the remote doesn't understand it. we can instead just use data we already have and avoid many copies. Change-Id: Ib20740df16555269cbb19c7b12937280a00fc1f0 --- lix/libstore/daemon.cc | 13 ++++++-- lix/libstore/remote-store.cc | 61 ++++++++++++++++-------------------- lix/libstore/remote-store.hh | 5 --- lix/libstore/store-api.cc | 21 ------------- lix/libstore/store-api.hh | 5 --- 5 files changed, 37 insertions(+), 68 deletions(-) diff --git a/lix/libstore/daemon.cc b/lix/libstore/daemon.cc index dea4cdf14..2555bd816 100644 --- a/lix/libstore/daemon.cc +++ b/lix/libstore/daemon.cc @@ -516,9 +516,16 @@ static void performOp(AsyncIoRoot & aio, TunnelLogger * logger, ref store logger->startWork(); { FramedSource source(from); - aio.blockOn(store->addMultipleToStore(source, - RepairFlag{repair}, - dontCheckSigs ? NoCheckSigs : CheckSigs)); + auto expected = readNum(source); + for (uint64_t i = 0; i < expected; ++i) { + auto info = WorkerProto::Serialise::read( + *store, WorkerProto::ReadConn{source, clientVersion} + ); + info.ultimate = false; // duplicated in RemoteStore::addMultipleToStore + aio.blockOn(store->addToStore( + info, source, RepairFlag{repair}, dontCheckSigs ? NoCheckSigs : CheckSigs + )); + } } logger->stopWork(); break; diff --git a/lix/libstore/remote-store.cc b/lix/libstore/remote-store.cc index e0022ea23..1ec3e83e7 100644 --- a/lix/libstore/remote-store.cc +++ b/lix/libstore/remote-store.cc @@ -515,40 +515,29 @@ kj::Promise> RemoteStore::addMultipleToStore( Activity & act, RepairFlag repair, CheckSigsFlag checkSigs) -try { - auto remoteVersion = getProtocol(); - - GeneratorSource source{ - [](auto self, auto & pathsToCopy, auto remoteVersion) -> WireFormatGenerator { - co_yield pathsToCopy.size(); - for (auto & [pathInfo, pathSource] : pathsToCopy) { - co_yield WorkerProto::Serialise::write(*self, - WorkerProto::WriteConn {remoteVersion}, - pathInfo); - try { - char buf[65536]; - while (true) { - const auto read = pathSource->read(buf, sizeof(buf)); - co_yield std::span{buf, read}; - } - } catch (EndOfFile &) { - } - } - }(this, pathsToCopy, remoteVersion) - }; - - TRY_AWAIT(addMultipleToStore(source, repair, checkSigs)); - co_return result::success(); -} catch (...) { - co_return result::current_exception(); -} - -kj::Promise> RemoteStore::addMultipleToStore( - Source & source, - RepairFlag repair, - CheckSigsFlag checkSigs) try { if (GET_PROTOCOL_MINOR(getConnection()->daemonVersion) >= 32) { + auto remoteVersion = getProtocol(); + + GeneratorSource source{ + [](auto self, auto & pathsToCopy, auto remoteVersion) -> WireFormatGenerator { + co_yield pathsToCopy.size(); + for (auto & [pathInfo, pathSource] : pathsToCopy) { + co_yield WorkerProto::Serialise::write(*self, + WorkerProto::WriteConn {remoteVersion}, + pathInfo); + try { + char buf[65536]; + while (true) { + const auto read = pathSource->read(buf, sizeof(buf)); + co_yield std::span{buf, read}; + } + } catch (EndOfFile &) { + } + } + }(this, pathsToCopy, remoteVersion) + }; + auto conn(getConnection()); conn->to << WorkerProto::Op::AddMultipleToStore @@ -557,8 +546,12 @@ try { conn.withFramedSink([&](Sink & sink) { source.drainInto(sink); }); - } else - TRY_AWAIT(Store::addMultipleToStore(source, repair, checkSigs)); + } else { + for (auto & [pathInfo, pathSource] : pathsToCopy) { + pathInfo.ultimate = false; // duplicated in daemon.cc AddMultipleToStore + TRY_AWAIT(addToStore(pathInfo, *pathSource, repair, checkSigs)); + } + } co_return result::success(); } catch (...) { co_return result::current_exception(); diff --git a/lix/libstore/remote-store.hh b/lix/libstore/remote-store.hh index fab835d95..fecb9e66b 100644 --- a/lix/libstore/remote-store.hh +++ b/lix/libstore/remote-store.hh @@ -93,11 +93,6 @@ public: kj::Promise> addToStore(const ValidPathInfo & info, Source & nar, RepairFlag repair, CheckSigsFlag checkSigs) override; - kj::Promise> addMultipleToStore( - Source & source, - RepairFlag repair, - CheckSigsFlag checkSigs) override; - kj::Promise> addMultipleToStore( PathsSource & pathsToCopy, Activity & act, diff --git a/lix/libstore/store-api.cc b/lix/libstore/store-api.cc index 72f40f4e9..d4acafc5b 100644 --- a/lix/libstore/store-api.cc +++ b/lix/libstore/store-api.cc @@ -368,27 +368,6 @@ try { co_return result::current_exception(); } -kj::Promise> Store::addMultipleToStore( - Source & source, - RepairFlag repair, - CheckSigsFlag checkSigs) -try { - auto remoteVersion = getProtocol(); - - auto expected = readNum(source); - for (uint64_t i = 0; i < expected; ++i) { - // FIXME we should not be using the worker protocol here at all! - auto info = WorkerProto::Serialise::read(*this, - WorkerProto::ReadConn {source, remoteVersion} - ); - info.ultimate = false; - TRY_AWAIT(addToStore(info, source, repair, checkSigs)); - } - co_return result::success(); -} catch (...) { - co_return result::current_exception(); -} - namespace { /** * If the NAR archive contains a single file at top-level, then save diff --git a/lix/libstore/store-api.hh b/lix/libstore/store-api.hh index cb2568db1..aca843101 100644 --- a/lix/libstore/store-api.hh +++ b/lix/libstore/store-api.hh @@ -511,11 +511,6 @@ public: /** * Import multiple paths into the store. */ - virtual kj::Promise> addMultipleToStore( - Source & source, - RepairFlag repair = NoRepair, - CheckSigsFlag checkSigs = CheckSigs); - virtual kj::Promise> addMultipleToStore( PathsSource & pathsToCopy, Activity & act,