Faster collection operations #748

Adlai-Holler · 2018-01-16T06:35:04Z

The idea here is, standardize on ASArrayByFlatMapping and make that macro faster and return an immutable array.

Immutable arrays mean (1) free copying, and (2) free zero-arrays and optimized singleObjectArrays, which means less retaining/releasing. Yay!

I haven't profiled this yet. Just want to put it out there.

Adlai-Holler · 2018-01-16T06:35:57Z

Source/Layout/ASLayout.mm

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


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.

Adlai-Holler · 2018-01-16T07:01:18Z

AsyncDisplayKit.xcodeproj/xcshareddata/xcschemes/AsyncDisplayKit.xcscheme

@@ -60,6 +60,7 @@
      buildConfiguration = "Debug"
      selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
      selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
+      disableMainThreadChecker = "YES"


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.

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)

Adlai-Holler · 2018-01-16T07:02:52Z

Source/Details/ASElementMap.h

+/**
+ * The total number of elements in this map.
+ */
+@property (readonly) NSUInteger count;


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

Adlai-Holler · 2018-01-16T08:34:10Z

Source/Base/ASBaseDefines.h

+    CFArrayCallBacks cb = kCFTypeArrayCallBacks; \
+    cb.retain = NULL; \
+    result = (__bridge NSArray *)CFArrayCreate(kCFAllocatorDefault, _cArray, _i, &cb); \
+  } \


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.

Adlai-Holler · 2018-01-16T21:08:29Z

Note that, as much as I love this diff, copy-on-write support was added to CoreFoundation for arrays, sets, and dictionaries. So copying a mutable array gives you an __NSFrozenArrayM, and copying that gives you the same instance back. The mechanics of the copy-on-write are unclear because it's written in Objective-C (and thus not open source) but it's definitely (attempted to be) running.

appleguy · 2018-01-18T05:33:34Z

This is extremely cool.

It could be enough to avoid 1 frame drop in several common scenarios, either faster devices that are near the threshold or dialing back the severity of an inevitable drop on a slower device.

There is an awful lot of runtime traffic in common codepaths that run on main — just thousands of very short method calls, which do contend with background threads, especially when any part of the system is using dynamically-implemented methods. This should reduce the surface area for contention (and thus risk for the longer kinds of sporadic stalls / priority inversion and rapid priority donation thrash), and also reduce total CPU time even when un-contended.

I'd like to study the code and profile this in more detail, but if you develop confidence in it, then don't wait on me to land.

maicki

That is pretty cool!

appleguy · 2018-01-21T05:34:06Z

Source/Base/ASBaseDefines.h

+    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) ({ \


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.

nguyenhuy · 2018-01-22T12:45:31Z

Merging per approvals from @maicki and @appleguy. Thanks all!

ghost · 2018-01-22T12:54:39Z

1 Warning

⚠️

Please ensure license is correct for ASCollectionGalleryLayoutDelegate.mm:

//
//  ASCollectionGalleryLayoutDelegate.mm
//  Texture
//
//  Copyright (c) 2014-present, Facebook, Inc.  All rights reserved.
//  This source code is licensed under the BSD-style license found in the
//  LICENSE file in the /ASDK-Licenses directory of this source tree. An additional
//  grant of patent rights can be found in the PATENTS file in the same directory.
//
//  Modifications to this file made after 4/13/2017 are: Copyright (c) through the present,
//  Pinterest, Inc.  Licensed under the Apache License, Version 2.0 (the "License");
//  you may not use this file except in compliance with the License.
//  You may obtain a copy of the License at
//
//      http://www.apache.org/licenses/LICENSE-2.0
//

Generated by 🚫 Danger

This reverts commit 5c13403.

* Faster collection operations * Fix a few things * Put the stupid semicolon * Address warning * Cut down retain/releases during collection operations * Update CHANGELOG.md

…p#759) This reverts commit 5c13403.

Adlai-Holler commented Jan 16, 2018

View reviewed changes

TextureGroup deleted a comment Jan 16, 2018

Adlai-Holler commented Jan 16, 2018

View reviewed changes

TextureGroup deleted a comment Jan 16, 2018

appleguy approved these changes Jan 18, 2018

View reviewed changes

maicki approved these changes Jan 19, 2018

View reviewed changes

appleguy reviewed Jan 21, 2018

View reviewed changes

appleguy approved these changes Jan 21, 2018

View reviewed changes

Adlai Holler and others added 6 commits January 22, 2018 12:53

Faster collection operations

68d9849

Fix a few things

a599b07

Put the stupid semicolon

f313fc4

Address warning

e5a4fdc

Cut down retain/releases during collection operations

3bf0279

Update CHANGELOG.md

5ea8c67

nguyenhuy force-pushed the AHFasterCollections branch from 4bdba96 to 5ea8c67 Compare January 22, 2018 12:53

nguyenhuy merged commit 5c13403 into master Jan 22, 2018

Adlai-Holler added a commit that referenced this pull request Jan 22, 2018

Revert "Faster collection operations (#748)"

66278c5

This reverts commit 5c13403.

Adlai-Holler mentioned this pull request Jan 22, 2018

Revert "Faster collection operations" #759

Merged

Adlai-Holler added a commit that referenced this pull request Jan 22, 2018

Revert "Faster collection operations (#748)" (#759)

9b8a919

This reverts commit 5c13403.

Adlai-Holler mentioned this pull request Jan 22, 2018

Faster collection operations v2 #760

Closed

bernieperez pushed a commit to AtomTickets/Texture that referenced this pull request Apr 25, 2018

Revert "Faster collection operations (TextureGroup#748)" (TextureGrou…

47ab6cc

…p#759) This reverts commit 5c13403.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Faster collection operations #748

Faster collection operations #748

Adlai-Holler commented Jan 16, 2018 •

edited

Loading

Adlai-Holler Jan 16, 2018

Adlai-Holler Jan 16, 2018

appleguy Jan 18, 2018

Adlai-Holler Jan 16, 2018

Adlai-Holler Jan 16, 2018 •

edited

Loading

Adlai-Holler commented Jan 16, 2018

appleguy commented Jan 18, 2018

maicki left a comment

appleguy Jan 21, 2018

nguyenhuy commented Jan 22, 2018

ghost commented Jan 22, 2018

Faster collection operations #748

Faster collection operations #748

Conversation

Adlai-Holler commented Jan 16, 2018 • edited Loading

Adlai-Holler Jan 16, 2018

Choose a reason for hiding this comment

Adlai-Holler Jan 16, 2018

Choose a reason for hiding this comment

appleguy Jan 18, 2018

Choose a reason for hiding this comment

Adlai-Holler Jan 16, 2018

Choose a reason for hiding this comment

Adlai-Holler Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Adlai-Holler commented Jan 16, 2018

appleguy commented Jan 18, 2018

maicki left a comment

Choose a reason for hiding this comment

appleguy Jan 21, 2018

Choose a reason for hiding this comment

nguyenhuy commented Jan 22, 2018

ghost commented Jan 22, 2018

Adlai-Holler commented Jan 16, 2018 •

edited

Loading

Adlai-Holler Jan 16, 2018 •

edited

Loading