[CPU] Enable memory reuse for nested graphs #27521

EgorDuplensky · 2024-11-12T10:53:06Z

Details:

All the nested graphs are now must be a part of a global memory reuse logic
The core logic of the memory reuse is untouched (a bit refactored)
Instead of solving memory reuse for every graph / subgraph, now all the edges and global execution indices are collected from the "virtually flatten" graph first and then memory reuse is solved once for a model.
All the nodes with nested graphs are updated, including:
1. LoRa
2. Composite
3. If
4. TensorIterator
5. Convolution + Sum fallback subgraph

Tickets:

ticket-id

EgorDuplensky · 2024-11-13T13:47:42Z

@maxnick Ready for review. Could you please take a look?

EgorDuplensky · 2024-11-14T10:51:34Z

Added a fix for Convolution + Sum fallback graph.
Now such graph is also a part of memory reuse.

src/plugins/intel_cpu/src/compiled_model.cpp

src/plugins/intel_cpu/src/edge.h

src/plugins/intel_cpu/src/graph.h

maxnick · 2024-11-22T15:14:38Z

src/plugins/intel_cpu/src/node.h

+    virtual bool canBeSkipped() const {
+        return getSelectedPrimitiveDescriptor()->hasZeroInputDims();
+    }


The name is rather vague. Where can it be skipped from? Apparently we need to change name to make it more clear.
What about isNop?

Let's move this naming discussion to the end of the queue.

Is not the right time?

@maxnick Lets revive the discussion, since all the other discussions are finished.
After taking a fresh look into this, I think the main idea is that here we decide that the node should not be added to the list of the executable ones. We can express it in a verbose way, like:

neverExecute()

shouldNeverBeExecuted

includeIntoExecutables

I am ok with isNop as well, but it does not really express that the node must never be executed. NoOp in general can be executed, it just does nothing.

From what you've proposed I like neverExecute().

src/plugins/intel_cpu/src/nodes/composite.cpp

src/plugins/intel_cpu/src/nodes/conv.cpp

src/plugins/intel_cpu/src/nodes/lora.cpp

src/plugins/intel_cpu/tests/functional/cmake/target_per_test.cmake

src/plugins/intel_cpu/src/graph.cpp

src/plugins/intel_cpu/src/nodes/lora.cpp

src/plugins/intel_cpu/src/graph.cpp

src/plugins/intel_cpu/src/nodes/conv.cpp

github-actions · 2024-12-26T00:23:08Z

This PR will be closed in a week because of 2 weeks of no activity.

github-actions · 2025-01-17T00:22:58Z

This PR will be closed in a week because of 2 weeks of no activity.

src/plugins/intel_cpu/src/graph.h

maxnick · 2025-01-28T14:54:27Z

src/plugins/intel_cpu/src/graph.cpp

