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.

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

Faster collection operations #748

Merged
merged 6 commits into from
Jan 22, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
buildConfiguration = "Debug"
selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
disableMainThreadChecker = "YES"
Copy link
Member Author

Choose a reason for hiding this comment

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

This disables the main thread checker for unit tests and there are cases where we expect to call UIKit methods off-main during unit tests.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, I'm a bit curious which things we perform off-main intentionally in tests? I wonder if there is a way to do that in-situ, since the general existence of this checker does have value (e.g. if you were to accidentally call UIKit on main in a regression sense, then a unit test could catch it)

launchStyle = "0"
useCustomWorkingDirectory = "NO"
ignoresPersistentStateOnLaunch = "NO"
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
- [ASCollectionNode] Added support for interactive item movement. [Adlai Holler](https://github.com/Adlai-Holler)
- Added an experimental "no-copy" rendering API. See ASGraphicsContext.h for info. [Adlai Holler](https://github.com/Adlai-Holler)
- Dropped support for iOS 8. [Adlai Holler](https://github.com/Adlai-Holler)
- Optimize internal collection operations. [Adlai Holler](https://github.com/Adlai-Holler) [#748](https://github.com/TextureGroup/Texture/pull/748)

## 2.6
- [Xcode 9] Updated to require Xcode 9 (to fix warnings) [Garrett Moon](https://github.com/garrettmoon)
Expand Down
39 changes: 9 additions & 30 deletions Source/ASCollectionView.mm
Original file line number Diff line number Diff line change
Expand Up @@ -719,19 +719,9 @@ - (NSIndexPath *)convertIndexPathToCollectionNode:(NSIndexPath *)indexPath

- (NSArray<NSIndexPath *> *)convertIndexPathsToCollectionNode:(NSArray<NSIndexPath *> *)indexPaths
{
if (indexPaths == nil) {
return nil;
}

NSMutableArray<NSIndexPath *> *indexPathsArray = [NSMutableArray arrayWithCapacity:indexPaths.count];

for (NSIndexPath *indexPathInView in indexPaths) {
NSIndexPath *indexPath = [self convertIndexPathToCollectionNode:indexPathInView];
if (indexPath != nil) {
[indexPathsArray addObject:indexPath];
}
}
return indexPathsArray;
return ASArrayByFlatMapping(indexPaths, NSIndexPath *indexPathInView, ({
[self convertIndexPathToCollectionNode:indexPathInView];
}));
}

- (ASCellNode *)supplementaryNodeForElementKind:(NSString *)elementKind atIndexPath:(NSIndexPath *)indexPath
Expand All @@ -747,17 +737,10 @@ - (NSIndexPath *)indexPathForNode:(ASCellNode *)cellNode
- (NSArray *)visibleNodes
{
NSArray *indexPaths = [self indexPathsForVisibleItems];
NSMutableArray *visibleNodes = [[NSMutableArray alloc] init];

for (NSIndexPath *indexPath in indexPaths) {
ASCellNode *node = [self nodeForItemAtIndexPath:indexPath];
if (node) {
// It is possible for UICollectionView to return indexPaths before the node is completed.
[visibleNodes addObject:node];
}
}

return visibleNodes;
return ASArrayByFlatMapping(indexPaths, NSIndexPath *indexPath, ({
[self nodeForItemAtIndexPath:indexPath];
}));
}

- (BOOL)usesSynchronousDataLoading
Expand Down Expand Up @@ -2176,13 +2159,9 @@ - (void)nodesDidRelayout:(NSArray<ASCellNode *> *)nodes
return;
}

NSMutableArray<NSIndexPath *> *uikitIndexPaths = [NSMutableArray arrayWithCapacity:nodes.count];
for (ASCellNode *node in nodes) {
NSIndexPath *uikitIndexPath = [self indexPathForNode:node];
if (uikitIndexPath != nil) {
[uikitIndexPaths addObject:uikitIndexPath];
}
}
NSArray *uikitIndexPaths = ASArrayByFlatMapping(nodes, ASCellNode *node, ({
[self indexPathForNode:node];
}));

[_layoutFacilitator collectionViewWillEditCellsAtIndexPaths:uikitIndexPaths batched:NO];

Expand Down
10 changes: 4 additions & 6 deletions Source/ASDisplayNode+Yoga.mm
Original file line number Diff line number Diff line change
Expand Up @@ -158,14 +158,12 @@ - (ASLayout *)layoutForYogaNode
- (void)setupYogaCalculatedLayout
{
YGNodeRef yogaNode = self.style.yogaNode;
uint32_t childCount = YGNodeGetChildCount(yogaNode);
ASDisplayNodeAssert(childCount == self.yogaChildren.count,
ASDisplayNodeAssert(YGNodeGetChildCount(yogaNode) == self.yogaChildren.count,
@"Yoga tree should always be in sync with .yogaNodes array! %@", self.yogaChildren);

NSMutableArray *sublayouts = [NSMutableArray arrayWithCapacity:childCount];
for (ASDisplayNode *subnode in self.yogaChildren) {
[sublayouts addObject:[subnode layoutForYogaNode]];
}
NSArray *sublayouts = ASArrayByFlatMapping(self.yogaChildren, ASDisplayNode *subnode, ({
[subnode layoutForYogaNode];
}));

// The layout for self should have position CGPointNull, but include the calculated size.
CGSize size = CGSizeMake(YGNodeLayoutGetWidth(yogaNode), YGNodeLayoutGetHeight(yogaNode));
Expand Down
12 changes: 3 additions & 9 deletions Source/ASTableView.mm
Original file line number Diff line number Diff line change
Expand Up @@ -607,15 +607,9 @@ - (NSIndexPath *)convertIndexPathToTableNode:(NSIndexPath *)indexPath
return nil;
}

NSMutableArray<NSIndexPath *> *indexPathsArray = [NSMutableArray new];

for (NSIndexPath *indexPathInView in indexPaths) {
NSIndexPath *indexPath = [self convertIndexPathToTableNode:indexPathInView];
if (indexPath != nil) {
[indexPathsArray addObject:indexPath];
}
}
return indexPathsArray;
return ASArrayByFlatMapping(indexPaths, NSIndexPath *indexPathInView, ({
[self convertIndexPathToTableNode:indexPathInView];
}));
}

- (NSIndexPath *)indexPathForNode:(ASCellNode *)cellNode
Expand Down
83 changes: 52 additions & 31 deletions Source/Base/ASBaseDefines.h
Original file line number Diff line number Diff line change
Expand Up @@ -132,22 +132,6 @@
#define __has_attribute(x) 0 // Compatibility with non-clang compilers.
#endif

#ifndef NS_CONSUMED
#if __has_feature(attribute_ns_consumed)
#define NS_CONSUMED __attribute__((ns_consumed))
#else
#define NS_CONSUMED
#endif
#endif

#ifndef NS_RETURNS_RETAINED
#if __has_feature(attribute_ns_returns_retained)
#define NS_RETURNS_RETAINED __attribute__((ns_returns_retained))
#else
#define NS_RETURNS_RETAINED
#endif
#endif

#ifndef CF_RETURNS_RETAINED
#if __has_feature(attribute_cf_returns_retained)
#define CF_RETURNS_RETAINED __attribute__((cf_returns_retained))
Expand Down Expand Up @@ -245,26 +229,38 @@
* Create a new set by mapping `collection` over `work`, ignoring nil.
*/
#define ASSetByFlatMapping(collection, decl, work) ({ \
NSMutableSet *s = [NSMutableSet set]; \
CFTypeRef _cArray[collection.count]; \
NSUInteger _i = 0; \
for (decl in collection) {\
id result = work; \
if (result != nil) { \
[s addObject:result]; \
if ((_cArray[_i] = (__bridge_retained CFTypeRef)work)) { \
_i++; \
} \
} \
s; \
NSSet *result; \
if (_i == 0) { \
/** Zero fast path. */ \
result = [NSSet set]; \
} else if (_i == 1) { \
/** NSSingleObjectSet is fast. Create one and release. */ \
CFTypeRef val = _cArray[0]; \
result = [NSSet setWithObject:(__bridge id)val]; \
CFBridgingRelease(val); \
} else { \
CFSetCallBacks cb = kCFTypeSetCallBacks; \
cb.retain = NULL; \
result = (__bridge NSSet *)CFSetCreate(kCFAllocatorDefault, _cArray, _i, &cb); \
} \
result; \
})

/**
* Create a new ObjectPointerPersonality NSHashTable by mapping `collection` over `work`, ignoring nil.
*/
#define ASPointerTableByFlatMapping(collection, decl, work) ({ \
Copy link
Member

Choose a reason for hiding this comment

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

Is this one not yet used? Just wanted to say, I think this #define-based approach is very powerful as an optimization, in its ability to avoid wrapping and retains that most other mechanisms lead to. We should use it judiciously, but I bet there are a few more cases where it is justified and impactful.

NSHashTable *t = [NSHashTable hashTableWithOptions:NSHashTableObjectPointerPersonality]; \
NSHashTable *t = [[NSHashTable alloc] initWithOptions:NSHashTableObjectPointerPersonality capacity:collection.count]; \
for (decl in collection) {\
id result = work; \
if (result != nil) { \
[t addObject:result]; \
} \
/* NSHashTable accepts nil and avoid extra retain/release. */ \
[t addObject:work]; \
} \
t; \
})
Expand All @@ -273,12 +269,37 @@
* Create a new array by mapping `collection` over `work`, ignoring nil.
*/
#define ASArrayByFlatMapping(collection, decl, work) ({ \
NSMutableArray *a = [NSMutableArray array]; \
CFTypeRef _cArray[collection.count]; \
NSUInteger _i = 0; \
for (decl in collection) {\
if ((_cArray[_i] = (__bridge_retained CFTypeRef)work)) { \
_i++; \
} \
} \
NSArray *result; \
if (_i == 0) { \
/** Zero array fast path. */ \
result = @[]; \
} else if (_i == 1) { \
/** NSSingleObjectArray is fast. Create one and release. */ \
CFTypeRef val = _cArray[0]; \
result = [NSArray arrayWithObject:(__bridge id)val]; \
CFBridgingRelease(val); \
} else { \
CFArrayCallBacks cb = kCFTypeArrayCallBacks; \
cb.retain = NULL; \
result = (__bridge NSArray *)CFArrayCreate(kCFAllocatorDefault, _cArray, _i, &cb); \
} \
Copy link
Member Author

@Adlai-Holler Adlai-Holler Jan 16, 2018

Choose a reason for hiding this comment

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

I'm going to factor this out a bit and clean it up, but here's the approach:

  • We need to safely retain the result of the work (if the work's result isn't retained e.g. [NSObject new]. Only ARC knows!)
  • We want to transfer that retain into the array, instead of having it retain and then we release.
  • The way is, use __bridge_retained to tell ARC "make sure this is a +1, and I'll take responsibility for it."
  • Then when you create the array, use CFArray and provide NULL for the retain callback. Boom, CFArray won't retain, but it will release when the array goes away.
  • Fast paths for 0- or 1- element collections e.g. __NSArray0 and __NSSingleObjectArray.
  • Not possible with id [] because ARC will always release the elements when the symbol goes out of scope – there is no CF_CONSUMED for c-arrays.
  • Down from (2N retains, N) releases to (N retains, 0 releases). Plus we get an immutable result, so free copying.

TODO: If someone calls CFArrayCreateCopy, there's no short-circuit and the callbacks will be copied as well, so the objects will get overreleased. Same if they use -mutableCopy. The solution is to create a retain callback that checks for the presence of a pthread_key that we will set during array creation. If the key is there, skip the retain. Otherwise, retain normally.

result; \
})

