-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix cleanup of Environment class. #25743
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
Fix cleanup of Environment class. #25743
Conversation
…invalidates all entries in shared_ort_allocators_. Remove unused shared_arena_allocators_. That became unnecessary by providing EPs an example implementation for an OrtAllocator based stream-aware arena that they can use directly.
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.
Pull Request Overview
This PR fixes a cleanup issue in the Environment class destructor that was causing access violations during shutdown. The changes remove the unnecessary arena_ort_allocators_ container and improve the destruction order to prevent accessing invalidated allocator entries.
Key changes:
- Removes the unused
arena_ort_allocators_member and related cleanup code - Adds explicit clearing of
shared_ort_allocators_in the destructor before EP library unregistration - Removes arena allocator cleanup from
UnregisterAllocatorImplandCreateSharedAllocatorImpl
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| environment.cc | Removes arena allocator cleanup code and adds proper clearing order in destructor |
| environment.h | Removes the unused arena_ort_allocators_ member variable and its documentation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
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.
You can commit the suggested changes from lintrunner.
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.
![]()
### Description <!-- Describe your changes. --> The clearing of shared_allocators_ invalidates all entries in shared_ort_allocators_. Remove unused shared_arena_allocators_. That became unnecessary by providing EPs an example implementation for an OrtAllocator based stream-aware arena that they can use directly. ### 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. --> Fix access violation (swallowed as it happens during shutdown) in dtor.
### Description Cherry-pick the following PRs into the `rel-1.23.0` branch: - #25592 - #25622 - #25688 - #25729 - #25743 - #25769 - #25745 - #25761 - #25751 - #25716 - #25228 - #25768 - #25788 - #25747 - #25800 - #25818 - #25762 - #25749 - #25831 ### 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. --> --------- Co-authored-by: quic-tirupath <[email protected]> Co-authored-by: quic-calvnguy <[email protected]> Co-authored-by: qti-kromero <[email protected]> Co-authored-by: Jeff Kilpatrick <[email protected]> Co-authored-by: Scott McKay <[email protected]> Co-authored-by: David Fan <[email protected]> Co-authored-by: kuanyul-qti <[email protected]> Co-authored-by: Dmitri Smirnov <[email protected]> Co-authored-by: Chi Lo <[email protected]> Co-authored-by: Edward Chen <[email protected]> Co-authored-by: Chunye Wang@AMD <[email protected]> Co-authored-by: minfhong-qti <[email protected]> Co-authored-by: Vishal Agarwal <[email protected]> Co-authored-by: Maximilian Müller <[email protected]> Co-authored-by: Maximilian Müller <[email protected]> Co-authored-by: Changming Sun <[email protected]> Co-authored-by: adrastogi <[email protected]> Co-authored-by: Aditya Rastogi <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
### Description <!-- Describe your changes. --> The clearing of shared_allocators_ invalidates all entries in shared_ort_allocators_. Remove unused shared_arena_allocators_. That became unnecessary by providing EPs an example implementation for an OrtAllocator based stream-aware arena that they can use directly. ### 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. --> Fix access violation (swallowed as it happens during shutdown) in dtor.
Description
The clearing of shared_allocators_ invalidates all entries in shared_ort_allocators_.
Remove unused shared_arena_allocators_. That became unnecessary by providing EPs an example implementation for an OrtAllocator based stream-aware arena that they can use directly.
Motivation and Context
Fix access violation (swallowed as it happens during shutdown) in dtor.