-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[ASDisplayNode] Implement a std::atomic-based flag system for superb performance #89
Conversation
appleguy
commented
Apr 30, 2017
This reduces lock contention, and should also fix a very rarely seen deadlock.
🚫 CI failed with log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Left an idea to make it more scalable.
typedef NS_OPTIONS(uint_least32_t, ASDisplayNodeAtomicFlags) | ||
{ | ||
ASDNFlagSynchronous = 1 << 0, | ||
ASDNFlagLayerBacked = 1 << 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove this field until we use it.
#define setFlag(flags, flag, x) (x ? flags.fetch_or(flag) : flags.fetch_and(~flag)) | ||
|
||
#define atomic_isSynchronous checkFlag(_atomicFlags, ASDNFlagSynchronous) | ||
#define atomic_setIsSynchronous(x) setFlag(_atomicFlags, ASDNFlagSynchronous, x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's eliminate the flags
argument from checkFlag
and setFlag
, and then remove the atomic_*
macros.
Let's also have setFlag
return the old value of the flag, so it would shake out like this:
#define checkFlag(flag) ((_atomicFlags.load() & flag) == flag)
// Returns the old value of the flag as a BOOL.
#define setFlag(flag, x) (((x ? _atomicFlags.fetch_or(flag) : _atomicFlags.fetch_and(~flag)) & flag) != 0)
// In code:
BOOL sync = checkFlag(ASDNFlagSynchronous);
setFlag(ASDNFlagSynchronous, YES);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alrighty! I'm down with this and we'll see how nice we can get the naming as we go.
…performance Although Objective-C atomics are equally fast, or better that std::atomic when access through method calls, for the most intense use cases it is best to avoid method calls entirely. In ASDisplayNode, we could benefit significantly from avoiding both method calls (already true today) but also avoid locking the mutex, for both CPU and contention gains. There will still be many methods that need locking for transactional consistency - however, there are currently dozens of accessor methods that could avoid frequent lock / unlock cycles with use of atomics, and other usages of the ivars directly where locking could be delayed until after early-return conditions are checked and passed.
Generated by 🚫 Danger |
Thanks for your help on this @Adlai-Holler ! |