-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[C# ] Improve string marshalling and reduce GC pressure #15545
Merged
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
1575f22
Removed PinnedHandle
yuslepukhin 8bdf230
Reduce the number of auxilary OrtMemoryAllocation instances
yuslepukhin dd15bec
Reuse pinned names
yuslepukhin 9a0ae72
Reduce a number of auxillary objects created to reduce GC pressure.
yuslepukhin 3220cf4
Allocate input/output names in native heap to avoid pinning
yuslepukhin 9f80b60
Fix a comment
yuslepukhin a3144e7
Merge branch 'main' into yuslepukhin/gc_reduction
yuslepukhin 238c775
Introduce GetStringTensorElementBuffer
yuslepukhin a5862d1
Fill out string tensor straight to the native buffer
yuslepukhin 7c02b0d
Address formatting failures
yuslepukhin 4ee05bb
Address lint failures
yuslepukhin 3a50226
Merge branch 'main' into yuslepukhin/gc_reduction
yuslepukhin 38b45b5
Address comments
yuslepukhin 415df19
Merge branch 'main' into yuslepukhin/gc_reduction
yuslepukhin 0782488
Rename the new API OrtApi struct member
yuslepukhin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
255 changes: 114 additions & 141 deletions
255
csharp/src/Microsoft.ML.OnnxRuntime/InferenceSession.shared.cs
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of interest when should
byte[]
be used instead of IntPtr and why (given IntPtr has worked up until now)? #PendingThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The managed memory must be pinned before being passed to the native code so it is not GCed or moved. Before this PR we relied on
GCHandle
like pinning (same asMemoryHandle
) which is very expensive as it turns out. The passedIntPtr
did not require marshalling.Chunks of memory allocated on unmanaged heap do not require pinning and can be passed on as
IntPtr
.I learned that when one passes blittable array of blittable types it is pinned automatically using 'fixed()' style much more efficient pinning. And we do not have to do try/finally for that. We sort of used it in other places if one looks carefully (
IntPtr[]
is also considered blittable).In fact, this was one of the most expensive parts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a guideline that could be documented so that going forward the best thing to do can be captured in that instead of trying to infer it from existing implementation details?