Skip to content

[NFCI][SYCL][Graph] Refactor graph_impl::add #19351

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 2 commits into
base: sycl
Choose a base branch
from

Conversation

aelovikov-intel
Copy link
Contributor

Part of the refactoring to eliminate std::weak_ptr<node_impl> and reduce usage of std::shared_ptr<node_impl> by preferring raw ptr/ref. Previous PRs in the series:
#19295
#19332
#19334
#19350

  • Accept Deps as nodes_range in graph_impl::add
  • Return node_impl & from graph_impl::add
  • Add node support in nodes_range and use that together with modified graph_impl::add when created new node_impls based on std::vector<node> Deps to avoid creation of temporary DepImpls storage.
  • Also updated registerSuccessor/registerPredecessor and addEventForNode/addDepsToNode to accept raw node_impl & as the changes above resulted in having raw reference at the call sites.

... and update the code surrounding their uses in the same spirit.

Continuation of
intel#19295
intel#19332
intel#19334
Part of the refactoring to eliminate `std::weak_ptr<node_impl>` and
reduce usage of `std::shared_ptr<node_impl>` by preferring raw ptr/ref.
Previous PRs in the series:
intel#19295
intel#19332
intel#19334
intel#19350

* Accept `Deps` as `nodes_range` in `graph_impl::add`
* Return `node_impl &` from `graph_impl::add`
* Add `node` support in `nodes_range` and use that together with
  modified `graph_impl::add` when created new `node_impl`s based on
  `std::vector<node> Deps` to avoid creation of temporary `DepImpls`
  storage.
* Also updated `registerSuccessor/registerPredecessor` and
  `addEventForNode/addDepsToNode` to accept raw `node_impl &` as the
  changes above resulted in having raw reference at the call sites.
Copy link
Contributor Author

@aelovikov-intel aelovikov-intel left a comment

Choose a reason for hiding this comment

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

Please ignore first commit - it's on review separately at #19350.

@@ -442,7 +432,9 @@ graph_impl::add(std::function<void(handler &)> CGF,

// Pass the node deps to the handler so they are available when processing the
// CGF, need for async_malloc nodes.
Handler.impl->MNodeDeps = Deps;
Handler.impl->MNodeDeps.clear();
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 to preserve the semantics. Not sure if truly necessary, maybe there is a guarantee that it must be empty. And if not, why are we dropping old deps?

Comment on lines +514 to +516
MNodeStorage.push_back(
std::make_shared<node_impl>(std::forward<Ts>(Args)...));
return *MNodeStorage.back();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if node_impl creation is under a mutex or not. If races are possible, then might need to change to

   auto Ptr = make_shared();
   node_impl &Res = *Ptr;
   MNodeStorage.push_back(std::move(Ptr));
   return Res;

If that's the case, then nodes_range over std::vector<node> optimization needs to be examined for the race conditions as well.

@aelovikov-intel aelovikov-intel marked this pull request as ready for review July 8, 2025 19:37
@aelovikov-intel aelovikov-intel requested review from a team as code owners July 8, 2025 19:37
@aelovikov-intel aelovikov-intel requested review from reble and againull July 8, 2025 19:37
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.

1 participant