[ASCollectionView] Small improvements #407

nguyenhuy · 2017-07-01T22:24:28Z

_ASCollectionReusableView and _ASCollectionViewCell no longer expose getters for their collection element and cell node properties. This is to make sure that clients can only obtain elements from the visible map which is always the source of truth.
Since the map can return nil for an element request, it's much safer to check and avoid adding/removing a nil pointer to an NSArray.
Since we use a special way to check whether an object of kind of _ASCollectionViewCell or _ASCollectionReusableView, based on the assumption that these classes are subclass restricted, I added cast-or-return macros in the header files, closer to where the restrictions are declared.

ghost · 2017-07-01T22:26:17Z

🚫 CI failed with log

ghost · 2017-07-01T22:27:39Z

🚫 CI failed with log

nguyenhuy · 2017-07-01T22:27:42Z

Source/ASCollectionView.mm

-  ASCellNode *cell = [self supplementaryNodeForElementKind:UICollectionElementKindSectionHeader
-                                               atIndexPath:indexPath];
-  if (cell.shouldUseUIKitCell && _asyncDelegateFlags.interop) {
+  ASCollectionElement *element = [_dataController.visibleMap supplementaryElementOfKind:UICollectionElementKindSectionHeader


Because -supplementaryNodeForElementKind:atIndexPath internally calls [_dataController.visibleMap supplementaryElementOfKind:atIndexPath].node, we're literally asking for the element twice in this method. Let's directly reach out to the visible map.

nguyenhuy · 2017-07-01T22:29:35Z

Source/ASCollectionView.mm

+  ASCollectionElement *element = [_dataController.visibleMap supplementaryElementOfKind:UICollectionElementKindSectionHeader
+                                                                            atIndexPath:indexPath];
+  if (element == nil) {
+    return CGSizeZero;


@appleguy In case of interop, should we reach out to the delegate here? Note that since element is nil, element.node.shouldUseUIKitCell must be NO here, so this is not a breaking change.

@nguyenhuy this is a very good and probably important question to resolve some of the crashes I've been trying to get past. I think the answer will come from...in what case can we get here but have a nil element?

If that is expected to be possible / valid, then we probably do need to call the delegate. If it is believed to be invalid, then we should probably add an assertion in the return CGSizeZero case, and not worry about calling the delegate.

I think the answer will come from...in what case can we get here but have a nil element?

I was assuming that clients that use interop can return an item/supplementary view that is not of type _ASCollectionViewCell/_ASCollectionReusableView, and so the element can be nil in these cases.

It turns out that, there is a race condition in ASDataController that can cause an element to be nil. We also got crashes reported at Pinterest with similar symptoms. More details here: #420. Now that we've fixed the root cause, I think the nil checks in this PR is less of a concern, although I still want to keep them should code correctness.

ghost · 2017-07-01T22:34:43Z

🚫 CI failed with log

ghost · 2017-07-01T22:36:03Z

🚫 CI failed with log

ghost · 2017-07-01T22:37:24Z

🚫 CI failed with log

Adlai-Holler · 2017-07-02T19:30:36Z

Source/Details/_ASCollectionReusableView.h

+#define ASCollectionReusableViewCast(x) ({ \
+  id __var = x; \
+  ((_ASCollectionReusableView *) (x.class == [_ASCollectionReusableView class] ? __var : nil)); \
+})


I see that @appleguy started this pattern at line 1100, and that you've formalized it, but it strikes me as serious over-optimization, and I think we should simply remove it and use whatever is most concise at the call site. -isKindOfClass: is extremely fast. Here's the source code, from https://opensource.apple.com/source/objc4/objc4-709/runtime/NSObject.mm

- (BOOL)isKindOfClass:(Class)cls { for (Class tcls = [self class]; tcls; tcls = tcls->superclass) { if (tcls == cls) return YES; } return NO; }

It only includes one method call – which is unavoidable – and otherwise just uses pointer logic.

Changing to isKindOfClass wouldn't simplify the code, because the macro is still needed to have the return-nil behavior. We could use ASDynamicCast, though.

I added the optimization after measuring its overhead. It has the most impact in the UICV compatibility case because we check superclasses' class all the way to NSObject as well. Overall because I would like to use ASCV everywhere, even when no ASCellNodes are being used, it is nice to minimize overhead where it does not add complexity.

If we're feeling generous for this use case, maybe moving it to neighbor ASDynamicCast (ASDynamicCastImmediate ?) would bring the complexity overhead to 0 for this file.

Regarding the class hierarchy, note that the UICV case has subclasses in the app too. Something like (guessing at the hierarchy):

CustomCellClass -> UICollectionViewCell -> UICollectionReusableView -> UIView -> UIResponder -> NSObject

hi, it sounds like we can get some benefits from this pattern. Would be great if you could share some numbers, @appleguy.

Since we already bite the bullet, and the changes in this diff actually make it a bit better, I'd vote for keeping it around. @Adlai-Holler Thoughts?

@nguyenhuy The macro ASCollectionReusableViewCast just doesn't scale well.

How about we add a variant of ASDynamicCast that uses [self class] == [c class]? ASDynamicCastStrict might be a good name.

I dig that. Will follow up tomorrow.

Adlai-Holler · 2017-07-02T19:33:57Z

Source/Details/_ASCollectionReusableView.h

 @property (nonatomic, strong, nullable) UICollectionViewLayoutAttributes *layoutAttributes;
+
+- (void)setElement:(ASCollectionElement *)element;


I don't totally agree with removing the readwrite property. It's true that the visibleMap is the source of truth about what ought to be displayed, but it is useful to be able to read, from a view, what element it is currently displaying in reality. For example, to be able to tell in collectionView:cellForItemAtIndexPath: what element the cell is transitioning from, or to read for debugging purposes. And since these classes are underbarred, I think the risk of external clients leaning on the property is really minor.

Do you have any theories on why we were seeing the misalignment between expected visibleMap and the cell.element?

Is it because layoutIfNeeded had not been flushed and somehow we are getting calls from UIKit about cell appearance with an invalid layout?

Indeed, there is a race condition in ASDataController that could cause the misalignment: #420.

I'm gonna update the code to still retrieve the element from visible map whenever possible, but keep these properties around in case they are needed.

Adlai-Holler

This is a solid cleanup pass. Let's hammer out the questions I raised and then we'll get this landed.

appleguy

@nguyenhuy this is a great set of improvements! I'm wondering if we should rename #trivial to something else, since this is definitely not trivial even if it's not relevant for release notes :).

