Implement ASPageTable #81

nguyenhuy · 2017-04-28T11:40:11Z

It is a screen page table that can be used to quickly filter out objects in a certain rect without checking each and every one of them.
ASCollectionLayoutState generates and keeps a table that maps page to layout attributes within that page.
ASCollectionLayout (and later, ASCollectionGalleryLayoutDelegate) consults its layout state for layoutAttributesForElementsInRect:. This ensures the method can return as quickly as possible, especially on a large data set (I heard some people have galleries with thousands of photos!).

This PR was extracted from #76. Ticket: #186

ghost · 2017-04-28T11:59:33Z

🚫 CI failed with log

Adlai-Holler

@nguyenhuy Awesome! We both know how much friggin time the range controller spends querying layoutAttributesForElementsInRect:, and stupid UIKit won't expose their cached version to us, so this is a major step forward.

Adlai-Holler · 2017-04-28T15:20:07Z

Source/Details/ASCollectionFlowLayoutDelegate.m

-                                elementToLayoutArrtibutesMap:[NSMapTable weakToStrongObjectsMapTable]];
+    return [[ASCollectionLayoutState alloc] initWithContext:context
+                                                contentSize:CGSizeZero
+                             elementToLayoutArrtibutesTable:[NSMapTable weakToStrongObjectsMapTable]];


There was a typo here we should fix – Attributes

While we're at it, we should initialize this map table with NSObjectPointerPersonality | NSMapTableWeakMemory so that we get pointer equality/hashing. Maybe it's worth making a convenience initializer that specifies these options.

Adlai-Holler · 2017-04-28T15:28:45Z

Source/Details/ASCollectionLayoutState.m

+  NSMutableArray<UICollectionViewLayoutAttributes *> *results = [NSMutableArray array];
+  for (ASPageCoordinates *page in _pageToLayoutAttributesTable) {
+    NSArray<UICollectionViewLayoutAttributes *> *allAttrs = [[_pageToLayoutAttributesTable objectForKey:page] allObjects];
+    CGRect pageRect = [page pageRectForPageSize:pageSize];


Since this is such a hot path, it's probably worth only calling -allObjects inside the CGRectContainsRect branch, since it will allocate memory.

Adlai-Holler · 2017-04-28T16:09:16Z

Source/Details/ASPageTable.h

+@property (nonatomic, assign, readonly) NSUInteger x;
+@property (nonatomic, assign, readonly) NSUInteger y;
+
+ (instancetype)pageCoordinatesWithX:(NSUInteger)x Y:(NSUInteger)y;


Since these are going to be created a lot I think it's worth optimizing upfront. Let's pack the coordinates into an integer and trick NSMapTable into using it.

typedef struct { uint16_t x, uint16_t y } ASPageCoordinate; __unsafe_unretained id ASPageCoordinateMakeKey(ASPageCoordinate coordinate) { return (__bridge id)(void *)((coordinate.x << 16) + coordinate.y + 1); }

We add 1 because 0 is not a valid key for NSMapTable.

It might be worth asserting that x, y < (1 << 16) and masking x,y & 0xFFFF.

Then create the map table with pointerFunctionsWithOptions:NSPointerFunctionsIntegerPersonality | NSPointerFunctionsOpaqueMemory and use [mapTable objectForKey:ASPageCoordinateMakeKey(coord)] to get your value.

Good point. At first, ASPageCoordinate was actually implemented as a struct. But back then I was not sure if I'd use it frequently enough to justify the NSMapTable and NSArray tricks, so I made it an object. Now that we have a better idea of its usage, let me convert it back to a struct =))

True, an integer is preferable even to a struct, since it'll avoid malloc/free. That said, if the code is cleaner the code is cleaner and that's mostly what matters.

It might be reasonable to use an inlined or #define accessor to unpack the x / y. CGRectGetMinX() type function, could be ASCoordinateX(coordinate). Might not be worth it though.

@Adlai-Holler @appleguy I've pushed a new change that turns ASPageCoordinate into an uint32_t. And yes, we need ASPageCoordinateGetX() and ASPageCoordinateGetY(). Please have another look when you have time!

Adlai-Holler · 2017-04-28T16:14:25Z

Source/Private/ASCollectionLayout.mm

+    return @[];
+  }
+
+  ASCollectionLayoutState *layout = _layout;


No need to create these local copies of strong ivars – since the ivar isn't marked volatile the compiler is free to cache the value in a register. If there's gonna be a lot of underscores, this can make things prettier, and if the ivar is weak, then we definitely ought to get a strong local copy.

Adlai-Holler · 2017-04-28T16:18:11Z

Source/Details/ASPageTable.mm

