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.

Sign up for GitHub

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

Use a sentinel NSUInteger for node layout data #trivial #428

Merged
merged 3 commits into from
Jul 10, 2017
Merged

Use a sentinel NSUInteger for node layout data #trivial #428

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
df1650f
Use a sentinel NSUInteger for node layout data
Adlai-Holler Jul 9, 2017
05a0e52
Add a comment
Adlai-Holler Jul 9, 2017
8f2be9a
Address feedback from @appleguy
Adlai-Holler Jul 9, 2017
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
26 changes: 14 additions & 12 deletions Source/ASDisplayNode+Layout.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,21 +82,19 @@ - (ASLayout *)layoutThatFits:(ASSizeRange)constrainedSize parentSize:(CGSize)par
}

ASLayout *layout = nil;
if (_calculatedDisplayNodeLayout->isValidForConstrainedSizeParentSize(constrainedSize, parentSize)) {
NSUInteger version = _layoutVersion;
if (_calculatedDisplayNodeLayout != nullptr && _calculatedDisplayNodeLayout->isValid(constrainedSize, parentSize, version)) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm fairly sure that _calculated is never nullptr, and thus we leave it out of all conditionals. If so let's leave it out here because it adds length.

Hopefully we refactor this soon. I think it would be much better to use a raw ASLayout with some special instance variables that only get set for the root (this would be very efficient but also very clean, e.g. specified in ASLayout+Root.h as a private header).

Copy link
Member Author

Choose a reason for hiding this comment

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

I made this change so that the redundancy between the first condition and the second condition is more explicit, and the fact that _calculated is never null isn't widely known (at least by this guy) and probably not ideal! Why should we have two ways of representing a null display node layout – literally nullptr vs this blank entry we create ASDisplayNodeLayout()? I'm sensitive to the length argument but I'm going to keep this change because I think the consistency of treatment outweighs the length.

That said I completely agree about the latter! Your suggestion is good or bundling those extra ivars up into a C-struct and storing two props in node _pendingLayout: ASLayout, _pendingLayoutExtras: ASLayoutMeta and mutating them together or something. Currently the use of shared_ptrs here is hard on the eyes.

ASDisplayNodeAssertNotNil(_calculatedDisplayNodeLayout->layout, @"-[ASDisplayNode layoutThatFits:parentSize:] _calculatedDisplayNodeLayout->layout should not be nil! %@", self);
// Our calculated layout is suitable for this constrainedSize, so keep using it and
// invalidate any pending layout that has been generated in the past.
_pendingDisplayNodeLayout = nullptr;
layout = _calculatedDisplayNodeLayout->layout;
} else if (_pendingDisplayNodeLayout != nullptr && _pendingDisplayNodeLayout->isValidForConstrainedSizeParentSize(constrainedSize, parentSize)) {
} else if (_pendingDisplayNodeLayout != nullptr && _pendingDisplayNodeLayout->isValid(constrainedSize, parentSize, version)) {
ASDisplayNodeAssertNotNil(_pendingDisplayNodeLayout->layout, @"-[ASDisplayNode layoutThatFits:parentSize:] _pendingDisplayNodeLayout->layout should not be nil! %@", self);
layout = _pendingDisplayNodeLayout->layout;
} else {
// Create a pending display node layout for the layout pass
layout = [self calculateLayoutThatFits:constrainedSize
restrictedToSize:self.style.size
relativeToParentSize:parentSize];
_pendingDisplayNodeLayout = std::make_shared<ASDisplayNodeLayout>(layout, constrainedSize, parentSize);
_pendingDisplayNodeLayout = std::make_shared<ASDisplayNodeLayout>(layout, constrainedSize, parentSize, version);
ASDisplayNodeAssertNotNil(layout, @"-[ASDisplayNode layoutThatFits:parentSize:] newly calculated layout should not be nil! %@", self);
}

