Skip to content
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

Cleanup existing std::shared_ptr usage #145

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kmichaelk
Copy link
Contributor

No description provided.

@kmichaelk
Copy link
Contributor Author

Regarding feasibility of shared_ptr usage, consider the following example:

auto taskData = std::make_shared<TaskData>();
{
  vector<int> in(...);
  // ...
  taskData->inputs.emplace_back(in.data());
  // ...
}
taskData->inputs[0]; // ???

@aobolensk
Copy link
Member

Regarding feasibility of shared_ptr usage, consider the following example:

auto taskData = std::make_shared<TaskData>();
{
  vector<int> in(...);
  // ...
  taskData->inputs.emplace_back(in.data());
  // ...
}
taskData->inputs[0]; // ???

This is a poor argument against shared_ptr

@kmichaelk
Copy link
Contributor Author

Give a counterexample. This design creates ideal conditions for dangling pointers as TaskData is not owning the memory it points too.

@kmichaelk
Copy link
Contributor Author

kmichaelk commented Nov 4, 2024

That is, I don't see a situation where TaskData can be needed when the memory it points to has been freed. TaskData always goes with the container memory and doesn't own it, it's enough to copy these four pointers to ppc::core::Task directly without using shared_ptr.

@aobolensk
Copy link
Member

Your argument about type safety is valid. As for the memory, it should not be owned, because it can be used across multiple TaskData instances. Most probably, it will need to accept shared pointer to the memory for each buffer, but this part is just not finished, AFAIU

@kmichaelk
Copy link
Contributor Author

Right, but if it is not owned by TaskData, is there a point in storing the TaskData on heap? I believe it's enough to just copy it to the Task class instance, the key here is the lifetime of the memory it points to, not these four pointers.

@@ -78,8 +78,8 @@ TEST(mpi_example_perf_test, test_task_run) {
auto perfResults = std::make_shared<ppc::core::PerfResults>();

// Create Perf analyzer
auto perfAnalyzer = std::make_shared<ppc::core::Perf>(testMpiTaskParallel);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is almost the same as allocating a shared_ptr to PerfAnalyzer - it is freed anyway after the test is completed and is not passed anywhere, why not place it on the stack?

Copy link

codecov bot commented Nov 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.72%. Comparing base (812626b) to head (29999a2).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #145   +/-   ##
=======================================
  Coverage   96.72%   96.72%           
=======================================
  Files          33       33           
  Lines         581      581           
  Branches      170      170           
=======================================
  Hits          562      562           
  Misses          2        2           
  Partials       17       17           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

Successfully merging this pull request may close these issues.

3 participants