onnxruntime icon indicating copy to clipboard operation
onnxruntime copied to clipboard

Remove nsync

Open snnn opened this issue 1 year ago • 6 comments

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 avatar Apr 22 '24 21:04 snnn

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.

snnn avatar Apr 26 '24 23:04 snnn

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

snnn avatar Oct 18 '24 01:10 snnn

/azp run ONNX Runtime Web CI Pipeline

snnn avatar Oct 18 '24 01:10 snnn

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Oct 18 '24 01:10 azure-pipelines[bot]

/azp run Windows CPU CI Pipeline

snnn avatar Oct 18 '24 01:10 snnn

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Oct 18 '24 01:10 azure-pipelines[bot]

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 avatar Oct 21 '24 22:10 snnn

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'  {} \;

snnn avatar Oct 22 '24 03:10 snnn

FYI there are still some leftovers for nsync https://github.com/microsoft/onnxruntime/blob/ae628b935b819bab164645dcec341f9c5b83a545/cmake/onnxruntime_webassembly.cmake#L241 https://github.com/microsoft/onnxruntime/blob/main/cmake/patches/nsync/nsync_1.26.0.patch

Nekto89 avatar Jun 21 '25 07:06 Nekto89

@fs-eire , can we delete the WASM64 build option and all related cmake code? The feature never went to production and currently it does not have owner . If we still need this feature, we can rebuild it.

snnn avatar Jun 21 '25 17:06 snnn

Yes I am OK to remove it.

fs-eire avatar Jun 24 '25 17:06 fs-eire