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

Remove nsync #20413

Merged
merged 9 commits into from
Oct 21, 2024
Merged

Remove nsync #20413

merged 9 commits into from
Oct 21, 2024

Conversation

snnn
Copy link
Member

@snnn snnn commented Apr 22, 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.

@snnn snnn marked this pull request as ready for review April 23, 2024 20:28
@snnn snnn requested a review from a team as a code owner April 23, 2024 20:28
@snnn snnn requested a review from pranavsharma April 25, 2024 00:04
@snnn snnn marked this pull request as draft April 26, 2024 23:25
@snnn
Copy link
Member Author

snnn commented Apr 26, 2024

I hit a blocker in Web CI pipeline. Then Yulong and I had a meeting and discussed it. We found the root cause, but the fix is non-trivial. Instead, I will wait his ES6 change getting merged first.

Copy link
Member

@yuslepukhin yuslepukhin left a comment

Choose a reason for hiding this comment

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

General comment.
It would save us a lot of refactoring and preserve lots of flexibility if we simply refactored OrtMutex or typedefed as absl::Mutex. Also could continue to use std::lock_guard. Then most of the code would remain the same, and we could change it to something else as the circumstances require.

onnxruntime/test/platform/threadpool_test.cc Outdated Show resolved Hide resolved
@snnn snnn reopened this Oct 18, 2024
@snnn snnn force-pushed the snnn/remove_nsync branch from d1f00f6 to ce962d9 Compare October 18, 2024 00:59
@snnn
Copy link
Member Author

snnn commented Oct 18, 2024

I updated this PR to use std::mutex instead. Originally I tried to use absl::mutex.

@snnn
Copy link
Member Author

snnn commented Oct 18, 2024

/azp run ONNX Runtime Web CI Pipeline

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@snnn
Copy link
Member Author

snnn commented Oct 18, 2024

/azp run Windows CPU CI Pipeline

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@snnn snnn marked this pull request as ready for review October 18, 2024 01:49
@snnn snnn closed this Oct 18, 2024
@snnn snnn reopened this Oct 18, 2024
@snnn
Copy link
Member Author

snnn commented Oct 21, 2024

General comment. It would save us a lot of refactoring and preserve lots of flexibility if we simply refactored OrtMutex or typedefed as absl::Mutex. Also could continue to use std::lock_guard. Then most of the code would remain the same, and we could change it to something else as the circumstances require.

I reworked the PR. absl::Mutex cannot be used with std::lock_guard. So I changed the type to std::mutex . As we no longer use a custom type of mutex, I think we no longer need to have an extra typedef . Though this PR changed 110 files. All the changes were done by a simple bash script. If in the future we want to introduce a custom mutex type back, I volunteer to help.

@snnn snnn merged commit 88676e6 into main Oct 21, 2024
91 checks passed
@snnn snnn deleted the snnn/remove_nsync branch October 21, 2024 22:32
@snnn
Copy link
Member Author

snnn commented Oct 22, 2024

For reference, the code changes were generated by the following commands:

git rm include/onnxruntime/core/platform/ort_mutex.h -f
git rm onnxruntime/core/platform/posix/ort_mutex.cc -f


find . -type f -exec sed -i 's/onnxruntime::OrtMutex/std::mutex/g'  {} \;
find . -type f -exec sed -i 's/OrtMutex/std::mutex/g'  {} \;

find . -type f -exec sed -i 's/onnxruntime::OrtCondVar/std::condition_variable/g'  {} \;
find . -type f -exec sed -i 's/OrtCondVar/std::condition_variable/g'  {} \;

find . -type f -exec sed -i 's/#include "core\/platform\/ort_mutex.h"/#include <mutex>/g'  {} \;
find . -type f -exec sed -i 's/#include <core\/platform\/ort_mutex.h>/#include <mutex>/g'  {} \;

ishwar-raut1 pushed a commit to ishwar-raut1/onnxruntime that referenced this pull request Nov 19, 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 microsoft#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 microsoft#21005 , which is no longer needed since conda
has updated its msvc runtime DLL.

This PR unblocks microsoft#22173 and resolves microsoft#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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Build] cannot create ALIAS target "nsync::nsync_cpp"
3 participants