SWDEV-556588 - Use linked list for packetBatches_ as graphNode enable… by jaydeeppatel1111 · Pull Request #1128 · ROCm/rocm-systems

jaydeeppatel1111 · 2025-09-25T09:49:45Z

…/disable and set params do impact AQL packets captured in batch.

Motivation

Insert/remove at random place from vector is cost than linked list.

Technical Details

Use linked list for packetBatches_ as graphNode enable/disable and set params do impact AQL packets captured in batch.

Test Plan

Reported tests in SWDEV-556588 passes with this change.

Test Result

Reported tests in SWDEV-556588 passes with this change.

Submission Checklist

Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

…/disable and set params do impact AQL packets captured in batch.

anugodavar · 2025-09-25T15:02:12Z

This method is very inefficient we have to allocate memory to store batches and during launch we have to iterate through the linked list to form a batch. Instead, we can form a batch directly during hipGraphLaunch.

jaydeeppatel1111 · 2025-09-25T16:27:10Z

This method is very inefficient we have to allocate memory to store batches and during launch we have to iterate through the linked list to form a batch. Instead, we can form a batch directly during hipGraphLaunch.

I agree and in fact, I have mentioned to @saleelk over teams yesterday about this. Pros of moving batching to launch is that complication due to setParams and enable/disable can be avoided HOWEVER If I understand, Idea of hipGraphInstantiate is to PREPARE graph (load gun) and launch is like "shoot a gun" so this is more of design decision based on what is more often called hipGraphLaunch/hipGraphInstantiate as this approach would add overhead in hipGraphLaunch. I will discuss with @saleelk and decide.

saleelk · 2025-09-25T18:43:14Z

+        // Insert at head
+        newNode->next = head;
+        newNode->prev = nullptr;
+        if (head) head->prev = newNode;


use proper braces

multiple places here

use proper braces

Done.

multiple places here

Done.

saleelk · 2025-09-25T18:59:30Z

-        i += packetBatches_[batchIndex].capturedNodeCount - 1;  // -1 because loop will increment
+        // We need to count how many nodes are in this batch
+        size_t nodesInBatch = 0;
+        for (size_t j = i; j < topoOrder_.size() && j < nodeCaptureStatus_.size() && nodeCaptureStatus_[j]; ++j) {


I maintained the capturedNodeCount to avoid doing this loop here during the launch.

I maintained the capturedNodeCount to avoid doing this loop here during the launch.

Done.

saleelk · 2025-09-25T19:10:14Z

-    size_t capturedNodeCount;  // Number of consecutive captured nodes in this batch
+  //! Structure for individual packet in LinkedList
+  struct PacketNode {
+    uint8_t* packet;


This could be a vector of packets, ie if each node has more than once packet

This could be a vector of packets, ie if each node has more than once packet

Done.

saleelk · 2025-09-25T19:10:31Z

+  //! Structure for individual packet in LinkedList
+  struct PacketNode {
+    uint8_t* packet;
+    std::string kernelName;


same a vector of kernelNames

same a vector of kernelNames

Done.

saleelk · 2025-09-25T19:11:25Z

  //! Track which nodes were successfully captured (true) vs need individual execution (false)
  std::vector<bool> nodeCaptureStatus_;
+  //! Map from GraphNode to its PacketNodes for O(1) lookup
+  std::unordered_map<GraphNode*, std::vector<PacketNode*>> nodeToPacketsMap_;


if PacketNode already has vector of packets, this map doesnt need a vector of PacketNodes*, simplifies that

if PacketNode already has vector of packets, this map doesnt need a vector of PacketNodes*, simplifies that

Done.

saleelk · 2025-09-25T19:14:37Z

-        packetBatches_.emplace_back(std::move(currentBatch), std::move(currentKernelNames),
-                                    capturedNodeCount);
+      if (newBatch.head != nullptr) {
+        packetBatches_.push_back(std::move(newBatch));


use emplace_back

use emplace_back

Done.

saleelk · 2025-09-25T19:20:58Z

+        // Create PacketNodes for this node's packets
+        std::vector<PacketNode*> nodePacketNodes;
+        for (size_t k = 0; k < nodePackets.size(); ++k) {
+          PacketNode* packetNode = new PacketNode(nodePackets[k], nodeKernelNames[k], 


This would change, since now the constructor can directly accept the packet/kernelName vectors, no loop needed

This would change, since now the constructor can directly accept the packet/kernelName vectors, no loop needed

Done.

saleelk · 2025-09-25T19:22:51Z

+        // Create PacketNodes for this node's packets
+        std::vector<PacketNode*> nodePacketNodes;
+        for (size_t k = 0; k < nodePackets.size(); ++k) {
+          PacketNode* packetNode = new PacketNode(nodePackets[k], nodeKernelNames[k], 


Also the created nodes, where are they released. You are putting this on the heap.

Also the created nodes, where are they released. You are putting this on the heap.

Yeah, it is part of packetBatch_ so when it gets released, dctor for PacketBatch release PacketNodes.

saleelk · 2025-09-25T19:24:44Z

        childNode->SetGraphCaptureStatus(true);
        status = childNode->CaptureAndFormPacketsForGraph();
-        nodeCaptureStatus_[i] = (status == hipSuccess);
+        //nodeCaptureStatus_[i] = (status == hipSuccess);


why is nodeCaptureStatus ignored it,

why is nodeCaptureStatus ignored it,

Originally, I have this change as fix to child graph test as it is observed that if child graph is captured in batch, It is not processed in run. Child graph has its own graphExec. However it looks with recent changes that I have made, this can ve reverted to original.

SWDEV-556588 - Use linked list for packetBatches_ as graphNode enable…

9dfcd21

…/disable and set params do impact AQL packets captured in batch.

jaydeeppatel1111 requested a review from a team as a code owner September 25, 2025 09:49

github-actions Bot added the project: clr label Sep 25, 2025

Remove unused capturedNodeCount var.

d77e075

systems-assistant Bot added the organization: ROCm label Sep 25, 2025

Merge branch 'develop' into users/jaypatel/SWDEV-556588-linkedlist

c281a22

saleelk reviewed Sep 25, 2025

View reviewed changes

jaydeeppatel1111 and others added 3 commits September 26, 2025 10:51

Merge branch 'develop' into users/jaypatel/SWDEV-556588-linkedlist

6b15202

Merge branch 'develop' into users/jaypatel/SWDEV-556588-linkedlist

bb57b4a

556588 - Address review comments.

b8d73f0

jaydeeppatel1111 requested a review from saleelk September 26, 2025 12:19

jaydeeppatel1111 closed this Nov 3, 2025

Conversation

jaydeeppatel1111 commented Sep 25, 2025

Motivation

Technical Details

Test Plan

Test Result

Submission Checklist

Uh oh!

anugodavar commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaydeeppatel1111 commented Sep 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

anugodavar commented Sep 25, 2025 •

edited

Loading