-
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
Stricter locking assertions #1024
Merged
nguyenhuy
merged 9 commits into
TextureGroup:master
from
nguyenhuy:HN-Stricter-Locking-Safety-Checks
Jul 13, 2018
Merged
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
c037799
Stricter locking assertions
nguyenhuy c2bdd1a
Rename to ASAssertLocked and ASAssertUnlocked
nguyenhuy 48b7fa1
Revert unnecessary change
nguyenhuy b454388
Rename -_isNodeLoaded to -_unsafe_unlocked_isNodeLoaded
nguyenhuy 49c21ae
Use inline method
nguyenhuy 372ff89
Merge branch 'master' into HN-Stricter-Locking-Safety-Checks
Adlai-Holler 0325062
Move __loaded(node) macro to ASDisplayNodeInternal and use it throughout
nguyenhuy 4dd528f
Merge branch 'HN-Stricter-Locking-Safety-Checks' of github.com:nguyen…
nguyenhuy f4234df
Simpler node loaded check: regardless of whether the node is view or …
nguyenhuy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -184,6 +184,7 @@ - (ASSizeRange)constrainedSizeForCalculatedLayout | |
|
||
- (ASSizeRange)_locked_constrainedSizeForCalculatedLayout | ||
{ | ||
ASAssertLocked(__instanceLock__); | ||
if (_pendingDisplayNodeLayout != nullptr && _pendingDisplayNodeLayout->isValid(_layoutVersion)) { | ||
return _pendingDisplayNodeLayout->constrainedSize; | ||
} | ||
|
@@ -218,9 +219,10 @@ @implementation ASDisplayNode (ASLayoutInternal) | |
*/ | ||
- (void)_u_setNeedsLayoutFromAbove | ||
{ | ||
ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock); | ||
as_activity_create_for_scope("Set needs layout from above"); | ||
ASDisplayNodeAssertThreadAffinity(self); | ||
ASAssertUnlocked(__instanceLock__); | ||
|
||
as_activity_create_for_scope("Set needs layout from above"); | ||
|
||
// Mark the node for layout in the next layout pass | ||
[self setNeedsLayout]; | ||
|
@@ -243,7 +245,7 @@ - (void)_u_setNeedsLayoutFromAbove | |
- (void)_rootNodeDidInvalidateSize | ||
{ | ||
ASDisplayNodeAssertThreadAffinity(self); | ||
ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); | ||
ASAssertUnlocked(__instanceLock__); | ||
|
||
__instanceLock__.lock(); | ||
|
||
|
@@ -273,7 +275,7 @@ - (void)_rootNodeDidInvalidateSize | |
- (void)displayNodeDidInvalidateSizeNewSize:(CGSize)size | ||
{ | ||
ASDisplayNodeAssertThreadAffinity(self); | ||
ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); | ||
ASAssertUnlocked(__instanceLock__); | ||
|
||
// The default implementation of display node changes the size of itself to the new size | ||
CGRect oldBounds = self.bounds; | ||
|
@@ -295,19 +297,19 @@ - (void)displayNodeDidInvalidateSizeNewSize:(CGSize)size | |
|
||
- (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)bounds | ||
{ | ||
ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock); | ||
ASAssertUnlocked(__instanceLock__); | ||
|
||
BOOL isInLayoutPendingState = NO; | ||
{ | ||
ASDN::MutexLocker l(__instanceLock__); | ||
// Check if we are a subnode in a layout transition. | ||
// In this case no measurement is needed as it's part of the layout transition | ||
if ([self _isLayoutTransitionInvalid]) { | ||
if ([self _locked_isLayoutTransitionInvalid]) { | ||
return; | ||
} | ||
|
||
CGSize boundsSizeForLayout = ASCeilSizeValues(bounds.size); | ||
|
||
// Prefer a newer and not yet applied _pendingDisplayNodeLayout over _calculatedDisplayNodeLayout | ||
// If there is no such _pending, check if _calculated is valid to reuse (avoiding recalculation below). | ||
BOOL pendingLayoutIsPreferred = NO; | ||
|
@@ -328,38 +330,42 @@ - (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)bounds | |
if (!pendingLayoutIsPreferred && calculatedLayoutIsReusable) { | ||
return; | ||
} | ||
|
||
as_activity_create_for_scope("Update node layout for current bounds"); | ||
as_log_verbose(ASLayoutLog(), "Node %@, bounds size %@, calculatedSize %@, calculatedIsDirty %d", self, NSStringFromCGSize(boundsSizeForLayout), NSStringFromCGSize(_calculatedDisplayNodeLayout->layout.size), _calculatedDisplayNodeLayout->version < _layoutVersion.load()); | ||
as_log_verbose(ASLayoutLog(), "Node %@, bounds size %@, calculatedSize %@, calculatedIsDirty %d", | ||
self, | ||
NSStringFromCGSize(boundsSizeForLayout), | ||
NSStringFromCGSize(_calculatedDisplayNodeLayout->layout.size), | ||
_calculatedDisplayNodeLayout->version < _layoutVersion); | ||
// _calculatedDisplayNodeLayout is not reusable we need to transition to a new one | ||
[self cancelLayoutTransition]; | ||
|
||
BOOL didCreateNewContext = NO; | ||
ASLayoutElementContext *context = ASLayoutElementGetCurrentContext(); | ||
if (context == nil) { | ||
context = [[ASLayoutElementContext alloc] init]; | ||
ASLayoutElementPushContext(context); | ||
didCreateNewContext = YES; | ||
} | ||
|
||
// Figure out previous and pending layouts for layout transition | ||
std::shared_ptr<ASDisplayNodeLayout> nextLayout = _pendingDisplayNodeLayout; | ||
#define layoutSizeDifferentFromBounds !CGSizeEqualToSize(nextLayout->layout.size, boundsSizeForLayout) | ||
#define layoutSizeDifferentFromBounds !CGSizeEqualToSize(nextLayout->layout.size, boundsSizeForLayout) | ||
|
||
// nextLayout was likely created by a call to layoutThatFits:, check if it is valid and can be applied. | ||
// If our bounds size is different than it, or invalid, recalculate. Use #define to avoid nullptr-> | ||
BOOL pendingLayoutApplicable = NO; | ||
if (nextLayout == nullptr) { | ||
as_log_verbose(ASLayoutLog(), "No pending layout."); | ||
} else if (nextLayout->version < _layoutVersion) { | ||
} else if (nextLayout->isValid(_layoutVersion) == NO) { | ||
as_log_verbose(ASLayoutLog(), "Pending layout is stale."); | ||
} else if (layoutSizeDifferentFromBounds) { | ||
as_log_verbose(ASLayoutLog(), "Pending layout size %@ doesn't match bounds size.", NSStringFromCGSize(nextLayout->layout.size)); | ||
} else { | ||
as_log_verbose(ASLayoutLog(), "Using pending layout %@.", nextLayout->layout); | ||
pendingLayoutApplicable = YES; | ||
} | ||
|
||
if (!pendingLayoutApplicable) { | ||
as_log_verbose(ASLayoutLog(), "Measuring with previous constrained size."); | ||
// Use the last known constrainedSize passed from a parent during layout (if never, use bounds). | ||
|
@@ -373,11 +379,11 @@ - (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)bounds | |
// Release it and any orphaned subnodes it retains | ||
_pendingDisplayNodeLayout = nullptr; | ||
} | ||
|
||
if (didCreateNewContext) { | ||
ASLayoutElementPopContext(); | ||
} | ||
|
||
// If our new layout's desired size for self doesn't match current size, ask our parent to update it. | ||
// This can occur for either pre-calculated or newly-calculated layouts. | ||
if (nextLayout->requestedLayoutFromAbove == NO | ||
|
@@ -390,49 +396,60 @@ - (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)bounds | |
// In this case, we need to detect that we've already asked to be resized to match this | ||
// particular ASLayout object, and shouldn't loop asking again unless we have a different ASLayout. | ||
nextLayout->requestedLayoutFromAbove = YES; | ||
__instanceLock__.unlock(); | ||
[self _u_setNeedsLayoutFromAbove]; | ||
__instanceLock__.lock(); | ||
|
||
{ | ||
ASDN::MutexUnlocker u(__instanceLock__); | ||
[self _u_setNeedsLayoutFromAbove]; | ||
} | ||
|
||
// Update the layout's version here because _u_setNeedsLayoutFromAbove calls __setNeedsLayout which in turn increases _layoutVersion | ||
// Failing to do this will cause the layout to be invalid immediately | ||
nextLayout->version = _layoutVersion; | ||
} | ||
|
||
// Prepare to transition to nextLayout | ||
ASDisplayNodeAssertNotNil(nextLayout->layout, @"nextLayout->layout should not be nil! %@", self); | ||
_pendingLayoutTransition = [[ASLayoutTransition alloc] initWithNode:self | ||
pendingLayout:nextLayout | ||
previousLayout:_calculatedDisplayNodeLayout]; | ||
isInLayoutPendingState = ASHierarchyStateIncludesLayoutPending(_hierarchyState); | ||
} | ||
|
||
// If a parent is currently executing a layout transition, perform our layout application after it. | ||
if (isInLayoutPendingState == NO) { | ||
// If no transition, apply our new layout immediately (common case). | ||
[self _completePendingLayoutTransition]; | ||
} | ||
} | ||
|
||
- (ASSizeRange)_constrainedSizeForLayoutPass | ||
{ | ||
ASDN::MutexLocker l(__instanceLock__); | ||
return [self _locked_constrainedSizeForLayoutPass]; | ||
} | ||
|
||
- (ASSizeRange)_locked_constrainedSizeForLayoutPass | ||
{ | ||
// TODO: The logic in -_u_setNeedsLayoutFromAbove seems correct and doesn't use this method. | ||
// logic seems correct. For what case does -this method need to do the CGSizeEqual checks? | ||
// IF WE CAN REMOVE BOUNDS CHECKS HERE, THEN WE CAN ALSO REMOVE "REQUESTED FROM ABOVE" CHECK | ||
|
||
|
||
ASAssertLocked(__instanceLock__); | ||
|
||
CGSize boundsSizeForLayout = ASCeilSizeValues(self.threadSafeBounds.size); | ||
std::shared_ptr<ASDisplayNodeLayout> pendingLayout = _pendingDisplayNodeLayout; | ||
std::shared_ptr<ASDisplayNodeLayout> calculatedLayout = _calculatedDisplayNodeLayout; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cache these properties to improve readability of the following lines #notAPerfOptimization |
||
|
||
// Checkout if constrained size of pending or calculated display node layout can be used | ||
if (_pendingDisplayNodeLayout != nullptr | ||
&& (_pendingDisplayNodeLayout->requestedLayoutFromAbove | ||
|| CGSizeEqualToSize(_pendingDisplayNodeLayout->layout.size, boundsSizeForLayout))) { | ||
if (pendingLayout != nullptr | ||
&& (pendingLayout->requestedLayoutFromAbove || CGSizeEqualToSize(pendingLayout->layout.size, boundsSizeForLayout))) { | ||
// We assume the size from the last returned layoutThatFits: layout was applied so use the pending display node | ||
// layout constrained size | ||
return _pendingDisplayNodeLayout->constrainedSize; | ||
} else if (_calculatedDisplayNodeLayout->layout != nil | ||
&& (_calculatedDisplayNodeLayout->requestedLayoutFromAbove | ||
|| CGSizeEqualToSize(_calculatedDisplayNodeLayout->layout.size, boundsSizeForLayout))) { | ||
return pendingLayout->constrainedSize; | ||
} else if (calculatedLayout->layout != nil | ||
&& (calculatedLayout->requestedLayoutFromAbove || CGSizeEqualToSize(calculatedLayout->layout.size, boundsSizeForLayout))) { | ||
// We assume the _calculatedDisplayNodeLayout is still valid and the frame is not different | ||
return _calculatedDisplayNodeLayout->constrainedSize; | ||
return calculatedLayout->constrainedSize; | ||
} else { | ||
// In this case neither the _pendingDisplayNodeLayout or the _calculatedDisplayNodeLayout constrained size can | ||
// be reused, so the current bounds is used. This is usual the case if a frame was set manually that differs to | ||
|
@@ -444,7 +461,7 @@ - (ASSizeRange)_locked_constrainedSizeForLayoutPass | |
- (void)_layoutSublayouts | ||
{ | ||
ASDisplayNodeAssertThreadAffinity(self); | ||
ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); | ||
ASAssertUnlocked(__instanceLock__); | ||
|
||
ASLayout *layout; | ||
{ | ||
|
@@ -503,6 +520,7 @@ - (BOOL)_isLayoutTransitionInvalid | |
|
||
- (BOOL)_locked_isLayoutTransitionInvalid | ||
{ | ||
ASAssertLocked(__instanceLock__); | ||
if (ASHierarchyStateIncludesLayoutPending(_hierarchyState)) { | ||
ASLayoutElementContext *context = ASLayoutElementGetCurrentContext(); | ||
if (context == nil || _pendingTransitionID != context.transitionID) { | ||
|
@@ -535,18 +553,10 @@ - (void)transitionLayoutWithAnimation:(BOOL)animated | |
measurementCompletion:(void(^)())completion | ||
{ | ||
ASDisplayNodeAssertMainThread(); | ||
|
||
ASSizeRange sizeRange; | ||
{ | ||
ASDN::MutexLocker l(__instanceLock__); | ||
sizeRange = [self _locked_constrainedSizeForLayoutPass]; | ||
} | ||
|
||
[self transitionLayoutWithSizeRange:sizeRange | ||
[self transitionLayoutWithSizeRange:[self _constrainedSizeForLayoutPass] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Easier to just call the "unlocked" variant. |
||
animated:animated | ||
shouldMeasureAsync:shouldMeasureAsync | ||
measurementCompletion:completion]; | ||
|
||
} | ||
|
||
- (void)transitionLayoutWithSizeRange:(ASSizeRange)constrainedSize | ||
|
@@ -865,8 +875,8 @@ - (void)didCompleteLayoutTransition:(id<ASContextTransitioning>)context | |
*/ | ||
- (void)_completePendingLayoutTransition | ||
{ | ||
ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock); | ||
ASAssertUnlocked(__instanceLock__); | ||
|
||
ASLayoutTransition *pendingLayoutTransition = nil; | ||
{ | ||
ASDN::MutexLocker l(__instanceLock__); | ||
|
@@ -875,7 +885,7 @@ - (void)_completePendingLayoutTransition | |
[self _locked_setCalculatedDisplayNodeLayout:pendingLayoutTransition.pendingLayout]; | ||
} | ||
} | ||
|
||
if (pendingLayoutTransition != nil) { | ||
[self _completeLayoutTransition:pendingLayoutTransition]; | ||
[self _pendingLayoutTransitionDidComplete]; | ||
|
@@ -895,6 +905,8 @@ - (void)_completeLayoutTransition:(ASLayoutTransition *)layoutTransition | |
|
||
// Trampoline to the main thread if necessary | ||
if (ASDisplayNodeThreadIsMain() || layoutTransition.isSynchronous == NO) { | ||
// Committing the layout transition will result in subnode insertions and removals, both of which must be called without the lock held | ||
ASAssertUnlocked(__instanceLock__); | ||
[layoutTransition commitTransition]; | ||
} else { | ||
// Subnode insertions and removals need to happen always on the main thread if at least one subnode is already loaded | ||
|
@@ -948,12 +960,13 @@ - (void)_assertSubnodeState | |
- (void)_pendingLayoutTransitionDidComplete | ||
{ | ||
// This assertion introduces a breaking behavior for nodes that has ASM enabled but also manually manage some subnodes. | ||
// Let's gate it behind YOGA flag and remove it right after a branch cut. | ||
// Let's gate it behind YOGA flag. | ||
#if YOGA | ||
[self _assertSubnodeState]; | ||
#endif | ||
|
||
// Subclass hook | ||
ASAssertUnlocked(__instanceLock__); | ||
[self calculatedLayoutDidChange]; | ||
|
||
// Grab lock after calling out to subclass | ||
|
@@ -998,6 +1011,7 @@ - (void)_setCalculatedDisplayNodeLayout:(std::shared_ptr<ASDisplayNodeLayout>)di | |
|
||
- (void)_locked_setCalculatedDisplayNodeLayout:(std::shared_ptr<ASDisplayNodeLayout>)displayNodeLayout | ||
{ | ||
ASAssertLocked(__instanceLock__); | ||
ASDisplayNodeAssertTrue(displayNodeLayout->layout.layoutElement == self); | ||
ASDisplayNodeAssertTrue(displayNodeLayout->layout.size.width >= 0.0); | ||
ASDisplayNodeAssertTrue(displayNodeLayout->layout.size.height >= 0.0); | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Do you mind adding a comment explaining the shared_ptr's here?