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

Minor rework of SpinLock class for making it more efficient on Unlock() #33

Merged
merged 4 commits into from
May 23, 2019

Conversation

noSTALKER
Copy link
Contributor

  • atomic_flag is initialized using the recommended ATOMIC_FLAG_INIT
  • Unlock() function clears the atomic_flag using more efficient std::memory_order_acquire
  • Unlock() and Lock() methods are now both noexcept
  • Removed unnecessary include files

@msftclas
Copy link

msftclas commented May 17, 2019

CLA assistant check
All CLA requirements met.

@noSTALKER
Copy link
Contributor Author

Just a query for future. I haven't seen noexcept keyword usage much in the code base. Is it intentional or i can include it in future pull request?

@noSTALKER
Copy link
Contributor Author

noSTALKER commented May 17, 2019

Also the the functions are name Lock() and UnLock() so it can't be used in a code section safely which can throw exception with std::scoped_lock() or std::lock_guard() as it requires function names to be lock() and unlock()

@MaggieQi MaggieQi requested a review from AlphardWang May 20, 2019 02:30
@AlphardWang
Copy link
Contributor

Just a query for future. I haven't seen noexcept keyword usage much in the code base. Is it intentional or i can include it in future pull request?

Currently we don't have intention to use or not to use noexcept. Please feel free to use it in your PR. We will do refactoring and add it into readme if it becomes an explicit requirement in the future.


SpinLock(const SpinLock&) = delete;
SpinLock& operator = (const SpinLock&) = delete;

private:
std::atomic_flag m_lock;
std::atomic_flag m_lock = ATOMIC_FLAG_INIT;
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I don't recommend to use default member initializer since it may become confused and hard to read when there are multiple data members or constructors. But it should be fine here since it is the only data member and all implementation is in same file now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the recommended way of in initializing std::atomic_flag, that is why i didn't used it in initializer list as it requires copy initialization using the ATOMIC_FLAG_INIT

Copy link
Contributor

@AlphardWang AlphardWang left a comment

Choose a reason for hiding this comment

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

Please replace the tabs with spaces.

@MaggieQi MaggieQi merged commit 4d15203 into microsoft:master May 23, 2019
t-phada pushed a commit to t-phada/SPTAG that referenced this pull request Aug 19, 2020
…() (microsoft#33)

* Minor rework of SpinLock class for making it more efficient on Unlock()

* remove tabs in Concurrent.h class
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.

4 participants