Expand Down Expand Up @@ -308,7 +306,7 @@ - (void)_locked_measureNodeWithBoundsIfNecessary:(CGRect)bounds
// Prefer _pendingDisplayNodeLayout over _calculatedDisplayNodeLayout (if exists, it's the newest)
// If there is no _pending, check if _calculated is valid to reuse (avoiding recalculation below).
if (_pendingDisplayNodeLayout == nullptr) {
if (_calculatedDisplayNodeLayout->isDirty() == NO
if (_calculatedDisplayNodeLayout->version >= _layoutVersion
&& (_calculatedDisplayNodeLayout->requestedLayoutFromAbove == YES
|| CGSizeEqualToSize(_calculatedDisplayNodeLayout->layout.size, boundsSizeForLayout))) {
return;
Expand Down Expand Up @@ -337,8 +335,8 @@ - (void)_locked_measureNodeWithBoundsIfNecessary:(CGRect)bounds
BOOL pendingLayoutApplicable = NO;
if (nextLayout == nullptr) {
as_log_verbose(ASLayoutLog(), "No pending layout.");
} else if (nextLayout->isDirty()) {
as_log_verbose(ASLayoutLog(), "Pending layout is invalid.");
} else if (nextLayout->version < _layoutVersion) {
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 {
Expand All @@ -349,12 +347,13 @@ - (void)_locked_measureNodeWithBoundsIfNecessary:(CGRect)bounds
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).
NSUInteger version = _layoutVersion;
ASSizeRange constrainedSize = [self _locked_constrainedSizeForLayoutPass];
ASLayout *layout = [self calculateLayoutThatFits:constrainedSize
restrictedToSize:self.style.size
relativeToParentSize:boundsSizeForLayout];

nextLayout = std::make_shared<ASDisplayNodeLayout>(layout, constrainedSize, boundsSizeForLayout);
nextLayout = std::make_shared<ASDisplayNodeLayout>(layout, constrainedSize, boundsSizeForLayout, version);
}

if (didCreateNewContext) {
Expand Down Expand Up @@ -425,7 +424,7 @@ - (void)_layoutSublayouts
ASLayout *layout;
{
ASDN::MutexLocker l(__instanceLock__);
if (_calculatedDisplayNodeLayout->isDirty()) {
if (_calculatedDisplayNodeLayout->version < _layoutVersion) {
return;
}
layout = _calculatedDisplayNodeLayout->layout;
Expand Down Expand Up @@ -570,6 +569,7 @@ - (void)transitionLayoutWithSizeRange:(ASSizeRange)constrainedSize

// Perform a full layout creation pass with passed in constrained size to create the new layout for the transition
ASLayout *newLayout;
NSUInteger newLayoutVersion;
{
ASDN::MutexLocker l(__instanceLock__);

Expand All @@ -579,6 +579,7 @@ - (void)transitionLayoutWithSizeRange:(ASSizeRange)constrainedSize

BOOL automaticallyManagesSubnodesDisabled = (self.automaticallyManagesSubnodes == NO);
self.automaticallyManagesSubnodes = YES; // Temporary flag for 1.9.x
newLayoutVersion = _layoutVersion;
Copy link
Member

Choose a reason for hiding this comment

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

Can you set this immediately after the lock is taken so it is more clear why it is separate from its definition?

newLayout = [self calculateLayoutThatFits:constrainedSize
restrictedToSize:self.style.size
relativeToParentSize:constrainedSize.max];
Expand Down Expand Up @@ -609,7 +610,8 @@ - (void)transitionLayoutWithSizeRange:(ASSizeRange)constrainedSize
auto previousLayout = _calculatedDisplayNodeLayout;
auto pendingLayout = std::make_shared<ASDisplayNodeLayout>(newLayout,
constrainedSize,
constrainedSize.max);
constrainedSize.max,
newLayoutVersion);
[self _locked_setCalculatedDisplayNodeLayout:pendingLayout];

// Setup pending layout transition for animation
Expand Down
9 changes: 2 additions & 7 deletions Source/ASDisplayNode.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,7 @@ - (void)_initializeInstance

_calculatedDisplayNodeLayout = std::make_shared<ASDisplayNodeLayout>();
_pendingDisplayNodeLayout = nullptr;
_layoutVersion = 1;

_defaultLayoutTransitionDuration = 0.2;
_defaultLayoutTransitionDelay = 0.0;
Expand Down Expand Up @@ -882,12 +883,7 @@ - (void)invalidateCalculatedLayout
{
ASDN::MutexLocker l(__instanceLock__);

// This will cause the next layout pass to compute a new layout instead of returning
// the cached layout in case the constrained or parent size did not change
_calculatedDisplayNodeLayout->invalidate();
if (_pendingDisplayNodeLayout != nullptr) {
_pendingDisplayNodeLayout->invalidate();
}
_layoutVersion++;

_unflattenedLayout = nil;

Expand Down Expand Up @@ -924,7 +920,6 @@ - (void)__layout
// This method will confirm that the layout is up to date (and update if needed).
// Importantly, it will also APPLY the layout to all of our subnodes if (unless parent is transitioning).
[self _locked_measureNodeWithBoundsIfNecessary:bounds];
_pendingDisplayNodeLayout = nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this removed? Although it may be valid, I think we should revert this change if it's not necessary, as it adds at least a touch of risk that would be better to do in a diff focused on the _pending layout storage.

For example, I'm not sure if there is ever a case where the _pending is not copied to _calculated, such as if the _calculated matches the current bounds but _pending does not. In this case it would change the behavior of other calls that would use _pending later, compared to before this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

My aim here is to converge our handling of invalidation. If the _pendingDisplayNodeLayout is still valid, we should keep it around and use it freely. It may be a behavior change but I think it'll be for the better. If _pending and _calculated both have the same version, and it's the current version, then we should be comfortable arbitrarily using either one (minus size constraints) since neither has fresher layout data.

I believe that if I'm wrong, and I may well be, then the solution is to fix our understanding of layout freshness and perhaps find the right place to bump _layoutVersion to avoid using this pending layout again, rather than relying on this one-off (this is the only place where we explicitly clear pending layout). Thoughts? I'd also like to hear @maicki 's take on this if possible.


[self _locked_layoutPlaceholderIfNecessary];
}
Expand Down
5 changes: 4 additions & 1 deletion Source/Private/ASDisplayNodeInternal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ NS_ASSUME_NONNULL_BEGIN
@protocol _ASDisplayLayerDelegate;
@class _ASDisplayLayer;
@class _ASPendingState;
@class ASSentinel;
Copy link
Member Author

Choose a reason for hiding this comment

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

This was just a leftover – the ASSentinel class was removed in favor of atomic integers.

struct ASDisplayNodeFlags;

BOOL ASDisplayNodeSubclassOverridesSelector(Class subclass, SEL selector);
Expand Down Expand Up @@ -160,6 +159,10 @@ FOUNDATION_EXPORT NSString * const ASRenderingEngineDidDisplayNodesScheduledBefo
std::shared_ptr<ASDisplayNodeLayout> _calculatedDisplayNodeLayout;
std::shared_ptr<ASDisplayNodeLayout> _pendingDisplayNodeLayout;

/// Sentinel for layout data. Incremented when we get -setNeedsLayout / -invalidateCalculatedLayout.
/// Starts at 1.
std::atomic<NSUInteger> _layoutVersion;

ASDisplayNodeViewBlock _viewBlock;
ASDisplayNodeLayerBlock _layerBlock;
NSMutableArray<ASDisplayNodeDidLoadBlock> *_onDidLoadBlocks;
Expand Down
23 changes: 7 additions & 16 deletions Source/Private/ASDisplayNodeLayout.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,35 +30,26 @@ struct ASDisplayNodeLayout {
ASSizeRange constrainedSize;
CGSize parentSize;
BOOL requestedLayoutFromAbove;
BOOL _dirty;
NSUInteger version;

/*
* Create a new display node layout with
* @param layout The layout to associate, usually returned from a call to -layoutThatFits:parentSize:
* @param constrainedSize Constrained size used to create the layout
* @param parentSize Parent size used to create the layout
* @param version The version of the layout data.
Copy link
Member

Choose a reason for hiding this comment

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

In at least one of the comments we should describe what this means.. E.g. the layout is considered invalid when this version doesn't match the node's own state, which is guaranteed to occur after each call to invalidateCalculatedLayout (which is called by setNeedslayout).

I saw you touched on this with the instance variable definition, but this is just my observation of where exposition would be useful.

*/
ASDisplayNodeLayout(ASLayout *layout, ASSizeRange constrainedSize, CGSize parentSize)
: layout(layout), constrainedSize(constrainedSize), parentSize(parentSize), requestedLayoutFromAbove(NO), _dirty(NO) {};
ASDisplayNodeLayout(ASLayout *layout, ASSizeRange constrainedSize, CGSize parentSize, NSUInteger version)
: layout(layout), constrainedSize(constrainedSize), parentSize(parentSize), requestedLayoutFromAbove(NO), version(version) {};

/*
* Creates a layout without any layout associated. By default this display node layout is dirty.
*/
ASDisplayNodeLayout()
: layout(nil), constrainedSize({{0, 0}, {0, 0}}), parentSize({0, 0}), requestedLayoutFromAbove(NO), _dirty(YES) {};
: layout(nil), constrainedSize({{0, 0}, {0, 0}}), parentSize({0, 0}), requestedLayoutFromAbove(NO), version(0) {};

/**
* Returns if the display node layout is dirty as it was invalidated or it was created without a layout.
* Returns whether this is valid for a given constrained size, parent size, and version
*/
BOOL isDirty();

/**
* Returns if ASDisplayNode is still valid for a given constrained and parent size
*/
BOOL isValidForConstrainedSizeParentSize(ASSizeRange constrainedSize, CGSize parentSize);

/**
* Invalidate the display node layout
*/
void invalidate();
BOOL isValid(ASSizeRange constrainedSize, CGSize parentSize, NSUInteger version);
};
23 changes: 5 additions & 18 deletions Source/Private/ASDisplayNodeLayout.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,23 +17,10 @@

#import <AsyncDisplayKit/ASDisplayNodeLayout.h>

BOOL ASDisplayNodeLayout::isDirty()
BOOL ASDisplayNodeLayout::isValid(ASSizeRange theConstrainedSize, CGSize theParentSize, NSUInteger versionArg)
{
return _dirty || layout == nil;
}

BOOL ASDisplayNodeLayout::isValidForConstrainedSizeParentSize(ASSizeRange theConstrainedSize, CGSize theParentSize)
{
// Only generate a new layout if:
// - The current layout is dirty
// - The passed constrained size is different than the original layout's parent or constrained size
return (layout != nil
&& _dirty == NO
&& CGSizeEqualToSize(parentSize, theParentSize)
&& ASSizeRangeEqualToSizeRange(constrainedSize, theConstrainedSize));
}

void ASDisplayNodeLayout::invalidate()
{
_dirty = YES;
return version >= versionArg
&& layout != nil
&& CGSizeEqualToSize(parentSize, theParentSize)
&& ASSizeRangeEqualToSizeRange(constrainedSize, theConstrainedSize);
}