+  dispatch_once(&onceToken, ^{
+    weakObjectPointerFuncs = [NSPointerFunctions pointerFunctionsWithOptions:NSPointerFunctionsWeakMemory | NSPointerFunctionsObjectPointerPersonality];
+  });
+  return [self pageTableWithValuePointerFunctions:weakObjectPointerFuncs];


Note: Personality doesn't matter for values, since they're never hashed/equaled.

Adlai-Holler · 2017-04-28T16:19:01Z

Source/Details/ASPageTable.mm

+
+ (NSArray<ASPageCoordinates *> *)pageCoordinatesForPagesInRect:(CGRect)rect withContentSize:(CGSize)contentSize pageSize:(CGSize)pageSize
+{
+  CGRect contentRect = CGRectMake(0.0f, 0.0f, contentSize.width, contentSize.height);


Let's never use 3.0f 0.0f etc – the compiler will choose the appropriate type. CGFloat is double on 64-bit, after all.

Adlai-Holler · 2017-04-28T16:19:41Z

Source/Details/ASPageTable.h

+ *
+ * @param pageSize The size of each page.
+ */
+ (ASPageTable<ASPageCoordinates *, NSMutableSet<UICollectionViewLayoutAttributes *> *> *)pageTableWithLayoutAttributes:(NSArray<UICollectionViewLayoutAttributes *> *)layoutAttributes pageSize:(CGSize)pageSize;


It may not be worth it, but we could make the layoutAttributes argument id<NSFastEnumeration> and then pass the map table's object enumerator directly, rather than forming an array.

Actually keyEnumerator and objectEnumerator of NSMapTable are of type NSEnumerator. But yes, it's better to just accept an enumerator.

NSEnumerator conforms to NSFastEnumeration

Oh yeah, that's true! Thanks for clarifying

Adlai-Holler · 2017-04-28T16:21:35Z

Source/Details/ASCollectionLayoutDelegate.h

+ *
+ * @discussion This method will be called on main thread.
+ */
+- (nullable id)additionalInfoForLayoutWithElements:(ASElementMap *)elements;


Let's make this required – they can always return nil if they want, and that way they are sure to know that this method exists (and we don't have to ask the runtime whether they implement.)

Adlai-Holler

Sweeeeet! One more round of refinements and then let's move forward. Feel free to merge this if you address these and want to do more work when it's the middle-of-the-night in Cali.

Adlai-Holler · 2017-05-03T22:02:58Z

Source/Details/ASCollectionLayoutState.h

-/// Element to layout attributes map. Should use weak pointers for elements.
-@property (nonatomic, strong, readonly) NSMapTable<ASCollectionElement *, UICollectionViewLayoutAttributes *> *elementToLayoutArrtibutesMap;
+/// The final content rect calculated from content size
+@property (nonatomic, assign, readonly) CGRect contentRect;


Let's make this contentSize since the origin will always be 0

Adlai-Holler · 2017-05-03T22:03:46Z

Source/Details/ASCollectionLayoutState.h

+ *
+ * @param rect The rect containing the target elements.
+ */
+- (nullable NSArray<UICollectionViewLayoutAttributes *> *)layoutAttributesForElementsInRect:(CGRect)rect;


Let's make this non-null, since @[] is free.

Adlai-Holler · 2017-05-03T22:09:24Z

Source/Details/ASPageTable.mm