#define ASMutableArrayByFlatMapping(collection, decl, work) ({ \
id _cArray[collection.count]; \
NSUInteger _i = 0; \
for (decl in collection) {\
id result = work; \
if (result != nil) { \
[a addObject:result]; \
if ((_cArray[_i] = work)) { \
_i++; \
} \
} \
a; \
[[NSMutableArray alloc] initWithObjects:_cArray count:_i]; \
})
2 changes: 1 addition & 1 deletion Source/Details/ASCollectionFlowLayoutDelegate.m
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ - (id)additionalInfoForLayoutWithElements:(ASElementMap *)elements
+ (ASCollectionLayoutState *)calculateLayoutWithContext:(ASCollectionLayoutContext *)context
{
ASElementMap *elements = context.elements;
NSMutableArray<ASCellNode *> *children = ASArrayByFlatMapping(elements.itemElements, ASCollectionElement *element, element.node);
NSArray<ASCellNode *> *children = ASArrayByFlatMapping(elements.itemElements, ASCollectionElement *element, element.node);
if (children.count == 0) {
return [[ASCollectionLayoutState alloc] initWithContext:context];
}
Expand Down
6 changes: 3 additions & 3 deletions Source/Details/ASCollectionGalleryLayoutDelegate.mm
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,9 @@ + (ASCollectionLayoutState *)calculateLayoutWithContext:(ASCollectionLayoutConte
return [[ASCollectionLayoutState alloc] initWithContext:context];
}

NSMutableArray<_ASGalleryLayoutItem *> *children = ASArrayByFlatMapping(elements.itemElements,
ASCollectionElement *element,
[[_ASGalleryLayoutItem alloc] initWithItemSize:itemSize collectionElement:element]);
NSArray<_ASGalleryLayoutItem *> *children = ASArrayByFlatMapping(elements.itemElements,
ASCollectionElement *element,
[[_ASGalleryLayoutItem alloc] initWithItemSize:itemSize collectionElement:element]);
if (children.count == 0) {
return [[ASCollectionLayoutState alloc] initWithContext:context];
}
Expand Down
5 changes: 5 additions & 0 deletions Source/Details/ASElementMap.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ NS_ASSUME_NONNULL_BEGIN
AS_SUBCLASSING_RESTRICTED
@interface ASElementMap : NSObject <NSCopying, NSFastEnumeration>

/**
* The total number of elements in this map.
*/
@property (readonly) NSUInteger count;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is required by the new implementation of ASArrayByFlatMapping, in addition to NSFastEnumeration conformance.


/**
* The number of sections (of items) in this map.
*/
Expand Down
5 changes: 5 additions & 0 deletions Source/Details/ASElementMap.m
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,11 @@ - (instancetype)initWithSections:(NSArray<ASSection *> *)sections items:(ASColle
return self;
}

- (NSUInteger)count
{
return _elementToIndexPathMap.count;
}

- (NSArray<NSIndexPath *> *)itemIndexPaths
{
return ASIndexPathsForTwoDimensionalArray(_sectionsOfItems);
Expand Down
17 changes: 7 additions & 10 deletions Source/Layout/ASLayout.mm
Original file line number Diff line number Diff line change
Expand Up @@ -203,13 +203,9 @@ - (void)setRetainSublayoutLayoutElements:(BOOL)retainSublayoutLayoutElements
_sublayoutLayoutElements = nil;
} else {
// Add sublayouts layout elements to an internal array to retain it while the layout lives
NSUInteger sublayoutCount = _sublayouts.count;
if (sublayoutCount > 0) {
_sublayoutLayoutElements = [NSMutableArray arrayWithCapacity:sublayoutCount];
for (ASLayout *sublayout in _sublayouts) {
[_sublayoutLayoutElements addObject:sublayout.layoutElement];
}
}
_sublayoutLayoutElements = ASMutableArrayByFlatMapping(_sublayouts, ASLayout *sublayout, ({
sublayout.layoutElement;
}));
}
}
}
Expand All @@ -236,7 +232,7 @@ - (ASLayout *)filteredNodeLayoutTree
queue.push_back({sublayout, sublayout.position});
}

