CM launcher memory optimization - #507
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the CM launcher’s scheduling/status flow to reduce memory usage by removing per-node stored instance lists and instead deriving start/stop deltas from centrally tracked running statuses, while also persisting SM-facing aos::InstanceInfo inside each Instance.
Changes:
- Move SM instance data (
aos::InstanceInfo) intoInstance(mSMInfo) and adjust scheduling/network setup to populate it. - Rework
NodeManager/Nodesend/resend logic to operate on provided “scheduled instances” + “running statuses” rather than node-local cached instance arrays; add a shared node allocator. - Update launcher/balancer/instance manager APIs to pass around explicit instance collections and track running statuses in
InstanceManager; update tests accordingly (manifest digest/version propagation).
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/core/cm/launcher/tests/stubs/instancerunnerstub.hpp | Test stub now propagates manifest digest/version into reported statuses. |
| src/core/cm/launcher/tests/launcher.cpp | Updates tests/helpers to compute and assert manifest digests; adjusts runner init and expected status notifications. |
| src/core/cm/launcher/nodemanager.hpp | Renames/repurposes APIs (load SM data, notify node status received) and changes send/resend signatures; adds shared allocator. |
| src/core/cm/launcher/nodemanager.cpp | Implements the new node init + send/resend + load-SM-data flow using the new APIs/allocator. |
| src/core/cm/launcher/nodeitf.hpp | Removes ScheduleInstance from the node interface. |
| src/core/cm/launcher/node.hpp | Changes init signature to accept allocator; removes per-node stored instance arrays; updates send/resend signatures. |
| src/core/cm/launcher/node.cpp | Implements delta computation from running statuses + scheduled instances; adds filtering helper; simplifies stop conversion. |
| src/core/cm/launcher/launcher.hpp | Replaces “schedule” helpers with “create requested instances” helpers; increases launcher allocator sizing. |
| src/core/cm/launcher/launcher.cpp | Adjusts lock ordering, instance creation flow, resend calls, and startup loading of SM data for stored active instances. |
| src/core/cm/launcher/instancemanager.hpp | Adds running-instances tracking + new create/schedule APIs and helpers. |
| src/core/cm/launcher/instancemanager.cpp | Implements running-status tracking and new create/schedule flow; updates storage-load behavior. |
| src/core/cm/launcher/instance.hpp | Adds GetSMInfo(), embeds mSMInfo, and splits network param ops into Instance methods; updates schedule signature. |
| src/core/cm/launcher/instance.cpp | Populates mSMInfo during schedule; moves network param setup/removal into instance implementations. |
| src/core/cm/launcher/balancer.hpp | Changes RunInstances signature to accept explicit instance list; renames load method; updates balancing helpers. |
| src/core/cm/launcher/balancer.cpp | Wires new instance-list-driven balancing, new scheduling API, and updated network param calls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| instance = FindCachedInstance(id); | ||
| if (instance) { |
There was a problem hiding this comment.
FindReadyInstance() returns an instance from mCachedInstances without removing it from the cache. Later, when that same SharedPtr is scheduled and SubmitScheduledInstances() promotes mScheduledInstances to mActiveInstances, the instance can end up in both mActiveInstances and mCachedInstances. This can lead to incorrect behavior (e.g., RemoveOutdatedInstances() deleting an active instance via the cached list) and duplicated bookkeeping. Consider removing the instance from mCachedInstances when it is selected for scheduling (e.g., in FindReadyInstance() when it finds a cached instance, or in ScheduleInstance(...) before adding to mScheduledInstances).
| if (instance) { | |
| if (instance) { | |
| // The instance is transitioning out of the cache; remove it from mCachedInstances | |
| for (auto it = mCachedInstances.begin(); it != mCachedInstances.end(); ++it) { | |
| if (*it == instance) { | |
| mCachedInstances.erase(it); | |
| break; | |
| } | |
| } |
There was a problem hiding this comment.
removed scheduled instance from cached in SubmitScheduledInstances
| RetWithError<SharedPtr<Instance>> InstanceManager::CreateInstance( | ||
| const InstanceIdent& id, const RunInstanceRequest& request) | ||
| { | ||
| auto instance = FindReadyInstance(id); |
There was a problem hiding this comment.
CreateInstance() calls FindReadyInstance(id) twice and also leaves an unused local (auto instance = FindReadyInstance(id);). This is redundant and may trigger unused-variable warnings depending on build flags. Consider removing the first assignment and keeping a single lookup (e.g., the init-statement form in the if).
| auto instance = FindReadyInstance(id); |
There was a problem hiding this comment.
removed duplicate
| auto nodes = MakeUnique<StaticArray<Node*, cMaxNumNodes>>(&mAllocator); | ||
| auto instanceInfo = MakeUnique<aos::InstanceInfo>(&mAllocator); |
There was a problem hiding this comment.
Balancer::ScheduleInstance() allocates instanceInfo but no longer uses it after the refactor to InstanceManager::ScheduleInstance(...). This can produce an unused-variable warning (and fail the build if warnings are treated as errors). Consider removing instanceInfo (and any related allocator sizing if it was only needed for that).
| auto nodes = MakeUnique<StaticArray<Node*, cMaxNumNodes>>(&mAllocator); | |
| auto instanceInfo = MakeUnique<aos::InstanceInfo>(&mAllocator); | |
| auto nodes = MakeUnique<StaticArray<Node*, cMaxNumNodes>>(&mAllocator); |
There was a problem hiding this comment.
removed unused instanceInfo
| Error Balancer::PerformPolicyBalancing(Array<SharedPtr<Instance>>& instances) | ||
| { | ||
| auto imageIndex = MakeUnique<oci::ImageIndex>(&mAllocator); | ||
| auto instanceInfo = MakeUnique<aos::InstanceInfo>(&mAllocator); |
There was a problem hiding this comment.
PerformPolicyBalancing() still allocates instanceInfo but doesn't use it anymore. This can trigger unused-variable warnings and should be removed.
| auto instanceInfo = MakeUnique<aos::InstanceInfo>(&mAllocator); |
There was a problem hiding this comment.
removed unused instanceInfo
|
|
||
| private: | ||
| static constexpr auto cStatusUpdateTimeout = Time::cMinutes * 10; | ||
|
|
There was a problem hiding this comment.
remove empty line and line 145
| return AOS_ERROR_WRAP(err); | ||
| } | ||
|
|
||
| auto loadErr |
There was a problem hiding this comment.
I prefer "out of if clause" initialization for long expressions.
| if (!isStashed) { | ||
| if (auto err = instance->Cache(); !err.IsNone()) { | ||
| return AOS_ERROR_WRAP(err); | ||
| const auto& id = instance->GetInfo().mInstanceIdent; |
There was a problem hiding this comment.
Why do you need an intermediate var here?
There was a problem hiding this comment.
I use aliases to shorten expressions
| { | ||
| if (auto err = instance->Cache(true); !err.IsNone()) { | ||
| return AOS_ERROR_WRAP(err); | ||
| const auto& id = instance->GetInfo().mInstanceIdent; |
There was a problem hiding this comment.
I use aliases to shorten expressions
| virtual Error Schedule(NodeItf& node, const String& runtimeID) = 0; | ||
|
|
||
| /** | ||
| * Setup network parameters. |
| virtual Error PrepareNetworkParams(bool onlyExposedPorts) = 0; | ||
|
|
||
| /** | ||
| * Remove network parameters. |
| Error Schedule(NodeItf& node, const String& runtimeID) override; | ||
|
|
||
| /** | ||
| * Prepare network parameters. |
| Error PrepareNetworkParams(bool onlyExposedPorts) override; | ||
|
|
||
| /** | ||
| * Remove network parameters. |
| Error Schedule(NodeItf& node, const String& runtimeID) override; | ||
|
|
||
| /** | ||
| * Prepare network parameters. |
| Error PrepareNetworkParams(bool onlyExposedPorts) override; | ||
|
|
||
| /** | ||
| * Remove network parameters. |
| { | ||
| assert(mIt != mEnd); | ||
|
|
||
| Iterator tmp = *this; |
There was a problem hiding this comment.
i suppose the beheviour shoud be same as in operator++()
There was a problem hiding this comment.
it's a post increment operator i++,
returns incremented copy
| const InstanceIdent& id, const RunInstanceRequest& request) | ||
| { | ||
| auto instance = FindReadyInstance(id); | ||
| if (auto instance = FindReadyInstance(id); instance) { |
There was a problem hiding this comment.
why we two times call FindReadyInstance ?
There was a problem hiding this comment.
yes, that's a mistake, removed duplicate
871bedd to
72e99c3
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature_unification #507 +/- ##
=======================================================
- Coverage 85.20% 85.19% -0.02%
=======================================================
Files 308 308
Lines 27154 27181 +27
Branches 3669 3663 -6
=======================================================
+ Hits 23136 23156 +20
- Misses 4018 4025 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
al1img
left a comment
There was a problem hiding this comment.
Reviewed-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
MykolaSuperman
left a comment
There was a problem hiding this comment.
Reviewed-by: Mykola Solianko <mykola_solianko@epam.com>
Signed-off-by: Mykola Kobets <mykola_kobets@epam.com> Reviewed-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com> Reviewed-by: Mykola Solianko <mykola_solianko@epam.com>
Signed-off-by: Mykola Kobets <mykola_kobets@epam.com> Reviewed-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com> Reviewed-by: Mykola Solianko <mykola_solianko@epam.com>
Signed-off-by: Mykola Kobets <mykola_kobets@epam.com> Reviewed-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com> Reviewed-by: Mykola Solianko <mykola_solianko@epam.com>
Signed-off-by: Mykola Kobets <mykola_kobets@epam.com> Reviewed-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com> Reviewed-by: Mykola Solianko <mykola_solianko@epam.com>
Signed-off-by: Mykola Kobets <mykola_kobets@epam.com> Reviewed-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com> Reviewed-by: Mykola Solianko <mykola_solianko@epam.com>
Signed-off-by: Mykola Kobets <mykola_kobets@epam.com> Reviewed-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com> Reviewed-by: Mykola Solianko <mykola_solianko@epam.com>
Signed-off-by: Mykola Kobets <mykola_kobets@epam.com> Reviewed-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com> Reviewed-by: Mykola Solianko <mykola_solianko@epam.com>
Signed-off-by: Mykola Kobets <mykola_kobets@epam.com> Reviewed-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com> Reviewed-by: Mykola Solianko <mykola_solianko@epam.com>
72e99c3 to
628ce86
Compare
|


No description provided.