+    if (memoryControl->allocated()) {
+        return;  // memory is already allocated globally


To my understanding, Activate() for the subgraph must be called in creaetPrimitive or prepareParams methods. Also the Allocate() method is called only inside Activate before CreatePrimitivesAndExecConstants(). Thus the code below is executed only for the top most graph as it calls memoryControl->allocateMemory() and this if branch is always taken for subgraphs.
Moreover, do I correctly understand that if a node with a subgraph by some reason calls Activate for a subgraph before it's called for the main graph (any stage of Configure). The whole memory management subsystem will be broken, as the rest of this method code will not be invoked.
If so, it's just another indicator that we need to consider as special subgraph type.

The problem here is the way the legacy graph pipeline stages are implemented.
With a proper pipeline such situation will not be possible.

Are there any plans of implementing the proper pipeline? How it should look like?

src/plugins/intel_cpu/src/graph.cpp

src/plugins/intel_cpu/src/graph_context.h

maxnick · 2025-01-28T15:00:42Z

src/plugins/intel_cpu/src/node.h

+    virtual bool canBeSkipped() const {
+        return getSelectedPrimitiveDescriptor()->hasZeroInputDims();
+    }


src/plugins/intel_cpu/src/compiled_model.h

src/plugins/intel_cpu/src/nodes/memory.cpp

EgorDuplensky · 2025-01-21T19:08:01Z

src/plugins/intel_cpu/src/graph_context.cpp

+      m_auxiliaryNetworkMemoryControl(std::make_shared<NetworkMemoryControl>()),
+      m_memoryControl(m_auxiliaryNetworkMemoryControl->createMemoryControlUnit()) {


@dmitry-gorokhov I decided to not create a wrapper around m_auxiliaryNetworkMemoryControl and m_memoryControl.
Instead, it is redesigned, so m_memoryControl is just one memory control instance of m_auxiliaryNetworkMemoryControl

EgorDuplensky · 2025-01-21T19:17:54Z

src/plugins/intel_cpu/src/node.h

+    virtual bool canBeSkipped() const {
+        return getSelectedPrimitiveDescriptor()->hasZeroInputDims();
+    }


@maxnick Lets revive the discussion, since all the other discussions are finished.
After taking a fresh look into this, I think the main idea is that here we decide that the node should not be added to the list of the executable ones. We can express it in a verbose way, like:

neverExecute()

shouldNeverBeExecuted

includeIntoExecutables

I am ok with isNop as well, but it does not really express that the node must never be executed. NoOp in general can be executed, it just does nothing.

src/plugins/intel_cpu/src/nodes/lora.cpp

src/plugins/intel_cpu/src/edge.h

src/plugins/intel_cpu/src/infer_request.cpp

EgorDuplensky · 2025-01-28T16:12:51Z

src/plugins/intel_cpu/src/graph.cpp

+    if (memoryControl->allocated()) {
+        return;  // memory is already allocated globally


The problem here is the way the legacy graph pipeline stages are implemented.
With a proper pipeline such situation will not be possible.

src/plugins/intel_cpu/src/graph.cpp

src/plugins/intel_cpu/src/compiled_model.h

src/plugins/intel_cpu/src/nodes/memory.cpp

Sync node indexes must be registered to global allocation context in order.

maxnick · 2025-01-31T15:30:36Z

src/plugins/intel_cpu/src/compiled_model.h

+class NetworkMemoryControl;
+


Looks like this forward declaration is also not needed anymore.

maxnick · 2025-01-31T15:43:04Z

src/plugins/intel_cpu/src/graph.cpp

+    if (memoryControl->allocated()) {
+        return;  // memory is already allocated globally


Are there any plans of implementing the proper pipeline? How it should look like?

maxnick · 2025-01-31T15:45:32Z

src/plugins/intel_cpu/src/graph.cpp

 #include "edge.h"
+#include "graph_context.h"


It's already included in graph.h.

maxnick · 2025-01-31T15:48:03Z

src/plugins/intel_cpu/src/graph.cpp

+ * Partition the \clusters of Edges, by moving to the end and allocating at the same time
+ * the clusters which cannot be handled as part of generic memory solver algorithm.


Suggested change

* Partition the \clusters of Edges, by moving to the end and allocating at the same time

* the clusters which cannot be handled as part of generic memory solver algorithm.

* Partition the \clusters of Edges, by moving to the end and allocating at the same time

* the clusters that cannot be handled as part of the generic memory solver algorithm.

maxnick · 2025-01-31T15:52:53Z

src/plugins/intel_cpu/src/graph.cpp

+                                            [](const EdgePtr& edge) {
+                                                return edge->getOriginalDesc().getPrecision() == element::string;
+                                            }),
+                                "All edges in the cluster must be string.");


Suggested change

"All edges in the cluster must be string.");

"All edges in a string cluster must be strings.");

maxnick · 2025-01-31T16:18:25Z

src/plugins/intel_cpu/src/graph.cpp

+    status = hasDynNodes ? (parallel_get_max_threads() > 1 ? Status::ReadyDynamic : Status::ReadyDynamicSeq)
+                         : Status::ReadyStatic;


This operation is useless as the following if/else block completely redefines the status once again.

src/plugins/intel_cpu/src/graph_context.h