Let me know if I can help test after you've gone through to look at feedback. This is close enough that I'm comfortable accepting, so land whenever you feel ready.

appleguy · 2017-07-02T23:12:40Z

Source/ASCollectionView.mm

+  ASCollectionElement *element = [_dataController.visibleMap supplementaryElementOfKind:UICollectionElementKindSectionHeader
+                                                                            atIndexPath:indexPath];
+  if (element == nil) {
+    return CGSizeZero;


@nguyenhuy this is a very good and probably important question to resolve some of the crashes I've been trying to get past. I think the answer will come from...in what case can we get here but have a nil element?

If that is expected to be possible / valid, then we probably do need to call the delegate. If it is believed to be invalid, then we should probably add an assertion in the return CGSizeZero case, and not worry about calling the delegate.

appleguy · 2017-07-02T23:14:30Z

Source/ASCollectionView.mm

@@ -1005,7 +1013,7 @@ - (UICollectionReusableView *)collectionView:(UICollectionView *)collectionView
    view = [self dequeueReusableSupplementaryViewOfKind:kind withReuseIdentifier:kReuseIdentifier forIndexPath:indexPath];
  }

-  if (_ASCollectionReusableView *reusableView = ASDynamicCast(view, _ASCollectionReusableView)) {
+  if (_ASCollectionReusableView *reusableView = ASCollectionReusableViewCast(view)) {


IMO this is a very simple, efficient and appropriate check. This is some pretty hardcore infrastructure and we totally control _ASCollection* views, with confidence that they aren't going to have subclasses.

In fact, this check is simpler than using ASDynamicCast and having to manually pass _ASCollectionReusableView.

appleguy · 2017-07-02T23:17:15Z

Source/ASCollectionView.mm

@@ -1086,7 +1096,7 @@ - (void)collectionView:(UICollectionView *)collectionView willDisplayCell:(UICol

  [_rangeController setNeedsUpdate];

-  if (ASSubclassOverridesSelector([ASCellNode class], [cellNode class], @selector(cellNodeVisibilityEvent:inScrollView:withCellFrame:))) {
+  if ([cell consumesCellNodeVisibilityEvents]) {


This is potentially valuable if made into an API that can be implemented optionally on UIKit cells too.

appleguy · 2017-07-02T23:18:07Z

Source/ASCollectionView.mm

@@ -1131,37 +1143,45 @@ - (void)collectionView:(UICollectionView *)collectionView didEndDisplayingCell:(

 - (void)collectionView:(UICollectionView *)collectionView willDisplaySupplementaryView:(UICollectionReusableView *)rawView forElementKind:(NSString *)elementKind atIndexPath:(NSIndexPath *)indexPath
 {
-  if (rawView.class != [_ASCollectionReusableView class]) {
+  ASCollectionReusableViewCastOrReturn(rawView, view, (void)0);


This one seems slightly more magical - maybe worth just using the other cast methods and including the return nil check.

Possible issue: do we really want to return here, before the element is added to the visibleMap?

It seems like for consistency, we should add the element to the visibleMap, even if it is backed by an ASCellNode that uses UIKit / non-_ASCollection* view

It turns out that we can't track visibility of elements that are not backed by a non-_ASCollection* view. That is because in -didEndDisplaying* methods, we can't retrieve the element from visible map since the map might have been updated and no longer holds the element. So we have to rely on cell.element, otherwise we risk having imbalanced add and remove calls on _visibleElements, i.e elements that aren't backed by a non-_ASCollection* view will stay visible forever.

appleguy · 2017-07-02T23:20:21Z

Source/ASCollectionView.mm

-
-  [_visibleElements removeObject:view.element];
+
+  [_visibleElements removeObject:element];


Same question, should we allow removing the element even if the view is a non-_AS cellview?

appleguy · 2017-07-02T23:25:06Z

Source/ASCollectionView.mm

-    if (_asyncDelegateFlags.scrollViewDidEndDragging) {
-        [_asyncDelegate scrollViewDidEndDragging:scrollView willDecelerate:decelerate];
-    }
+  for (_ASCollectionViewCell *cell in _cellsForVisibilityUpdates) {


Is it valid to use this type when iterating over _cellsForVisibilityUpdates?

The gating factor to add things to this array is whether the cell consumes visibility events, but this might theoretically happen for non-_AS cells too.

Maybe we should make the _cellsForVisibilityUpdates collection typed to <id > or something.

If this already works correctly because of the early-returns checking for the _AS classes that occur before the cells are added, then that's OK for now.

I'm gonna keep this as is as we do make sure only _ASCollectionViewCell are added to this collection. We'll update it to id when we support non-_AS cells.

appleguy · 2017-07-02T23:26:29Z

Source/Details/_ASCollectionViewCell.h

+ * Whether or not this cell is interested in cell node visibility events.
+ * -cellNodeVisibilityEvent:inScrollView: should be called only if this property is YES.
+ */
+@property (nonatomic, readonly) BOOL consumesCellNodeVisibilityEvents;


Did these events ever work for Supplementary views? It would be nice if they did, particularly because they use the same ASCellNode and the API is exposed there, so it would be unclear at a user level if they did not get called.

No, this has never supported supplementary views. This API is in _ASCollectionViewCell so I think it's pretty clear that only _AS item cells are considered. We can of course extend it to work on other types of cells or supplementary views, but it is not in the scope of this PR.

- `_ASCollectionReusableView` and `_ASCollectionViewCell` no longer expose getters for their collection element and cell node properties. This is to make sure that clients can only obtain elements from the visible map which is always the source of truth. - Since the map can return `nil` for an element request, it's much safer to check and avoid adding/removing a nil pointer to an `NSArray`. - Since we use a special way to check whether an object of kind of `_ASCollectionViewCell` or `_ASCollectionReusableView`, based on the assumption that these classes are subclass restricted, I added cast-or-return macros in the header files, closer to where the restrictions are declared.

… _ASCollectionViewCell

…-_ASCollection* view

ghost · 2017-07-06T12:11:57Z

🚫 CI failed with log

ghost · 2017-07-06T12:16:20Z

🚫 CI failed with log

ghost · 2017-07-06T12:17:56Z

🚫 CI failed with log

ghost · 2017-07-06T12:22:11Z

🚫 CI failed with log

ghost · 2017-07-06T12:23:43Z

🚫 CI failed with log

ghost · 2017-07-06T12:25:04Z

🚫 CI failed with log

nguyenhuy · 2017-07-06T13:23:00Z

@Adlai-Holler This PR is ready for another round of review. Thanks!

Adlai-Holler · 2017-07-06T17:32:10Z

Source/Details/_ASCollectionViewCell.h


 NS_ASSUME_NONNULL_BEGIN

-AS_SUBCLASSING_RESTRICTED
+AS_SUBCLASSING_RESTRICTED // Note: ASDynamicCastStrict is used on instances of this class based on this restriction.


Adlai-Holler

Great!

* Minor refactors in ASCollectionView and its private cell classes - `_ASCollectionReusableView` and `_ASCollectionViewCell` no longer expose getters for their collection element and cell node properties. This is to make sure that clients can only obtain elements from the visible map which is always the source of truth. - Since the map can return `nil` for an element request, it's much safer to check and avoid adding/removing a nil pointer to an `NSArray`. - Since we use a special way to check whether an object of kind of `_ASCollectionViewCell` or `_ASCollectionReusableView`, based on the assumption that these classes are subclass restricted, I added cast-or-return macros in the header files, closer to where the restrictions are declared. * Add ASDynamicCastStrict * Add element and node properties back to _ASCollectionReusableView and _ASCollectionViewCell * Assert unexpected nil elements * Always mark an element visible even if it is backed by an UIKit / non-_ASCollection* view * Fix typo * Remove unnecessary changes * Dump mistakes * Update CHANGELOG * Can't track visibility of elements backed by non-_AS views

nguyenhuy force-pushed the HNMissingCollectionElement branch from e301d7b to e687f01 Compare July 1, 2017 22:24

nguyenhuy changed the title ~~[ASCollectionView] Minor refactors in ASCollectionView and its private cell classes~~ [ASCollectionView] Minor refactors in ASCollectionView and its private cell classes #trivial Jul 1, 2017

nguyenhuy commented Jul 1, 2017

View reviewed changes

nguyenhuy force-pushed the HNMissingCollectionElement branch 2 times, most recently from a86b1b1 to e292521 Compare July 1, 2017 22:34

nguyenhuy force-pushed the HNMissingCollectionElement branch from e292521 to 7303caf Compare July 1, 2017 22:35

nguyenhuy force-pushed the HNMissingCollectionElement branch from 7303caf to 9fde42d Compare July 1, 2017 22:40

Adlai-Holler reviewed Jul 2, 2017

View reviewed changes

appleguy approved these changes Jul 2, 2017

View reviewed changes

nguyenhuy added 5 commits July 6, 2017 12:23

Add ASDynamicCastStrict

e32ad6d

Add element and node properties back to _ASCollectionReusableView and…

33a9c4d

… _ASCollectionViewCell

Assert unexpected nil elements

bd31b05

Always mark an element visible even if it is backed by an UIKit / non…

2bcf410

…-_ASCollection* view

nguyenhuy force-pushed the HNMissingCollectionElement branch from 9fde42d to 2bcf410 Compare July 6, 2017 12:10

Fix typo

8b0dfe0

Remove unnecessary changes

e384428

nguyenhuy changed the title ~~[ASCollectionView] Minor refactors in ASCollectionView and its private cell classes #trivial~~ [ASCollectionView] Small improvements Jul 6, 2017

nguyenhuy force-pushed the HNMissingCollectionElement branch from 70c6d9c to 940910d Compare July 6, 2017 12:23

nguyenhuy added 3 commits July 6, 2017 13:30

Dump mistakes

4b9f906

Update CHANGELOG

c5f8754

Can't track visibility of elements backed by non-_AS views

8726c98

nguyenhuy force-pushed the HNMissingCollectionElement branch from 940910d to 8726c98 Compare July 6, 2017 13:12

Adlai-Holler reviewed Jul 6, 2017

View reviewed changes

Adlai-Holler approved these changes Jul 6, 2017

View reviewed changes

nguyenhuy merged commit e78d70e into TextureGroup:master Jul 6, 2017

		@property (nonatomic, strong, nullable) UICollectionViewLayoutAttributes *layoutAttributes;

		- (void)setElement:(ASCollectionElement *)element;


		[_visibleElements removeObject:view.element];

		[_visibleElements removeObject:element];

[ASCollectionView] Small improvements #407

[ASCollectionView] Small improvements #407

Conversation

nguyenhuy commented Jul 1, 2017

ghost commented Jul 1, 2017

ghost commented Jul 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 1, 2017

ghost commented Jul 1, 2017

ghost commented Jul 1, 2017

Adlai-Holler Jul 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adlai-Holler left a comment

Choose a reason for hiding this comment

appleguy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 6, 2017

ghost commented Jul 6, 2017

ghost commented Jul 6, 2017

ghost commented Jul 6, 2017

ghost commented Jul 6, 2017

ghost commented Jul 6, 2017

nguyenhuy commented Jul 6, 2017

Choose a reason for hiding this comment

Adlai-Holler left a comment

Choose a reason for hiding this comment

Adlai-Holler Jul 2, 2017 •

edited