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

Introduce reused_buffer_index_per_stream in allocation planner which will be reset after computing the reuse buffer for each stream #19515

Merged
merged 10 commits into from
Feb 22, 2024

Conversation

jslhcl
Copy link
Contributor

@jslhcl jslhcl commented Feb 14, 2024

Description

Introduce reused_buffer_index_per_stream in allocation planner which will be reset after computing the reuse buffer for each stream. So if a NodeArg is an input of several Ops across different streams and reuses other NodeArg, the reused NodeArg won't be involved when computing the second stream's reuse plan.

Motivation and Context

This is to fix #19480, which is a crash for the scenario mentioned above.

@jslhcl jslhcl requested a review from souptc February 20, 2024 18:13
@jslhcl jslhcl changed the title [DONT Review] Compute reuse count only once and do not clear during every stream Introduce reused_buffer_index_per_stream in allocation planner which will be reset after computing the reuse buffer for each stream Feb 20, 2024
@jslhcl jslhcl marked this pull request as ready for review February 20, 2024 18:39
@PatriceVignola
Copy link
Contributor

Thanks for this ! Do you know if it works when reverting this change? https://github.com/microsoft/onnxruntime/pull/19481/files

If it does, we don't need to disable the streams on the DML EP anymore and can revert it.

@jslhcl
Copy link
Contributor Author

jslhcl commented Feb 20, 2024

Thanks for this ! Do you know if it works when reverting this change? https://github.com/microsoft/onnxruntime/pull/19481/files

If it does, we don't need to disable the streams on the DML EP anymore and can revert it.

Yes it works. I've tested it in v1.17 without your PR. I will revert your PR once this PR is in

souptc
souptc previously approved these changes Feb 21, 2024
onnxruntime/core/framework/allocation_planner.cc Outdated Show resolved Hide resolved
onnxruntime/core/framework/allocation_planner.cc Outdated Show resolved Hide resolved
PatriceVignola
PatriceVignola previously approved these changes Feb 21, 2024
@jslhcl
Copy link
Contributor Author

jslhcl commented Feb 22, 2024

/azp run Python format

Copy link

No pipelines are associated with this pull request.

@jslhcl jslhcl merged commit 6b73ab3 into main Feb 22, 2024
94 checks passed
@jslhcl jslhcl deleted the leca/experiment branch February 22, 2024 18:19
tianleiwu pushed a commit that referenced this pull request Apr 3, 2024
…will be reset after computing the reuse buffer for each stream (#19515)

### Description
<!-- Describe your changes. -->
Introduce reused_buffer_index_per_stream in allocation planner which
will be reset after computing the reuse buffer for each stream. So if a
NodeArg is an input of several Ops across different streams and reuses
other NodeArg, the reused NodeArg won't be involved when computing the
second stream's reuse plan.


### Motivation and Context
<!-- - Why is this change required? What problem does it solve?
- If it fixes an open issue, please link to the issue here. -->
This is to fix #19480,
which is a crash for the scenario mentioned above.

---------

Co-authored-by: Lei Cao <[email protected]>
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.

Allocation plan reference counting doesn't always work for multiple streams
3 participants