maxnick · 2025-01-31T16:36:43Z

src/plugins/intel_cpu/src/infer_request.cpp

@@ -7,6 +7,7 @@
 #include "async_infer_request.h"
 #include "dnnl_extension_utils.h"
 #include "itt.h"
+#include "memory_control.hpp"


Probably this include is not needed anymore.

maxnick · 2025-01-31T16:44:01Z

src/plugins/intel_cpu/src/nodes/if.cpp

+    const int elseOffset = m_elseGraph.RegisterToAllocationContext(thenOffset, context);
+    return m_elseGraph.RegisterToAllocationContext(elseOffset, context);


What is the reason of registering the m_elseGraph in the allocation context two times?

maxnick · 2025-01-31T16:45:32Z

src/plugins/intel_cpu/src/nodes/if.cpp

-    const std::shared_ptr<const ov::Model>& elseBody = ifOp->get_else_body();
-    subGraphThen.CreateGraph(thenBody, context);
-    subGraphElse.CreateGraph(elseBody, context);
+    auto ifOp = ov::as_type_ptr<ov::op::v8::If>(m_op);


Should we add a null check here?

maxnick · 2025-01-31T16:48:03Z

src/plugins/intel_cpu/src/nodes/input.cpp

@@ -499,10 +499,12 @@ void Input::selectOptimalPrimitiveDescriptor() {
    // ignore previous configuration
    supportedPrimitiveDescriptors.clear();

+    int inPlacePort = m_isInPlace ? 0 : -1;


Suggested change

int inPlacePort = m_isInPlace ? 0 : -1;

const int inPlacePort = m_isInPlace ? 0 : -1;

maxnick · 2025-01-31T16:53:28Z

src/plugins/intel_cpu/src/nodes/memory.cpp

-            subgraphMemoryPtrs.push_back(mem);
-            inputMemory.emplace_back(std::move(mem));
+        CPU_NODE_ASSERT(getParentEdges().size() == subGraph->inputsNumber(),
+                        "Number of node inputs must be equal the number of inner graph's inputs");


Suggested change

"Number of node inputs must be equal the number of inner graph's inputs");

"The number of node inputs must be equal to the number of inner graph's inputs");

maxnick · 2025-01-31T17:09:38Z

src/plugins/intel_cpu/src/nodes/memory.cpp

+            auto subgraphInputNode = subGraph->getInputNodeByIndex(i);
+            auto subgraphInputMemory = subgraphInputNode->getDstMemoryAtPort(0);
+            subgraphMemoryPtrs.push_back(subgraphInputMemory);


Could you please elaborate on how we have valid memory objects at this point, while the memory objects are created during the Activate step?

maxnick · 2025-01-31T17:11:56Z

src/plugins/intel_cpu/src/nodes/memory.cpp