+{
+  ASPageTable *result = [ASPageTable pageTableForStrongObjectPointers];
+  for (UICollectionViewLayoutAttributes *attrs in layoutAttributesEnumerator) {
+    ASPageCoordinate page = ASPageCoordinateForPageThatContainsPoint(attrs.frame.origin, pageSize);


I think we need to key this on page-that-intersects-rect. You can imagine an item that spans 10 pages – we need to make sure we include that one whenever we search any of those 10 pages.

Great catch! Fixed.

Adlai-Holler · 2017-05-03T22:10:06Z

Source/Details/ASCollectionLayoutState.m

+    return nil;
+  }
+
+  NSMutableArray<UICollectionViewLayoutAttributes *> *results = [NSMutableArray array];


Let's make this a mutable set, since layout attributes could be span multiple pages and we want to dedupe (see previous comment).

Adlai-Holler · 2017-05-03T22:10:35Z

Source/Details/ASCollectionLayoutState.m

+
+@implementation ASCollectionLayoutState {
+  NSMapTable<ASCollectionElement *,UICollectionViewLayoutAttributes *> *_elementToLayoutAttributesTable;
+  ASPageTable<id, NSMutableSet<UICollectionViewLayoutAttributes *> *> *_pageToLayoutAttributesTable;


Let's have the value-type here be array rather than set. That way we avoid hashing, equality, and converting-to-array.

Adlai-Holler · 2017-05-03T22:11:34Z

Source/Details/ASPageTable.mm

+    return nil;
+  }
+
+  NSPointerArray *results = [NSPointerArray pointerArrayWithOptions:(NSPointerFunctionsIntegerPersonality | NSPointerFunctionsOpaqueMemory)];


This is clever! Nice!

Adlai-Holler · 2017-05-03T22:14:29Z

Source/Details/ASPageTable.mm

@@ -0,0 +1,146 @@
+//
+//  ASPageTable.mm


Looks like there's no C++ in this file, let's drop it down to .m.

Adlai-Holler · 2017-05-03T22:14:45Z

Source/Details/ASPageTable.h

+ *
+ * @param page A page to lookup the object for.
+ */
+- (ObjectType)objectForPage:(ASPageCoordinate)page;


Mark this nullable

Adlai-Holler · 2017-05-03T22:16:01Z

Source/Details/ASCollectionLayoutState.h

+/**
+ * Returns all layout attributes present in this object.
+ */
+- (NSArray<UICollectionViewLayoutAttributes *> *)allLayoutAttributes;


Looks like this isn't currently used. Unless you have a specific use in mind, let's remove for now.

It's used by the coming gallery layout delegate.

- It is a screen page table that can be used to quickly filter out objects in a certain rect without checking each and every one of them. - ASCollectionLayoutState generates and keeps a table that maps page to layout attributes within that page. - ASCollectionLayout (and later, ASCollectionGalleryLayoutDelegate) consults its layout state for `layoutAttributesForElementsInRect:`. This ensures the method can return as quickly as possible, especially on a large data set (I heard some people have galleries with thousands of photos!).

ghost · 2017-05-04T11:14:29Z

	1 Warning
⚠️	This is a big PR, please consider splitting it up to ease code review.

Generated by 🚫 Danger

@Adlai-Holler

Approved by @Adlai-Holler

nguyenhuy mentioned this pull request Apr 28, 2017

Introduce ASCollectionGalleryLayoutDelegate #76

Merged

nguyenhuy force-pushed the HNPageTable branch from 1ca103e to 4242ae9 Compare April 28, 2017 12:31

nguyenhuy requested a review from Adlai-Holler April 28, 2017 12:31

Adlai-Holler previously requested changes Apr 28, 2017

View reviewed changes

nguyenhuy force-pushed the HNPageTable branch 4 times, most recently from de85f8d to 2faf387 Compare May 3, 2017 16:21

Adlai-Holler reviewed May 3, 2017

View reviewed changes

nguyenhuy added 3 commits May 4, 2017 11:35

Address comments

02d7153

Handle items that span multiple pages

492004b

nguyenhuy force-pushed the HNPageTable branch from 2faf387 to 492004b Compare May 4, 2017 11:11

Make danger happy

f53b3da

nguyenhuy merged commit ca2885c into TextureGroup:master May 4, 2017

CoderXL mentioned this pull request Sep 28, 2019

ios13 -[ASTextKitContext initWithAttributedString:lineBreakMode:maximumNumberOfLines:exclusionPaths:constrainedSize:] deadlock appear in rare cases #1690

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ASPageTable #81

Implement ASPageTable #81

nguyenhuy commented Apr 28, 2017 •

edited

Loading

ghost commented Apr 28, 2017

Adlai-Holler left a comment

Adlai-Holler Apr 28, 2017

Adlai-Holler Apr 28, 2017

Adlai-Holler Apr 28, 2017

Adlai-Holler Apr 28, 2017

nguyenhuy Apr 28, 2017

Adlai-Holler Apr 28, 2017

appleguy Apr 30, 2017

nguyenhuy May 3, 2017

Adlai-Holler Apr 28, 2017

nguyenhuy Apr 28, 2017

Adlai-Holler Apr 28, 2017

Adlai-Holler Apr 28, 2017

Adlai-Holler Apr 28, 2017

nguyenhuy Apr 28, 2017

nguyenhuy Apr 28, 2017 •

edited

Loading

Adlai-Holler Apr 28, 2017

nguyenhuy Apr 28, 2017

Adlai-Holler Apr 28, 2017

Adlai-Holler left a comment

Adlai-Holler May 3, 2017

Adlai-Holler May 3, 2017

Adlai-Holler May 3, 2017

nguyenhuy May 4, 2017

Adlai-Holler May 3, 2017

Adlai-Holler May 3, 2017

Adlai-Holler May 3, 2017

Adlai-Holler May 3, 2017

Adlai-Holler May 3, 2017

Adlai-Holler May 3, 2017

nguyenhuy May 4, 2017

ghost commented May 4, 2017

Implement ASPageTable #81

Implement ASPageTable #81

Conversation

nguyenhuy commented Apr 28, 2017 • edited Loading

ghost commented Apr 28, 2017

Adlai-Holler 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenhuy Apr 28, 2017 • edited Loading

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

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 May 4, 2017

nguyenhuy commented Apr 28, 2017 •

edited

Loading

nguyenhuy Apr 28, 2017 •

edited

Loading