Implement ASPageTable

[nguyenhuy]

• 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]

🚫 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]

There was a typo here we should fix – Attributes

[Adlai-Holler]

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]

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]

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.

[nguyenhuy]

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 =))

[Adlai-Holler]

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.

[appleguy]

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.

[nguyenhuy]

@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]

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.

[nguyenhuy]

👍

[Adlai-Holler]

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

[Adlai-Holler]

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]

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.

[nguyenhuy]

👍

[nguyenhuy]

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

[Adlai-Holler]

NSEnumerator conforms to NSFastEnumeration

[nguyenhuy]

Oh yeah, that's true! Thanks for clarifying

[Adlai-Holler]

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]

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

[Adlai-Holler]

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

[Adlai-Holler]

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.

[nguyenhuy]

Great catch! Fixed.

[Adlai-Holler]

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]

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

[Adlai-Holler]

This is clever! Nice!

[Adlai-Holler]

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

[Adlai-Holler]

Mark this nullable

[Adlai-Holler]

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

[nguyenhuy]

It's used by the coming gallery layout delegate.

[ghost]

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

Generated by 🚫 Danger

Approved by @Adlai-Holler