NSMutableArray *flattenedSublayouts = [NSMutableArray array];
std::vector<ASLayout *> flattenedSublayouts;

while (!queue.empty()) {
const Context context = queue.front();
Expand All @@ -255,7 +251,7 @@ - (ASLayout *)filteredNodeLayoutTree
position:absolutePosition
sublayouts:@[]];
}
[flattenedSublayouts addObject:layout];
flattenedSublayouts.push_back(layout);
} else if (sublayoutsCount > 0){
std::vector<Context> sublayoutContexts;
for (ASLayout *sublayout in sublayouts) {
Expand All @@ -265,7 +261,8 @@ - (ASLayout *)filteredNodeLayoutTree
}
}

ASLayout *layout = [ASLayout layoutWithLayoutElement:_layoutElement size:_size sublayouts:flattenedSublayouts];
NSArray *sublayoutsArray = [NSArray arrayWithObjects:flattenedSublayouts.data() count:flattenedSublayouts.size()];
ASLayout *layout = [ASLayout layoutWithLayoutElement:_layoutElement size:_size sublayouts:sublayoutsArray];
Copy link
Member Author

Choose a reason for hiding this comment

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

IIRC this method is one of our hottest methods, and we copy the resulting array so I'm hopeful this will be a nice bump.

// All flattened layouts must have this flag enabled
// to ensure sublayout elements are retained until the layouts are applied.
layout.retainSublayoutLayoutElements = YES;
Expand Down
10 changes: 3 additions & 7 deletions Source/Layout/ASLayoutSpec.mm
Original file line number Diff line number Diff line change
Expand Up @@ -295,19 +295,15 @@ - (instancetype)initWithLayoutElements:(NSArray<id<ASLayoutElement>> *)layoutEle

