Skip to content

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

Merged
appleguy merged 2 commits into
masterfrom
atomicFlags
Apr 30, 2017
Merged

[ASDisplayNode] Implement a std::atomic-based flag system for superb performance#89
appleguy merged 2 commits into
masterfrom
atomicFlags

Conversation

@appleguy

Copy link
Copy Markdown
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
@appleguy
appleguy requested a review from Adlai-Holler April 30, 2017 00:58
@ghost

ghost commented Apr 30, 2017

Copy link
Copy Markdown

🚫 CI failed with log

@Adlai-Holler Adlai-Holler left a comment

Copy link
Copy Markdown
Member

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.

Comment thread Source/Private/ASDisplayNodeInternal.h Outdated

Copy link
Copy Markdown
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.

Comment thread Source/Private/ASDisplayNodeInternal.h Outdated

Copy link
Copy Markdown
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);

@Adlai-Holler Adlai-Holler left a comment

Copy link
Copy Markdown
Member

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

ghost commented Apr 30, 2017

Copy link
Copy Markdown
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
Copy Markdown
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.

2 participants