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

Bugfix in Semaphore::Acquire for non-windows platforms #1456

Merged
merged 2 commits into from
Jan 13, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Docs/ReleaseNotes.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ For breaking API changes see [this document](https://github.com/jrouwe/JoltPhysi
* Fixed CharacterVirtual::Contact::mIsSensorB not being persisted in SaveState.
* Fixed CharacterVirtual::Contact::mHadContact not being true for collisions with sensors. They will still be marked as mWasDiscarded to prevent any further interaction.
* Fixed numerical inaccuracy in penetration depth calculation when CollideShapeSettings::mMaxSeparationDistance was set to a really high value (e.g. 1000).
* Bugfix in Semaphore::Acquire for non-windows platform. The count was updated before waiting, meaning that the counter would become -(number of waiting threads) and the semaphore would not wake up until at least the same amount of releases was done. In practice this meant that the main thread had to do the last (number of threads) jobs, slowing down the simulation a bit.
Copy link
Contributor

Choose a reason for hiding this comment

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

How bad is this exactly? How many jobs can you expect to be run during an update? If someone were to run with a GetMaxConcurrency of say 64, would you then end up running the large majority of jobs on the main thread?

Copy link
Owner Author

Choose a reason for hiding this comment

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

The best improvement I saw was 3% on the Ragdoll Performance test and this was roughly when using 8 threads. When using 20 threads (I have 20 hyper threaded cores) the difference was not measurable. I only noticed it because I have a Vulkan/Metal renderer now and it deadlocked in the MultithreadedTest in the samples app (which I wasn't able to run before on non-windows platforms).

Copy link
Owner Author

Choose a reason for hiding this comment

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

(This deadlock is only possible in this particular test because it starts its own jobs outside the physics update and blocks the main thread when shutting down)


## v5.2.0

Expand Down
4 changes: 3 additions & 1 deletion Jolt/Core/Semaphore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,10 @@ void Semaphore::Acquire(uint inNumber)
}
#else
std::unique_lock lock(mLock);
mWaitVariable.wait(lock, [this, inNumber]() {
return mCount.load(std::memory_order_relaxed) >= int(inNumber);
});
mCount.fetch_sub(inNumber, std::memory_order_relaxed);
mWaitVariable.wait(lock, [this]() { return mCount >= 0; });
#endif
}

Expand Down
Loading