From 5b9be961d84a57dd1736c47f171f2d63272276f4 Mon Sep 17 00:00:00 2001 From: Julian Mundhahs Date: Fri, 17 Jan 2025 12:30:08 +0100 Subject: [PATCH] code review --- src/engine/Server.cpp | 51 +++++++++++++++++----------------- src/engine/Server.h | 14 ++++------ src/index/DeltaTriples.cpp | 39 ++++++++++++++++++-------- src/index/DeltaTriples.h | 3 +- src/index/IndexImpl.cpp | 2 +- test/DeltaTriplesCountTest.cpp | 1 - test/ServerTest.cpp | 2 +- 7 files changed, 63 insertions(+), 49 deletions(-) diff --git a/src/engine/Server.cpp b/src/engine/Server.cpp index 4180916194..115d3014cc 100644 --- a/src/engine/Server.cpp +++ b/src/engine/Server.cpp @@ -883,11 +883,15 @@ Awaitable Server::processQuery( co_return; } -json Server::createResponseMetadata( +json Server::createResponseMetadataForUpdate( const ad_utility::Timer& requestTimer, const Index& index, const DeltaTriples& deltaTriples, const PlannedQuery& plannedQuery, const QueryExecutionTree& qet, const DeltaTriplesCount& countBefore, const UpdateMetadata& updateMetadata, const DeltaTriplesCount& countAfter) { + auto formatTime = [](std::chrono::milliseconds time) { + return absl::StrCat(time.count(), "ms"); + }; + json response; response["update"] = plannedQuery.parsedQuery_._originalString; response["status"] = "OK"; @@ -907,20 +911,19 @@ json Server::createResponseMetadata( response["delta-triples"]["difference"] = nlohmann::json(countAfter - countBefore); response["time"]["planing"] = - absl::StrCat(runtimeInfoWholeOp.timeQueryPlanning.count(), "ms"); + formatTime(runtimeInfoWholeOp.timeQueryPlanning); response["time"]["where"] = - absl::StrCat(runtimeInfo.totalTime_.count() / 1000, "ms"); + formatTime(std::chrono::duration_cast( + runtimeInfo.totalTime_)); json updateTime{ - {"total", absl::StrCat(updateMetadata.triplePreparationTime_.count() + - updateMetadata.deletionTime_.count() + - updateMetadata.insertionTime_.count(), - "ms")}, - {"triplePreparation", - absl::StrCat(updateMetadata.triplePreparationTime_.count(), "ms")}, - {"deletion", absl::StrCat(updateMetadata.deletionTime_.count(), "ms")}, - {"insertion", absl::StrCat(updateMetadata.insertionTime_.count(), "ms")}}; + {"total", formatTime(updateMetadata.triplePreparationTime_ + + updateMetadata.deletionTime_ + + updateMetadata.insertionTime_)}, + {"triplePreparation", formatTime(updateMetadata.triplePreparationTime_)}, + {"deletion", formatTime(updateMetadata.deletionTime_)}, + {"insertion", formatTime(updateMetadata.insertionTime_)}}; response["time"]["update"] = updateTime; - response["time"]["total"] = absl::StrCat(requestTimer.msecs().count(), "ms"); + response["time"]["total"] = formatTime(requestTimer.msecs()); for (auto permutation : Permutation::ALL) { response["located-triples"][Permutation::toString( permutation)]["blocks-affected"] = @@ -976,10 +979,9 @@ json Server::processUpdateImpl( // cache key). cache_.clearAll(); - auto response = - createResponseMetadata(requestTimer, index_, deltaTriples, plannedQuery, - qet, countBefore, updateMetadata, countAfter); - return response; + return createResponseMetadataForUpdate(requestTimer, index_, deltaTriples, + plannedQuery, qet, countBefore, + updateMetadata, countAfter); } // ____________________________________________________________________________ @@ -999,27 +1001,24 @@ Awaitable Server::processUpdate( updateThreadPool_, [this, ¶ms, &update, &requestTimer, &timeLimit, &messageSender, &cancellationHandle] { - std::optional response; // Update the delta triples. - index_.deltaTriplesManager().modify( + return index_.deltaTriplesManager().modify( [this, ¶ms, &update, &requestTimer, &timeLimit, &messageSender, - &cancellationHandle, &response](auto& deltaTriples) { + &cancellationHandle](auto& deltaTriples) { // Use `this` explicitly to silence false-positive errors on // captured `this` being unused. - response = this->processUpdateImpl( - params, update, requestTimer, timeLimit, messageSender, - cancellationHandle, deltaTriples); + return this->processUpdateImpl(params, update, requestTimer, + timeLimit, messageSender, + cancellationHandle, deltaTriples); }); - return response; }, cancellationHandle); auto response = co_await std::move(coroutine); // SPARQL 1.1 Protocol 2.2.4 Successful Responses: "The response body of a // successful update request is implementation defined." - AD_CORRECTNESS_CHECK(response.has_value()); - co_await send(ad_utility::httpUtils::createJsonResponse( - std::move(response.value()), request)); + co_await send( + ad_utility::httpUtils::createJsonResponse(std::move(response), request)); co_return; } diff --git a/src/engine/Server.h b/src/engine/Server.h index abbf45b88b..6d56a13b9f 100644 --- a/src/engine/Server.h +++ b/src/engine/Server.h @@ -153,14 +153,12 @@ class Server { TimeLimit timeLimit); // For an executed update create a json with some stats on the update (timing, // number of changed triples, etc.). - static json createResponseMetadata(const ad_utility::Timer& requestTimer, - const Index& index, - const DeltaTriples& deltaTriples, - const PlannedQuery& plannedQuery, - const QueryExecutionTree& qet, - const DeltaTriplesCount& countBefore, - const UpdateMetadata& updateMetadata, - const DeltaTriplesCount& countAfter); + static json createResponseMetadataForUpdate( + const ad_utility::Timer& requestTimer, const Index& index, + const DeltaTriples& deltaTriples, const PlannedQuery& plannedQuery, + const QueryExecutionTree& qet, const DeltaTriplesCount& countBefore, + const UpdateMetadata& updateMetadata, + const DeltaTriplesCount& countAfter); FRIEND_TEST(ServerTest, createResponseMetadata); // Do the actual execution of an update. Awaitable processUpdate( diff --git a/src/index/DeltaTriples.cpp b/src/index/DeltaTriples.cpp index 23776adfa9..c9b01ac21a 100644 --- a/src/index/DeltaTriples.cpp +++ b/src/index/DeltaTriples.cpp @@ -216,24 +216,41 @@ DeltaTriplesManager::DeltaTriplesManager(const IndexImpl& index) currentLocatedTriplesSnapshot_{deltaTriples_.wlock()->getSnapshot()} {} // _____________________________________________________________________________ -void DeltaTriplesManager::modify( - const std::function& function) { +template +ReturnType DeltaTriplesManager::modify( + const std::function& function) { // While holding the lock for the underlying `DeltaTriples`, perform the // actual `function` (typically some combination of insert and delete // operations) and (while still holding the lock) update the // `currentLocatedTriplesSnapshot_`. - deltaTriples_.withWriteLock([this, &function](DeltaTriples& deltaTriples) { - function(deltaTriples); - auto newSnapshot = deltaTriples.getSnapshot(); - currentLocatedTriplesSnapshot_.withWriteLock( - [&newSnapshot](auto& currentSnapshot) { - currentSnapshot = std::move(newSnapshot); - }); - }); + return deltaTriples_.withWriteLock( + [this, &function](DeltaTriples& deltaTriples) { + auto updateSnapshot = [this, &deltaTriples] { + auto newSnapshot = deltaTriples.getSnapshot(); + currentLocatedTriplesSnapshot_.withWriteLock( + [&newSnapshot](auto& currentSnapshot) { + currentSnapshot = std::move(newSnapshot); + }); + }; + + if constexpr (std::is_void_v) { + function(deltaTriples); + updateSnapshot(); + } else { + ReturnType returnValue = function(deltaTriples); + updateSnapshot(); + return returnValue; + } + }); } +// Explicit instantions +template void DeltaTriplesManager::modify( + std::function const&); +template nlohmann::json DeltaTriplesManager::modify( + const std::function&); // _____________________________________________________________________________ -void DeltaTriplesManager::clear() { modify(&DeltaTriples::clear); } +void DeltaTriplesManager::clear() { modify(&DeltaTriples::clear); } // _____________________________________________________________________________ SharedLocatedTriplesSnapshot DeltaTriplesManager::getCurrentSnapshot() const { diff --git a/src/index/DeltaTriples.h b/src/index/DeltaTriples.h index 23ebe9472e..372cf3ddd8 100644 --- a/src/index/DeltaTriples.h +++ b/src/index/DeltaTriples.h @@ -224,7 +224,8 @@ class DeltaTriplesManager { // serialized, and each call to `getCurrentSnapshot` will either return the // snapshot before or after a modification, but never one of an ongoing // modification. - void modify(const std::function& function); + template + ReturnType modify(const std::function& function); // Reset the updates represented by the underlying `DeltaTriples` and then // update the current snapshot. diff --git a/src/index/IndexImpl.cpp b/src/index/IndexImpl.cpp index 4f5ce915fe..d3a80005c7 100644 --- a/src/index/IndexImpl.cpp +++ b/src/index/IndexImpl.cpp @@ -890,7 +890,7 @@ void IndexImpl::createFromOnDiskIndex(const string& onDiskBase) { // `Permutation`class, but we first have to deal with The delta triples for // the additional permutations. auto setMetadata = [this](const Permutation& p) { - deltaTriplesManager().modify([&p](DeltaTriples& deltaTriples) { + deltaTriplesManager().modify([&p](DeltaTriples& deltaTriples) { deltaTriples.setOriginalMetadata(p.permutation(), p.metaData().blockData()); }); diff --git a/test/DeltaTriplesCountTest.cpp b/test/DeltaTriplesCountTest.cpp index 4b2f656ecf..1a72f356de 100644 --- a/test/DeltaTriplesCountTest.cpp +++ b/test/DeltaTriplesCountTest.cpp @@ -4,7 +4,6 @@ // 2025 Julian Mundhahs #include -#include #include "index/DeltaTriples.h" diff --git a/test/ServerTest.cpp b/test/ServerTest.cpp index de83d9a3cb..93369b9b98 100644 --- a/test/ServerTest.cpp +++ b/test/ServerTest.cpp @@ -310,7 +310,7 @@ TEST(ServerTest, createResponseMetadata) { DeltaTriplesCount countAfter = deltaTriples.getCounts(); // Assertions - json metadata = Server::createResponseMetadata( + json metadata = Server::createResponseMetadataForUpdate( requestTimer, index, deltaTriples, plannedQuery, plannedQuery.queryExecutionTree_, countBefore, updateMetadata, countAfter);