-
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
Use a sentinel NSUInteger for node layout data #trivial #428
Conversation
@@ -34,7 +34,6 @@ NS_ASSUME_NONNULL_BEGIN | |||
@protocol _ASDisplayLayerDelegate; | |||
@class _ASDisplayLayer; | |||
@class _ASPendingState; | |||
@class ASSentinel; |
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.
This was just a leftover – the ASSentinel class was removed in favor of atomic integers.
@Adlai-Holler I feel like we're starting to overuse #trivial...@garrettmoon, what do you think about making the changelog a persistent warning rather than an error? Since #trivial disables it anyway, it might actually get more changelogs written if it couldn't be disabled as a warning. |
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.
Great work - this is a very well scoped improvement, which is tough to do considering the integrated nature of this system.
Source/ASDisplayNode+Layout.mm
Outdated
@@ -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)) { |
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.
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).
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.
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.
Source/ASDisplayNode+Layout.mm
Outdated
@@ -579,6 +579,7 @@ - (void)transitionLayoutWithSizeRange:(ASSizeRange)constrainedSize | |||
|
|||
BOOL automaticallyManagesSubnodesDisabled = (self.automaticallyManagesSubnodes == NO); | |||
self.automaticallyManagesSubnodes = YES; // Temporary flag for 1.9.x | |||
newLayoutVersion = _layoutVersion; |
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.
Can you set this immediately after the lock is taken so it is more clear why it is separate from its definition?
@@ -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; |
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.
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.
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.
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.
Source/Private/ASDisplayNodeLayout.h
Outdated
|
||
/* | ||
* 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. |
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.
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.
That's a fine position, but I think we should change it everywhere in that
case. At the moment, it had been consistent that there were no checks for
null on that variable.
It sounds like we can jump ahead to a better clean up state anyway. I think
your suggestion is a good one, and that two instance variables is fine
(definitely preferred over another object wrapping just to contain a couple
scalars).
It's worth noting that with the plan to always layout from root, this path
will become even hotter on the main thread, so these efforts seem justified.
…On Sun, Jul 9, 2017 at 11:14 AM Adlai Holler ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Source/ASDisplayNode+Layout.mm
<#428 (comment)>:
> @@ -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)) {
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.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#428 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAigA31SCVDcB1ECU4KsB25HfuSaMlx5ks5sMRhrgaJpZM4OR8_t>
.
|
LGTM. Merging! |
…ut to not be applied - Since the implementation of layout version (TextureGroup#428), if a node's pending and calculated layouts have the same current version as well as the same constrained size, the 2 layouts are considered equal and can be used interchangeably. A layout version check between the 2 layouts was added in TextureGroup#695. This PR adds a missing constrained size check. - If the pending layout has the same version but a different constrained size compare to the calculated layout's, we can assume that the pending layout is newer and should be preferred over the calculated one. That is because layout operations always register their new layout as pending, which then (immediately or eventually) get applied to the node as calculated layout.
…nding layout to not be applied (#792) * [ASDisplayNode layout] Fix an issue that causes a node's pending layout to not be applied - Since the implementation of layout version (#428), if a node's pending and calculated layouts have the same current version as well as the same constrained size, the 2 layouts are considered equal and can be used interchangeably. A layout version check between the 2 layouts was added in #695. This PR adds a missing constrained size check. - If the pending layout has the same version but a different constrained size compare to the calculated layout's, we can assume that the pending layout is newer and should be preferred over the calculated one. That is because layout operations always register their new layout as pending, which then (immediately or eventually) get applied to the node as calculated layout.
…nding layout to not be applied (TextureGroup#792) * [ASDisplayNode layout] Fix an issue that causes a node's pending layout to not be applied - Since the implementation of layout version (TextureGroup#428), if a node's pending and calculated layouts have the same current version as well as the same constrained size, the 2 layouts are considered equal and can be used interchangeably. A layout version check between the 2 layouts was added in TextureGroup#695. This PR adds a missing constrained size check. - If the pending layout has the same version but a different constrained size compare to the calculated layout's, we can assume that the pending layout is newer and should be preferred over the calculated one. That is because layout operations always register their new layout as pending, which then (immediately or eventually) get applied to the node as calculated layout.
Instead of pushing an
invalidate()
call onto display node layouts, and having a dirty flag on them, use an atomic counter that represents a node's layout data state.This makes it easier to control checkpoints and avoid accidentally using stale layout data.