Skip to content

[ISSUE-131] Moving GPU memory management to OperationManager #841

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 34 commits into
base: SharedDevelopment
Choose a base branch
from

Conversation

BenYang2002
Copy link
Contributor

@BenYang2002 BenYang2002 commented Apr 24, 2025

Closes
#131
#825
#821

Description

Summary
This PR completes the integration of the remaining GPU operations — copyFromGPU and deallocateGPUMemory — into the OperationManager framework, following the previously refactored copyToGPU. All related methods have been standardized to take no parameters and are now registered as std::function<void()> callbacks. This centralizes GPU operation dispatch, eliminates hardcoded call chains, and improves modularity and maintainability.

Changes
Refactored copyFromGPU and deallocateGPUMemory to be parameterless

Moved required context into member variables

Registered operations during class construction

Added #ifdef USE_GPU guards where applicable

Replaced explicit calls in GPUModel with OperationManager::execute(...)

Maintained consistency with the existing copyToGPU refactor

Checklist (Mandatory for new features)

  • Added Documentation
  • [] Added Unit Tests

Testing (Mandatory for all changes)

  • GPU Test: test-medium-connected.xml Passed
  • GPU Test: test-large-long.xml Passed

Ben Yang and others added 24 commits April 6, 2025 23:14
…ngNeuronsDeviceProperties, now they return by reference so we can use it to intialize the allVerticesDevice_ and allEdgesDevice_
…le files so that it does not take in a parameter. this is the preparation step for the register to operationmanager
…s not take in any parameter. This is to prepare the GPUModel::copySynapseIndexMapHostToDevice to be compatible to register to the operationManager
…ake in no parameter to be compatible with the operationManager
… such that it does not take in any parameter and thus can be registered to te operationManager. Also registered the copySynapseIndexMapHostToDevice In GPUModel.cpp to the OperationManager
… child class, so that it takes in no parameter. Modified functions copyEdgeDeviceToHost for allEdges class for similar functionality. register the copyFromGPU function to the operationMager. Copying simulation results are no longer done by long chain of responsibility of the neurons and synapses but are now through the centralized dispartch: operation Manager
modified function copyFromDevice for allVertices class as well as the…
… child vertex classes to take no parameters.Refactor deleteEdgeDeviceStruct in AllEdges and all child edge classes to take no parameters.registered the deleteSynapseImap in GPUModel to operationManager
…eviceStruct in AllVertices, now the deallocation is done through the operationManager
…d of having simulator.getModel().getLayout().getVertices().registerHistoryVariables(); we can just use operationManager to invokeit
…ryVariable

registered registerHistoryVariable to the operationManager now instea…
@BenYang2002 BenYang2002 requested a review from stiber April 24, 2025 00:11
@BenYang2002 BenYang2002 self-assigned this Apr 24, 2025
@stiber
Copy link
Contributor

stiber commented Apr 24, 2025

This PR isn't named properly, and it doesn't indicate which issue(s) it closes.

Copy link
Contributor

@stiber stiber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need more information about this PR. Which issues is it connected to? It seems to include multiple ones.

Copy link
Contributor

@stiber stiber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just looked at a couple files and I have many questions. I think that this PR is just too big and complex. This should have been done in smaller chunks, and/or it should have been made earlier so that interim code reviews could have been done. So, I will stop with 2 out of 26 files reviewed and around 10 questions. Once we get through that, we can move on to the next set.

@BenYang2002
Copy link
Contributor Author

This PR isn't named properly, and it doesn't indicate which issue(s) it closes.

I just included the issues it closes, also for the issue131(implementing the remaining operations) I included the subissues for it

Ben Yang and others added 8 commits April 24, 2025 18:32
find .clang-format and print the .clang-format out
confirmed that local and remote's .clang-format is the same
… register the copyCPUtoGPU to operationManager
…Graphitti into BenDevelopment

this merge is just to make sure my code on vsc is consistent on the github
…, get rid of the copySynapeseIndMap and replaced with copyCPUToGPU, to make the structure more clear
@stiber stiber changed the title Ben development [ISSUE-131] Moving GPU memory management to OperationManager May 8, 2025
Copy link
Contributor

@stiber stiber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments.

Comment on lines 67 to +70

#if defined(USE_GPU)
GPUModel &gpuModel = static_cast<GPUModel &>(simulator.getModel());
gpuModel.copyEdgeIndexMapHostToDevice(simulator.getModel().getConnections().getEdgeIndexMap(),
simulator.getTotalVertices());
gpuModel.copyCPUtoGPU();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to ask @d-kamath : why is only the GPUModel data being copied to the GPU? Why aren't we copying everything (which can be accomplished via the OperationManager now)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I contacted Dyvia and will post her answer later she replied

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement remaining operations Add in registerHistoryVariables to OperationManager
2 participants