+        CPU_NODE_ASSERT(getParentEdges().size() == subGraph->inputsNumber(),
+                        "Number of node inputs must be equal the number of inner graph's inputs");
+
+        for (size_t i = 0; i < subGraph->inputsNumber(); i++) {


Please note that when the subgraphMemoryPtrs are being used, the traversal goes using getOriginalInputsNumber(). Probably it does make sense to align these two loops.

maxnick · 2025-01-31T17:19:44Z

src/plugins/intel_cpu/src/nodes/lora.cpp

+        auto subgraphInputNode = m_graph.getInputNodeByIndex(i);
+        auto subgraphInputMemory = subgraphInputNode->getDstMemoryAtPort(0);
+        subgraphMemoryPtrs.emplace_back(subgraphInputMemory);


Are the memory objects valid before we call Activate()?

maxnick · 2025-01-31T17:30:26Z

src/plugins/intel_cpu/tests/unit/graph/merge_transpose_reorder_test.cpp

@@ -173,32 +183,33 @@ class MergeTransposeReorderCPUTest : public testing::WithParamInterface<MergeTra

    std::shared_ptr<GraphContext> m_context;
    std::unique_ptr<Graph> m_graph;
+    std::shared_ptr<NetworkMemoryControl> networkMemoryControl = std::make_shared<NetworkMemoryControl>();


It seems that the variable is not used. Could you please revise all the other similar places?

maxnick · 2025-01-31T17:31:08Z

src/plugins/intel_cpu/tests/unit/graph/memory_state.cpp


        Config conf;
        conf.rtCacheCapacity = 0;
-        auto context = std::make_shared<GraphContext>(conf, nullptr, false);
+        std::shared_ptr<NetworkMemoryControl> networkMemoryControl = std::make_shared<NetworkMemoryControl>();


It looks like this variable is not used.

EgorDuplensky requested review from a team as code owners November 12, 2024 10:53

github-actions bot added category: inference OpenVINO Runtime library - Inference category: CPU OpenVINO CPU plugin category: build OpenVINO cmake script / infra labels Nov 12, 2024

EgorDuplensky changed the title ~~Enable memory reuse for nested graps~~ Enable memory reuse for nested graphs Nov 12, 2024

EgorDuplensky changed the title ~~Enable memory reuse for nested graphs~~ [CPU] Enable memory reuse for nested graphs Nov 12, 2024

EgorDuplensky force-pushed the enable_memory_reuse_for_nested_graphs branch from da1d1a0 to 18563de Compare November 12, 2024 15:27

EgorDuplensky mentioned this pull request Nov 12, 2024

[CPU] Refactor memory control and allocation #27259

Closed

EgorDuplensky force-pushed the enable_memory_reuse_for_nested_graphs branch 2 times, most recently from 4734e61 to e140018 Compare November 13, 2024 13:32

github-actions bot removed the category: inference OpenVINO Runtime library - Inference label Nov 13, 2024

EgorDuplensky assigned maxnick Nov 13, 2024

EgorDuplensky force-pushed the enable_memory_reuse_for_nested_graphs branch from e140018 to d5bda07 Compare November 13, 2024 13:51

maxnick requested changes Nov 22, 2024

View reviewed changes

EgorDuplensky force-pushed the enable_memory_reuse_for_nested_graphs branch 2 times, most recently from 408e748 to 11a5115 Compare November 25, 2024 15:28

maxnick reviewed Dec 3, 2024

View reviewed changes

src/plugins/intel_cpu/src/graph.cpp Outdated Show resolved Hide resolved

src/plugins/intel_cpu/src/graph.cpp Outdated Show resolved Hide resolved

src/plugins/intel_cpu/src/graph.cpp Outdated Show resolved Hide resolved

maxnick reviewed Dec 4, 2024

View reviewed changes

src/plugins/intel_cpu/src/nodes/lora.cpp Outdated Show resolved Hide resolved

EgorDuplensky force-pushed the enable_memory_reuse_for_nested_graphs branch 2 times, most recently from 7f3fadf to 9e170ae Compare December 10, 2024 17:56

maxnick reviewed Dec 11, 2024

View reviewed changes

src/plugins/intel_cpu/src/graph.cpp Show resolved Hide resolved

src/plugins/intel_cpu/src/nodes/conv.cpp Show resolved Hide resolved

github-actions bot added the Stale label Dec 26, 2024

mg-intel removed the Stale label Jan 2, 2025

github-actions bot added the Stale label Jan 17, 2025

mg-intel removed the Stale label Jan 17, 2025

EgorDuplensky force-pushed the enable_memory_reuse_for_nested_graphs branch 9 times, most recently from 9e2648a to efb84d1 Compare January 21, 2025 19:24

EgorDuplensky force-pushed the enable_memory_reuse_for_nested_graphs branch from af3a926 to 705eabf Compare January 25, 2025 22:43

maxnick self-requested a review January 27, 2025 12:30

maxnick reviewed Jan 28, 2025

View reviewed changes

src/plugins/intel_cpu/src/compiled_model.h Outdated Show resolved Hide resolved

src/plugins/intel_cpu/src/compiled_model.h Outdated Show resolved Hide resolved

src/plugins/intel_cpu/src/nodes/memory.cpp Outdated Show resolved Hide resolved

EgorDuplensky force-pushed the enable_memory_reuse_for_nested_graphs branch 4 times, most recently from ed7ca84 to 467ef3b Compare January 28, 2025 17:32

EgorDuplensky commented Jan 28, 2025

View reviewed changes

EgorDuplensky force-pushed the enable_memory_reuse_for_nested_graphs branch 2 times, most recently from 31120e1 to 9ac1226 Compare January 29, 2025 18:47

EgorDuplensky added 3 commits January 30, 2025 13:18

[CPU] Enable memory reuse for nested graphs

4a3ffae

Fix e2e issue

76c0b47

Sync node indexes must be registered to global allocation context in order.

Apply review comments

2497eb3

EgorDuplensky force-pushed the enable_memory_reuse_for_nested_graphs branch from 9ac1226 to 2497eb3 Compare January 30, 2025 12:19

Fix CI issue

c1d5f75

maxnick reviewed Jan 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CPU] Enable memory reuse for nested graphs #27521

