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

[ASDisplayNode] Implement a std::atomic-based flag system for superb performance #89

Merged
merged 2 commits into from
Apr 30, 2017

Conversation

appleguy
Copy link
Member

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.

This reduces lock contention, and should also fix a very rarely seen deadlock.
@appleguy appleguy self-assigned this Apr 30, 2017
@ghost
Copy link

ghost commented Apr 30, 2017

🚫 CI failed with log

Copy link
Member

@Adlai-Holler Adlai-Holler left a 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,
Copy link
Member

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)
Copy link
Member

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);

Copy link
Member

@Adlai-Holler Adlai-Holler left a 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.
@ghost
Copy link

ghost commented Apr 30, 2017

1 Warning
⚠️ Any source code changes should have an entry in CHANGELOG.md or have #trivial in their title.

Generated by 🚫 Danger

@appleguy appleguy merged commit 03a1aa2 into master Apr 30, 2017
@appleguy
Copy link
Member Author

Thanks for your help on this @Adlai-Holler !

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.

None yet

2 participants