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

Avoid setting frame on a node's backing store while holding its lock #1048

Merged
merged 1 commit into from
Jul 24, 2018
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 64 additions & 56 deletions Source/Private/ASDisplayNode+UIViewBridge.mm
Original file line number Diff line number Diff line change
Expand Up @@ -296,70 +296,78 @@ - (CGRect)frame

- (void)setFrame:(CGRect)rect
{
_bridge_prologue_write;
BOOL setToView = NO;
BOOL setToLayer = NO;
CGRect newBounds = CGRectZero;
CGPoint newPosition = CGPointZero;
BOOL nodeLoaded = NO;
BOOL isMainThread = ASDisplayNodeThreadIsMain();
{
_bridge_prologue_write;

// For classes like ASTableNode, ASCollectionNode, ASScrollNode and similar - make sure UIView gets setFrame:
struct ASDisplayNodeFlags flags = _flags;
BOOL specialPropertiesHandling = ASDisplayNodeNeedsSpecialPropertiesHandling(checkFlag(Synchronous), flags.layerBacked);
// For classes like ASTableNode, ASCollectionNode, ASScrollNode and similar - make sure UIView gets setFrame:
struct ASDisplayNodeFlags flags = _flags;
BOOL specialPropertiesHandling = ASDisplayNodeNeedsSpecialPropertiesHandling(checkFlag(Synchronous), flags.layerBacked);

nodeLoaded = _loaded(self);
if (!specialPropertiesHandling) {
BOOL canReadProperties = isMainThread || !nodeLoaded;
if (canReadProperties) {
// We don't have to set frame directly, and we can read current properties.
// Compute a new bounds and position and set them on self.
CALayer *layer = _layer;
CGPoint origin = (nodeLoaded ? layer.bounds.origin : self.bounds.origin);
CGPoint anchorPoint = (nodeLoaded ? layer.anchorPoint : self.anchorPoint);

ASBoundsAndPositionForFrame(rect, origin, anchorPoint, &newBounds, &newPosition);

if (ASIsCGRectValidForLayout(newBounds) == NO || ASIsCGPositionValidForLayout(newPosition) == NO) {
ASDisplayNodeAssertNonFatal(NO, @"-[ASDisplayNode setFrame:] - The new frame (%@) is invalid and unsafe to be set.", NSStringFromCGRect(rect));
return;
}

BOOL nodeLoaded = _loaded(self);
BOOL isMainThread = ASDisplayNodeThreadIsMain();
if (!specialPropertiesHandling) {
BOOL canReadProperties = isMainThread || !nodeLoaded;
if (canReadProperties) {
// We don't have to set frame directly, and we can read current properties.
// Compute a new bounds and position and set them on self.
CALayer *layer = _layer;
BOOL useLayer = (layer != nil);
CGPoint origin = (useLayer ? layer.bounds.origin : self.bounds.origin);
CGPoint anchorPoint = (useLayer ? layer.anchorPoint : self.anchorPoint);

CGRect newBounds = CGRectZero;
CGPoint newPosition = CGPointZero;
ASBoundsAndPositionForFrame(rect, origin, anchorPoint, &newBounds, &newPosition);

if (ASIsCGRectValidForLayout(newBounds) == NO || ASIsCGPositionValidForLayout(newPosition) == NO) {
ASDisplayNodeAssertNonFatal(NO, @"-[ASDisplayNode setFrame:] - The new frame (%@) is invalid and unsafe to be set.", NSStringFromCGRect(rect));
return;
}

if (useLayer) {
layer.bounds = newBounds;
layer.position = newPosition;
if (nodeLoaded) {
setToLayer = YES;
} else {
self.bounds = newBounds;
self.position = newPosition;
}
} else {
self.bounds = newBounds;
self.position = newPosition;
}
} else {
// We don't have to set frame directly, but we can't read properties.
// Store the frame in our pending state, and it'll get decomposed into
// bounds and position when the pending state is applied.
_ASPendingState *pendingState = ASDisplayNodeGetPendingState(self);
if (nodeLoaded && !pendingState.hasChanges) {
[[ASPendingStateController sharedInstance] registerNode:self];
// We don't have to set frame directly, but we can't read properties.
// Store the frame in our pending state, and it'll get decomposed into
// bounds and position when the pending state is applied.
_ASPendingState *pendingState = ASDisplayNodeGetPendingState(self);
if (nodeLoaded && !pendingState.hasChanges) {
[[ASPendingStateController sharedInstance] registerNode:self];
}
pendingState.frame = rect;
}
pendingState.frame = rect;
}
} else {
if (nodeLoaded && isMainThread) {
// We do have to set frame directly, and we're on main thread with a loaded node.
// Just set the frame on the view.
// NOTE: Frame is only defined when transform is identity because we explicitly diverge from CALayer behavior and define frame without transform.
//#if DEBUG
// // Checking if the transform is identity is expensive, so disable when unnecessary. We have assertions on in Release, so DEBUG is the only way I know of.
// ASDisplayNodeAssert(CATransform3DIsIdentity(self.transform), @"-[ASDisplayNode setFrame:] - self.transform must be identity in order to set the frame property. (From Apple's UIView documentation: If the transform property is not the identity transform, the value of this property is undefined and therefore should be ignored.)");
//#endif
_view.frame = rect;
} else {
// We do have to set frame directly, but either the node isn't loaded or we're on a non-main thread.
// Set the frame on the pending state, and it'll call setFrame: when applied.
_ASPendingState *pendingState = ASDisplayNodeGetPendingState(self);
if (nodeLoaded && !pendingState.hasChanges) {
[[ASPendingStateController sharedInstance] registerNode:self];
if (nodeLoaded && isMainThread) {
// We do have to set frame directly, and we're on main thread with a loaded node.
// Just set the frame on the view.
// NOTE: Frame is only defined when transform is identity because we explicitly diverge from CALayer behavior and define frame without transform.
setToView = YES;
} else {
// We do have to set frame directly, but either the node isn't loaded or we're on a non-main thread.
// Set the frame on the pending state, and it'll call setFrame: when applied.
_ASPendingState *pendingState = ASDisplayNodeGetPendingState(self);
if (nodeLoaded && !pendingState.hasChanges) {
[[ASPendingStateController sharedInstance] registerNode:self];
}
pendingState.frame = rect;
}
pendingState.frame = rect;
}
}

if (setToView) {
ASDisplayNodeAssertTrue(nodeLoaded && isMainThread);
_view.frame = rect;
Copy link
Contributor

Choose a reason for hiding this comment

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

We may should grab the _view and _layer with the lock held?

Copy link
Member Author

Choose a reason for hiding this comment

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

Those properties are only modified on main, and since this is on main thread, we can avoid locking.

Copy link
Member Author

Choose a reason for hiding this comment

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

See also -isNodeLoaded.

} else if (setToLayer) {
ASDisplayNodeAssertTrue(nodeLoaded && isMainThread);
_layer.bounds = newBounds;
_layer.position = newPosition;
}
}

- (void)setNeedsDisplay
Expand Down