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

[ASDisplayNode] Fix infinite layout loop #455

Merged
merged 2 commits into from
Jul 18, 2017
Merged

[ASDisplayNode] Fix infinite layout loop #455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
74f0c78
Fix infinite layout loop
nguyenhuy Jul 18, 2017
12b4522
Update CHANGELOG
nguyenhuy Jul 18, 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
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
- [ASCollectionView] Add delegate bridging and index space translation for missing UICollectionViewLayout properties. [Scott Goodson](https://github.com/appleguy)
- [ASTextNode2] Add initial implementation for link handling. [Scott Goodson](https://github.com/appleguy) [#396](https://github.com/TextureGroup/Texture/pull/396)
- [ASTextNode2] Provide compile flag to globally enable new implementation of ASTextNode: ASTEXTNODE_EXPERIMENT_GLOBAL_ENABLE. [Scott Goodson](https://github.com/appleguy) [#396](https://github.com/TextureGroup/Texture/pull/410)
- Add ASCollectionGalleryLayoutDelegate - an async collection layout that makes same-size collections (e.g photo galleries, pagers, etc) fast and lightweight! [Huy Nguyen](https://github.com/nguyenhuy/) [#76](https://github.com/TextureGroup/Texture/pull/76) [#451](https://github.com/TextureGroup/Texture/pull/451)
- Add ASCollectionGalleryLayoutDelegate - an async collection layout that makes same-size collections (e.g photo galleries, pagers, etc) fast and lightweight! [Huy Nguyen](https://github.com/nguyenhuy/) [#76](https://github.com/TextureGroup/Texture/pull/76) [#451](https://github.com/TextureGroup/Texture/pull/451)
- Fix an issue that causes infinite layout loop in ASDisplayNode after [#428](https://github.com/TextureGroup/Texture/pull/428) [Huy Nguyen](https://github.com/nguyenhuy) [#455](https://github.com/TextureGroup/Texture/pull/455)

##2.3.5
- Fix an issue where inserting/deleting sections could lead to inconsistent supplementary element behavior. [Adlai Holler](https://github.com/Adlai-Holler)
Expand Down
13 changes: 9 additions & 4 deletions Source/ASDisplayNode+Layout.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,10 +302,10 @@ - (void)_locked_measureNodeWithBoundsIfNecessary:(CGRect)bounds
}

CGSize boundsSizeForLayout = ASCeilSizeValues(bounds.size);

// 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 (_pendingDisplayNodeLayout == nullptr || _pendingDisplayNodeLayout->version < _layoutVersion) {
if (_calculatedDisplayNodeLayout->version >= _layoutVersion
&& (_calculatedDisplayNodeLayout->requestedLayoutFromAbove == YES
|| CGSizeEqualToSize(_calculatedDisplayNodeLayout->layout.size, boundsSizeForLayout))) {
Expand Down Expand Up @@ -352,8 +352,10 @@ - (void)_locked_measureNodeWithBoundsIfNecessary:(CGRect)bounds
ASLayout *layout = [self calculateLayoutThatFits:constrainedSize
restrictedToSize:self.style.size
relativeToParentSize:boundsSizeForLayout];

nextLayout = std::make_shared<ASDisplayNodeLayout>(layout, constrainedSize, boundsSizeForLayout, version);
// Now that the constrained size of pending layout might have been reused, the layout is useless
// Release it and any orphaned subnodes it retains
_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.

Does this effectively replace the line I commented on here? https://github.com/TextureGroup/Texture/pull/428/files#r126296449

If so, we should probably replace the line at that same location. I think it should not have been removed originally.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, more or less. The main difference is that here only stale or inapplicable pending layouts are cleared, so it can happen that at a given time, both calculated and pending layouts point to a valid layout (and thus can be interchangeable). With the other original line, all pending layouts are cleared, including valid/applied ones.

Copy link
Member

@Adlai-Holler Adlai-Holler Jul 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Why is this line necessary @nguyenhuy ? EDIT: The pending layout is still valid, it's just not valid at our current constrained size. Why is the layoutVersion + currentConstrainedSize not sufficient to tell us whether a given layout is valid?

Copy link
Member Author

@nguyenhuy nguyenhuy Jul 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Either of this line or the new layout version check above fixes part of the problem. The reason I included it is that at this point, the pending layout can be stale (here) and it's never going to be applied. And yet, it still holds onto orphaned subnodes that either will never be inserted, or were/soon-will-be removed by newer layouts (check out -[ASLayout retainSublayoutLayoutElements]).

Edit: the version correction below is still absolutely needed, or we need to rethink the whole _setNeedsLayoutFromAbove strategy.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is true that in case the pending layout can't be applied because of a bounds mismatch, we can keep it around. I'm fine with adding another flag here to handle that edge.

However, as said above, in case _pendingDisplayNodeLayout->version < _layoutVersion, it's quite important to get rid of it.

Copy link
Member

@Adlai-Holler Adlai-Holler Jul 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry I must be missing something big. Why is it important to get rid of it? What's the benefit?

EDIT: Sure in terms of general hygiene we should clear out layouts to reclaim resources when we notice they're out-of-date, but why here?

Copy link
Member

Choose a reason for hiding this comment

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

Oh there's a comment between those two that I missed. OK that makes sense, to release those nodes. We should have a common pattern that in one step checks if a given layout is valid, and discards it if not, and use that everywhere, but yes I'm on the same page about this line now.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

}

if (didCreateNewContext) {
Expand All @@ -373,6 +375,9 @@ - (void)_locked_measureNodeWithBoundsIfNecessary:(CGRect)bounds
// particular ASLayout object, and shouldn't loop asking again unless we have a different ASLayout.
nextLayout->requestedLayoutFromAbove = YES;
[self _setNeedsLayoutFromAbove];
// Update the layout's version here because _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
Expand Down Expand Up @@ -956,7 +961,7 @@ - (void)_locked_setCalculatedDisplayNodeLayout:(std::shared_ptr<ASDisplayNodeLay
_unflattenedLayout = displayNodeLayout->layout;
displayNodeLayout->layout = [_unflattenedLayout filteredNodeLayoutTree];
}

_calculatedDisplayNodeLayout = displayNodeLayout;
}

Expand Down
2 changes: 1 addition & 1 deletion Source/ASDisplayNode.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -920,7 +920,7 @@ - (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];

[self _locked_layoutPlaceholderIfNecessary];
}

Expand Down