From ff1e955bde8373381ef235f6facd4ba6047e5c23 Mon Sep 17 00:00:00 2001 From: Martchus Date: Sun, 4 Apr 2021 22:20:47 +0200 Subject: [PATCH] Consider concurrent flag when starting build actions from task This allows concurrent build actions within the same task. --- librepomgr/buildactions/buildaction.cpp | 15 ++- librepomgr/buildactions/buildaction.h | 1 + librepomgr/webapi/routes.cpp | 143 ++++++++++++++++-------- 3 files changed, 111 insertions(+), 48 deletions(-) diff --git a/librepomgr/buildactions/buildaction.cpp b/librepomgr/buildactions/buildaction.cpp index 279c67e..094f898 100644 --- a/librepomgr/buildactions/buildaction.cpp +++ b/librepomgr/buildactions/buildaction.cpp @@ -219,7 +219,8 @@ bool BuildAction::haveSucceeded(const std::vector> } /*! - * \brief Starts the build action. The caller must acquire the lock to write build actions. + * \brief Starts the build action. The caller must acquire the lock to write build actions if + * the build action is setup-globally visible. * \returns Returns immediately. The real work is done in a build action thread. */ void BuildAction::start(ServiceSetup &setup) @@ -298,6 +299,16 @@ void BuildAction::startAfterOtherBuildActions(ServiceSetup &setup, const std::ve } } +void BuildAction::assignStartAfter(const std::vector> &startsAfterBuildActions) +{ + for (auto &previousBuildAction : startsAfterBuildActions) { + startAfter.emplace_back(previousBuildAction->id); + if (!previousBuildAction->hasSucceeded()) { + previousBuildAction->m_followUpActions.emplace_back(weak_from_this()); + } + } +} + void BuildAction::abort() { m_aborted.store(true); @@ -341,7 +352,7 @@ void BuildAction::conclude(BuildActionResult result) i = m_bufferingForSession.erase(i); } - // start follow-up actions if succeeded + // start globally visible follow-up actions if succeeded if (result == BuildActionResult::Success && m_setup) { for (auto &maybeStillValidFollowUpAction : m_followUpActions) { auto followUpAction = maybeStillValidFollowUpAction.lock(); diff --git a/librepomgr/buildactions/buildaction.h b/librepomgr/buildactions/buildaction.h index 57caad3..540cf37 100644 --- a/librepomgr/buildactions/buildaction.h +++ b/librepomgr/buildactions/buildaction.h @@ -180,6 +180,7 @@ public: bool isAborted() const; void start(ServiceSetup &setup); void startAfterOtherBuildActions(ServiceSetup &setup, const std::vector> &startsAfterBuildActions); + void assignStartAfter(const std::vector> &startsAfterBuildActions); void abort(); void appendOutput(std::string &&output); void appendOutput(std::string_view output); diff --git a/librepomgr/webapi/routes.cpp b/librepomgr/webapi/routes.cpp index d2f732f..abc9b1f 100644 --- a/librepomgr/webapi/routes.cpp +++ b/librepomgr/webapi/routes.cpp @@ -653,23 +653,30 @@ void postBuildAction(const Params ¶ms, ResponseHandler &&handler) handler(makeJson(params.request(), response)); } +struct SequencedBuildActions { + std::string_view name; + std::vector, SequencedBuildActions>> actions; + bool concurrent = false; +}; + static std::string allocateNewBuildAction(const BuildActionMetaInfo &metaInfo, const std::string &taskName, const std::vector &packageNames, const std::string &directory, - const std::unordered_map &actionTemplates, std::vector> &newBuildActions, - const std::string &actionName) + const std::unordered_map &actionTemplates, SequencedBuildActions &newActionSequence, + std::vector> &allocatedActions, const std::string &actionTemplateToAllocate) { - const auto actionTemplateIterator = actionTemplates.find(actionName); + const auto actionTemplateIterator = actionTemplates.find(actionTemplateToAllocate); if (actionTemplateIterator == actionTemplates.end()) { - return "the action \"" % actionName + "\" of the specified task is not configured"; + return "the action \"" % actionTemplateToAllocate + "\" of the specified task is not configured"; } const auto &actionTemplate = actionTemplateIterator->second; const auto buildActionType = actionTemplate.type; if (!metaInfo.isTypeIdValid(buildActionType)) { - return argsToString( - "the type \"", static_cast(buildActionType), "\" of action \"", actionName, "\" of the specified task is invalid"); + return argsToString("the type \"", static_cast(buildActionType), "\" of action \"", actionTemplateToAllocate, + "\" of the specified task is invalid"); } const auto &typeInfo = metaInfo.typeInfoForId(actionTemplate.type); - auto &buildAction = newBuildActions.emplace_back(std::make_shared()); // a real ID is set later + auto &allocatedBuildAction = allocatedActions.emplace_back(std::make_shared()); + auto *const buildAction = std::get>(newActionSequence.actions.emplace_back(allocatedBuildAction)).get(); buildAction->taskName = taskName; buildAction->directory = !typeInfo.directory || directory.empty() ? actionTemplate.directory : directory; buildAction->type = buildActionType; @@ -683,15 +690,19 @@ static std::string allocateNewBuildAction(const BuildActionMetaInfo &metaInfo, c static std::string allocateNewBuildActionSequence(const BuildActionMetaInfo &metaInfo, const std::string &taskName, const std::vector &packageNames, const std::string &directory, - const std::unordered_map &actionTemplates, std::vector> &newBuildActions, - const BuildActionSequence &actionSequence) + const std::unordered_map &actionTemplates, SequencedBuildActions &newActionSequence, + std::vector> &allocatedActions, const BuildActionSequence &actionSequenceToAllocate) { auto error = std::string(); - for (const auto &actionNode : actionSequence.actions) { - if (const auto *const actionName = std::get_if(&actionNode)) { - error = allocateNewBuildAction(metaInfo, taskName, packageNames, directory, actionTemplates, newBuildActions, *actionName); + newActionSequence.name = actionSequenceToAllocate.name; + newActionSequence.concurrent = actionSequenceToAllocate.concurrent; + for (const auto &actionNode : actionSequenceToAllocate.actions) { + if (const auto *const actionTemplateName = std::get_if(&actionNode)) { + error = allocateNewBuildAction( + metaInfo, taskName, packageNames, directory, actionTemplates, newActionSequence, allocatedActions, *actionTemplateName); } else if (const auto *const actionSequence = std::get_if(&actionNode)) { - error = allocateNewBuildActionSequence(metaInfo, taskName, packageNames, directory, actionTemplates, newBuildActions, *actionSequence); + error = allocateNewBuildActionSequence(metaInfo, taskName, packageNames, directory, actionTemplates, + std::get(newActionSequence.actions.emplace_back(SequencedBuildActions())), allocatedActions, *actionSequence); } if (!error.empty()) { return error; @@ -700,6 +711,61 @@ static std::string allocateNewBuildActionSequence(const BuildActionMetaInfo &met return error; } +static std::vector> allocateBuildActionIDs(ServiceSetup &setup, + const std::vector> &startAfterActions, const std::vector> &parentLevelActions, + SequencedBuildActions &newActionSequence) +{ + auto previousActions = std::vector>(); + for (auto &sequencedAction : newActionSequence.actions) { + // make concurrent actions depend on the parent-level actions (or actions specified via start after ID parameters on top-level) + // make the first action within a sequence depend on the parent-level actions (or actions specified via start after ID parameters on top-level) + // make further actions within a sequence depend on the previous action within the sequence + const auto &relevantLastBuildActions = newActionSequence.concurrent || previousActions.empty() ? parentLevelActions : previousActions; + if (auto *const action = std::get_if>(&sequencedAction)) { + (*action)->id = setup.building.allocateBuildActionID(); + (*action)->assignStartAfter(relevantLastBuildActions.empty() ? startAfterActions : relevantLastBuildActions); + if (!newActionSequence.concurrent) { + previousActions.clear(); // only the last action within the sequence is relevant + } + previousActions.emplace_back(*action); + } else if (auto *const subSequence = std::get_if(&sequencedAction)) { + auto newSubActions = allocateBuildActionIDs(setup, startAfterActions, relevantLastBuildActions, *subSequence); + if (!newSubActions.empty()) { + if (!newActionSequence.concurrent) { + previousActions.clear(); // only the last action within the sequence is relevant + } + if (subSequence->concurrent) { + // consider all new sub-actions from last level relevant when they're concurrent + previousActions.insert(previousActions.end(), newSubActions.begin(), newSubActions.end()); + } else { + previousActions.emplace_back(newSubActions.back()); + } + } + } + } + return previousActions; +} + +static bool startFirstBuildActions(ServiceSetup &setup, SequencedBuildActions &newActionSequence) +{ + for (auto &sequencedAction : newActionSequence.actions) { + if (auto *const maybeAction = std::get_if>(&sequencedAction)) { + auto &action = *maybeAction; + if (action->isScheduled()) { + action->start(setup); + } + if (!newActionSequence.concurrent) { + return true; + } + } else if (auto *const subSequence = std::get_if(&sequencedAction)) { + if (startFirstBuildActions(setup, newActionSequence) && !newActionSequence.concurrent) { + return true; + } + } + } + return false; +} + void postBuildActionsFromTask(const Params ¶ms, ResponseHandler &&handler, const std::string &taskName, const std::string &directory, const std::vector &startAfterIds, bool startImmediately) { @@ -736,13 +802,17 @@ void postBuildActionsFromTask(const Params ¶ms, ResponseHandler &&handler, c } // allocate a vector to store build actions (temporarily) in - auto newBuildActions = std::vector>(); - newBuildActions.reserve(task.actions.size()); + auto newActionSequence = SequencedBuildActions(); + auto allocatedActions = std::vector>(); + auto parentLevelActions = std::vector>(); + newActionSequence.actions.reserve(task.actions.size()); + allocatedActions.reserve(task.actions.size()); // copy data from templates into new build actions auto &metaInfo = params.setup.building.metaInfo; auto metaInfoLock = metaInfo.lockToRead(); - auto error = allocateNewBuildActionSequence(metaInfo, taskName, packageNames, directory, actionTemplates, newBuildActions, task); + auto error + = allocateNewBuildActionSequence(metaInfo, taskName, packageNames, directory, actionTemplates, newActionSequence, allocatedActions, task); metaInfoLock.unlock(); setupLock.unlock(); if (!error.empty()) { @@ -750,46 +820,27 @@ void postBuildActionsFromTask(const Params ¶ms, ResponseHandler &&handler, c return; } - // allocate build action IDs and populate "start after ID" - BuildAction *lastBuildAction = nullptr; + // allocate build action IDs and populate "start after ID" and follow-up actions auto &building = params.setup.building; auto buildLock = building.lockToWrite(); auto startsAfterBuildActions = building.getBuildActions(startAfterIds); - const auto startNow = startImmediately || (!startsAfterBuildActions.empty() && BuildAction::haveSucceeded(startsAfterBuildActions)); - for (auto &newBuildAction : newBuildActions) { - newBuildAction->id = building.allocateBuildActionID(); - if (lastBuildAction) { - newBuildAction->startAfter.emplace_back(lastBuildAction->id); - } else { - newBuildAction->startAfter = startAfterIds; - } - lastBuildAction = newBuildAction.get(); - } + allocateBuildActionIDs(params.setup, startsAfterBuildActions, parentLevelActions, newActionSequence); buildLock.unlock(); // serialize build actions - const auto response = ReflectiveRapidJSON::JsonReflector::toJsonDocument(newBuildActions); + auto buildReadLock = building.lockToRead(); + const auto startNow = startImmediately || (!startsAfterBuildActions.empty() && BuildAction::haveSucceeded(startsAfterBuildActions)); + const auto response = ReflectiveRapidJSON::JsonReflector::toJsonDocument(allocatedActions); - // start first build action immediately + // start first build action immediately (read-lock sufficient because build action not part of setup-global list yet) if (startNow) { - newBuildActions.front()->start(params.setup); + startFirstBuildActions(params.setup, newActionSequence); } - // add build actions to setup-global list and populate "follow-up actions" - buildLock = building.lockToWrite(); - lastBuildAction = nullptr; - for (auto &newBuildAction : newBuildActions) { - if (lastBuildAction) { - if (lastBuildAction->hasSucceeded()) { - newBuildAction->start(params.setup); - } else { - lastBuildAction->m_followUpActions.emplace_back(newBuildAction->weak_from_this()); - } - } else if (!startsAfterBuildActions.empty()) { - newBuildAction->startAfterOtherBuildActions(params.setup, startsAfterBuildActions); - } - lastBuildAction = newBuildAction.get(); - building.actions[lastBuildAction->id] = std::move(newBuildAction); + // add build actions to setup-global list + buildLock = building.lockToWrite(buildReadLock); + for (auto &newAction : allocatedActions) { + building.actions[newAction->id] = std::move(newAction); } buildLock.unlock();