[CPU] Enable memory reuse for nested graphs #27521

EgorDuplensky commented Nov 12, 2024 •

edited

Loading

EgorDuplensky commented Nov 13, 2024

EgorDuplensky commented Nov 14, 2024

maxnick Nov 22, 2024

EgorDuplensky Nov 25, 2024

maxnick Dec 11, 2024

EgorDuplensky Jan 21, 2025

maxnick Jan 28, 2025

EgorDuplensky Jan 28, 2025

maxnick Jan 31, 2025

github-actions bot commented Dec 26, 2024

github-actions bot commented Jan 17, 2025

maxnick Jan 28, 2025

EgorDuplensky Jan 28, 2025

maxnick Jan 31, 2025

maxnick Jan 28, 2025

EgorDuplensky Jan 21, 2025

EgorDuplensky Jan 21, 2025

EgorDuplensky Jan 28, 2025

maxnick Jan 31, 2025

maxnick Jan 31, 2025

maxnick Jan 31, 2025

maxnick Jan 31, 2025

maxnick Jan 31, 2025

maxnick Jan 31, 2025

maxnick Jan 31, 2025

maxnick Jan 31, 2025

maxnick Jan 31, 2025

maxnick Jan 31, 2025

maxnick Jan 31, 2025

maxnick Jan 31, 2025

maxnick Jan 31, 2025

maxnick Jan 31, 2025

maxnick Jan 31, 2025

maxnick Jan 31, 2025

		if (memoryControl->allocated()) {
		return; // memory is already allocated globally

		m_auxiliaryNetworkMemoryControl(std::make_shared<NetworkMemoryControl>()),
		m_memoryControl(m_auxiliaryNetworkMemoryControl->createMemoryControlUnit()) {

		* Partition the \clusters of Edges, by moving to the end and allocating at the same time
		* the clusters which cannot be handled as part of generic memory solver algorithm.

	"All edges in the cluster must be string.");
	"All edges in a string cluster must be strings.");

		status = hasDynNodes ? (parallel_get_max_threads() > 1 ? Status::ReadyDynamic : Status::ReadyDynamicSeq)
		: Status::ReadyStatic;

		const int elseOffset = m_elseGraph.RegisterToAllocationContext(thenOffset, context);
		return m_elseGraph.RegisterToAllocationContext(elseOffset, context);

	int inPlacePort = m_isInPlace ? 0 : -1;
	const int inPlacePort = m_isInPlace ? 0 : -1;

	"Number of node inputs must be equal the number of inner graph's inputs");
	"The number of node inputs must be equal to the number of inner graph's inputs");

[CPU] Enable memory reuse for nested graphs #27521

Are you sure you want to change the base?

[CPU] Enable memory reuse for nested graphs #27521

Conversation

EgorDuplensky commented Nov 12, 2024 • edited Loading

Details:

Tickets:

EgorDuplensky commented Nov 13, 2024

EgorDuplensky commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 26, 2024

github-actions bot commented Jan 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EgorDuplensky commented Nov 12, 2024 •

edited

Loading