- (ASLayout *)calculateLayoutThatFits:(ASSizeRange)constrainedSize
{
NSArray *children = self.children;
NSMutableArray *sublayouts = [NSMutableArray arrayWithCapacity:children.count];

CGSize size = constrainedSize.min;
for (id<ASLayoutElement> child in children) {
NSArray *sublayouts = ASArrayByFlatMapping(self.children, id<ASLayoutElement> child, ({
ASLayout *sublayout = [child layoutThatFits:constrainedSize parentSize:constrainedSize.max];
sublayout.position = CGPointZero;

size.width = MAX(size.width, sublayout.size.width);
size.height = MAX(size.height, sublayout.size.height);

[sublayouts addObject:sublayout];
}
sublayout;
}));

return [ASLayout layoutWithLayoutElement:self size:size sublayouts:sublayouts];
}
Expand Down
9 changes: 3 additions & 6 deletions Source/Private/ASTwoDimensionalArrayUtils.m
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,10 @@

NSMutableArray<NSMutableArray *> *ASTwoDimensionalArrayDeepMutableCopy(NSArray<NSArray *> *array)
{
NSMutableArray *newArray = [NSMutableArray arrayWithCapacity:array.count];
NSInteger i = 0;
for (NSArray *subarray in array) {
return ASMutableArrayByFlatMapping(array, NSArray *subarray, ({
ASDisplayNodeCAssert([subarray isKindOfClass:[NSArray class]], @"This function expects NSArray<NSArray *> *");
newArray[i++] = [subarray mutableCopy];
}
return newArray;
[subarray mutableCopy];
}));
}

void ASDeleteElementsInTwoDimensionalArrayAtIndexPaths(NSMutableArray *mutableArray, NSArray<NSIndexPath *> *indexPaths)
Expand Down
8 changes: 3 additions & 5 deletions Source/_ASTransitionContext.m
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,9 @@ - (CGRect)finalFrameForNode:(ASDisplayNode *)node

- (NSArray<ASDisplayNode *> *)subnodesForKey:(NSString *)key
{
NSMutableArray<ASDisplayNode *> *subnodes = [NSMutableArray array];
for (ASLayout *sublayout in [self layoutForKey:key].sublayouts) {
[subnodes addObject:(ASDisplayNode *)sublayout.layoutElement];
}
return subnodes;
return ASArrayByFlatMapping([self layoutForKey:key].sublayouts, ASLayout *sublayout, ({
(ASDisplayNode *)sublayout.layoutElement;
}));
}

- (NSArray<ASDisplayNode *> *)insertedSubnodes
Expand Down