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

Create new stream for data copy for IOBidning input scenario #14719

Merged
merged 2 commits into from
Feb 20, 2023
Merged

Conversation

jslhcl
Copy link
Contributor

@jslhcl jslhcl commented Feb 16, 2023

Description

Create new stream for data copy for IOBidning input scenario

Motivation and Context

Previously in bindInput(), a nullptr Stream is passed to copy data cross device. This caused the default stream is used thus hurt the performance.
This PR is to fix #14484

Stream* device_stream = nullptr;
#ifdef ORT_ENABLE_STREAM
DeviceStreamCollectionHolder device_stream_collection_holder(session_state);
if (device_stream_collection_holder.p_ != nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (device_stream_collection_holder.p_ != nullptr)

device_stream_collection_holder.p_

@@ -507,6 +507,48 @@ static common::Status CopyInputsAcrossDevices(const SessionState& session_state,
return Status::OK();
}

#ifdef ORT_ENABLE_STREAM
struct DeviceStreamCollectionHolder {
Copy link
Contributor

Choose a reason for hiding this comment

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

DeviceStreamCollectionHolder

why we need a holder here?
the collection might get "recycled" accidentally?

Copy link
Member

Choose a reason for hiding this comment

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

Lei just move this func to a new place. the original implementation is to use this holder as a guard to make sure if any error happened we return the device streams to session state correctly.

@@ -507,6 +507,48 @@ static common::Status CopyInputsAcrossDevices(const SessionState& session_state,
return Status::OK();
}

#ifdef ORT_ENABLE_STREAM
struct DeviceStreamCollectionHolder {
Copy link
Member

Choose a reason for hiding this comment

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

Lei just move this func to a new place. the original implementation is to use this holder as a guard to make sure if any error happened we return the device streams to session state correctly.

@jslhcl
Copy link
Contributor Author

jslhcl commented Feb 20, 2023

/azp run MacOS CI Pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jslhcl jslhcl merged commit 3d79b1f into main Feb 20, 2023
@jslhcl jslhcl deleted the leca/14484 branch February 20, 2023 17:47
PatriceVignola pushed a commit that referenced this pull request Feb 22, 2023
### Description
Create new stream for data copy for IOBidning input scenario



### Motivation and Context
Previously in bindInput(), a nullptr Stream is passed to copy data cross
device. This caused the default stream is used thus hurt the
performance.
This PR is to fix #14484

---------

Co-authored-by: Lei Cao <leca@microsoft.com>
PatriceVignola pushed a commit that referenced this pull request Feb 22, 2023
### Description
Create new stream for data copy for IOBidning input scenario



### Motivation and Context
Previously in bindInput(), a nullptr Stream is passed to copy data cross
device. This caused the default stream is used thus hurt the
performance.
This PR is to fix #14484

---------

Co-authored-by: Lei Cao <leca@microsoft.com>
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.

[Performance] 1.14RC1 Tensorrt Regression
3 participants