Provide alternative std::mutex implementation on Windows #3000
Conversation
This file contains hidden or 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
skottmckay
reviewed
Feb 11, 2020
skottmckay
reviewed
Feb 11, 2020
skottmckay
reviewed
Feb 11, 2020
skottmckay
reviewed
Feb 11, 2020
| option(onnxruntime_ENABLE_MEMLEAK_CHECKER "Experimental: Enable memory leak checker in Windows debug build" OFF) | ||
| option(onnxruntime_USE_CUDA "Build with CUDA support" OFF) | ||
| option(onnxruntime_USE_OPENVINO "Build with OpenVINO support" OFF) | ||
| option(onnxruntime_USE_NSYNC "Build with NSYNC support. This option only takes effect on Linux" OFF) |
Contributor
There was a problem hiding this comment.
Is nsync going to be ON by default? If yes, do we still need #ifdef USE_NSYNC?
Contributor
Author
There was a problem hiding this comment.
NO. We can remove the ifdef
snnn
force-pushed
the
snnn/nsync
branch
2 times, most recently
from
c6a1804 to
d94262a
Compare
pranavsharma
approved these changes
Feb 11, 2020
Contributor
Can you paste some perf numbers that show the advantage of this mutex? This will set an example for future PR authors who want to submit perf fixes. Thanks. |
Contributor
Author
Sent you an email. |
Merged
This was referenced Oct 18, 2024
Merged
snnn
pushed a commit
that referenced
this pull request
Oct 21, 2024
### Description 1. Remove the onnxruntime::OrtMutex class and replace it with ~absl::Mutex~ std::mutex. 2. After this change, most source files will not include <Windows.h> indirectly. ### Motivation and Context To reduce the number of deps we have, and address some Github issues that are related to build ONNX Runtime from source. In PR #3000 , I added a custom implementation of std::mutex . It was mainly because at that time std::mutex's default constructor was not trivial on Windows. If you had such a mutex as a global var, it could not be initialized at compile time. Then VC++ team fixed this issue. Therefore we don't need this custom implementation anymore. This PR also removes nsync. I ran several models tests on Linux. I didn't see any perf difference. This PR also reverts PR #21005 , which is no longer needed since conda has updated its msvc runtime DLL. This PR unblocks #22173 and resolves #22092 . We have a lot of open issues with nsync. This PR can resolve all of them.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description:
Provide alternative std::mutex implementation on Windows. OrtMutex is no longer an alias of std::mutex.
Motivation and Context
This PR is only for changing the implementation of OrtMutex. In a following PR I'll show how it will simplify things.