Adds support for having multiple interface state delegates. #979

garrettmoon · 2018-06-21T00:04:39Z

Hopefully in a performant way.

garrettmoon · 2018-06-21T00:05:10Z

Source/ASDisplayNode+InterfaceState.h

+ * The defualt state, ASInterfaceStateNone, means that the element is not predicted to be onscreen soon and
+ * preloading should not be performed. Swift: use [] for the default behavior.
+ */
+typedef NS_OPTIONS(NSUInteger, ASInterfaceState)


This is just moved, no changes.

garrettmoon · 2018-06-21T00:05:23Z

Source/ASDisplayNode+InterfaceState.h

+    ASInterfaceStateInHierarchy   = ASInterfaceStateMeasureLayout | ASInterfaceStatePreload | ASInterfaceStateDisplay | ASInterfaceStateVisible,
+};
+
+@protocol ASInterfaceStateDelegate <NSObject>


Only change here is that these are now all optional.

Also added hierarchyDisplayDidFinish

ghost · 2018-06-21T00:06:49Z

🚫 CI failed with log

Adlai-Holler · 2018-06-21T00:26:40Z

Source/ASDisplayNode+InterfaceState.m

+        _interfaceDidEnterPreloadDelegates = [NSHashTable weakObjectsHashTable];
+        _interfaceDidExitPreloadDelegates = [NSHashTable weakObjectsHashTable];
+        _interfaceNodeDidLayoutDelegates = [NSHashTable weakObjectsHashTable];
+        _interfaceNodeDidLoadDelegates = [NSHashTable weakObjectsHashTable];


Rather than separate tables, I think it's fine to use class_respondsToSelector before each call. That call actually runs through the same message-send machinery as a normal call, plus it warms the impcache. It's plenty fast.

Let's use -[NSHashTable initWithOptions:NSHashTableWeakMemory | NSHashTableObjectPointerPersonality capacity:0] (0 means default.)

That'll let you remove this intermediary object and it won't be slower by any amount that matters.

On second thought, using -respondsToSelector: is only a tiny bit slower and allows for proxying, so that's probably the way to go. Still fast.

I don't think we should have to create all these up front...since we would only have to use the respondsToSelector: once per class (not even once per delegate), we should get a significant win by relying on just one object tracking the delegates, and we could use a simple bitfield / NS_OPTIONS to cache the ASInterfaceStateDelegateMethodsImplemented (similar to the flags in ASCollection that cache what the delegate / datasource implement, but just an NS_OPTIONS).

In recent versions of Objective-C, respondsToSelector: is truly very fast. It amortizes to 1 additional objc_msgSend, 3 static function invocations, and one inline-assembly routine. It also has the added benefit of checking-from and storing-to the ObjC impcache, so the subsequent message-send is optimized.

Writing our own bitfield-by-class would amount to us reimplementing that portion of the impcache, without the benefit of direct access to the class method table. And even accessing our table – say it were in a singleton registry object – would require retaining-releasing it unless we coded around that, which would easily subsume the entire rest of the lookup. The added boilerplate is also nontrivial.

So there is a win available here, but I believe it's small. Let's avoid these calls only if we can do it in a simple and tremendously fast & scalable way. If we can't, then we shouldn't worry about it – I believe we can absolutely achieve far superior optimizations by using resources for other parts of the framework (such as transfer-collections or more judicious applications of __unsafe_unretained).

Implementations:

-respondsToSelector:

class_respondsToSelector_inst

lookupImpOrForward and lookupImpOrNil

ARM64: cache_getImp, used by both objc_msgSend and lookupImpOrNil

nguyenhuy · 2018-06-21T00:19:38Z

Source/ASDisplayNode.mm

@@ -543,8 +543,8 @@ - (void)_didLoad
  for (ASDisplayNodeDidLoadBlock block in onDidLoadBlocks) {
    block(self);
  }
-
-  [_interfaceStateDelegate nodeDidLoad];
+


nit: No trailing white spaces.

nguyenhuy · 2018-06-21T00:31:59Z

Source/ASNodeController+Beta.m

@@ -47,6 +47,11 @@ - (instancetype)init
  return self;
 }

+- (void)dealloc
+{
+    [_weakNode removeInterfaceStateDelegate:self];


-removeInterfaceStateDelegate: needs to be called on main. ~~and objects are de-allocated off main by default (code here)~~ But since node controller is an associated object of display node, if the node is de-allocated off main, the node controller will be as well. And since NSHashTable is not thread-safe, we have to dispatch to main here.

@garrettmoon I still think that we should dispatch to main here, no?

maicki · 2018-06-21T01:00:31Z

Source/ASDisplayNode.mm

+  // Not a fan of lazy loading, but this method won't get called very often and avoiding
+  // the overhead of creating this is probably worth it.
+  if (_interfaceStateDelegateManager == nil) {
+    [[ASDisplayNodeInterfaceDelegateManager alloc] init];


Missing an assign to _interfaceStateDelegateManager?

maicki · 2018-06-21T01:03:02Z

Source/ASDisplayNode.h

@@ -292,6 +262,24 @@ AS_EXTERN NSInteger const ASDefaultDrawingPriority;
 */
 @property (readonly) ASInterfaceState interfaceState;

+/**
+ * @abstract Adds a delegate to recieve notifications on interfaceState changes.


nit: Typo: "recieve"

maicki · 2018-06-21T01:03:18Z

Source/ASDisplayNode.h

+- (void)addInterfaceStateDelegate:(id <ASInterfaceStateDelegate>)interfaceStateDelegate;
+
+/**
+ * @abstract Removes a delegate from recieving notifications on interfaceState changes.


nit: Typo: "recieving"

appleguy · 2018-06-21T01:53:00Z

This could be pretty useful! Could you share some background on the use case? I've found ASNodeController quite valuable in the cases I've come across, but maybe it doesn't work for the case you have.

I think we can design an ideal solution if we have the same / general use case in mind to know where we need to scale. It seems maybe more generalized than needed to track an arbitrary number of delegates.

garrettmoon · 2018-06-21T20:57:01Z

@appleguy We have two use cases, both related to tracking performance. It'd be ideal if there were separate managers for each of these which could monitor the states.

ghost · 2018-06-27T19:28:34Z

🚫 CI failed with log

nguyenhuy · 2018-06-27T21:10:59Z

Source/ASDisplayNode.mm

@@ -433,6 +433,9 @@ - (void)dealloc

  // TODO: Remove this? If supernode isn't already nil, this method isn't dealloc-safe anyway.
  [self _setSupernode:nil];
+
+  ASPerformMainThreadDeallocation(&_interfaceStateDelegates);


I don't think this is necessary. If the node is gone, _interfaceStateDelegates can be safely deallocated as well, especially because it only stores weak references.

Ah, sorry, I thought the comment about the dealloc was here, now I get what you mean, thank you :)

nguyenhuy · 2018-06-27T21:11:46Z

Source/ASNodeController+Beta.m

@@ -47,6 +47,11 @@ - (instancetype)init
  return self;
 }

+- (void)dealloc
+{
+    [_weakNode removeInterfaceStateDelegate:self];


@garrettmoon I still think that we should dispatch to main here, no?

maicki

Looks good to me.

…roup#979) * Adds support for having multiple interface state delegates. Hopefully in a performant way. * Switch to respondsToSelector for int del instead of separate object * Add CHANGELOG * Make ASDisplayNode+InterfaceState.h public * Huy's comments * Don't even bother removing since it's a weak hash table.

Adds support for having multiple interface state delegates.

e8b8fd6

Hopefully in a performant way.

garrettmoon commented Jun 21, 2018

View reviewed changes

Adlai-Holler reviewed Jun 21, 2018

View reviewed changes

nguyenhuy requested changes Jun 21, 2018

View reviewed changes

maicki reviewed Jun 21, 2018

View reviewed changes

garrettmoon added 2 commits June 27, 2018 12:24

Switch to respondsToSelector for int del instead of separate object

8c5981e

Add CHANGELOG

205f6d4

Make ASDisplayNode+InterfaceState.h public

0dcfaec

nguyenhuy requested changes Jun 27, 2018

View reviewed changes

garrettmoon added 2 commits June 27, 2018 14:50

Huy's comments

2a1c063

Don't even bother removing since it's a weak hash table.

700d927

nguyenhuy approved these changes Jun 27, 2018

View reviewed changes

TextureGroup deleted a comment Jun 27, 2018

garrettmoon merged commit a4f78ad into master Jun 27, 2018

maicki reviewed Jun 28, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for having multiple interface state delegates. #979

Adds support for having multiple interface state delegates. #979

garrettmoon commented Jun 21, 2018

garrettmoon Jun 21, 2018

garrettmoon Jun 21, 2018

garrettmoon Jun 27, 2018

ghost commented Jun 21, 2018

Adlai-Holler Jun 21, 2018

Adlai-Holler Jun 21, 2018

appleguy Jun 21, 2018

Adlai-Holler Jun 21, 2018 •

edited

Loading

nguyenhuy Jun 21, 2018

nguyenhuy Jun 21, 2018 •

edited

Loading

nguyenhuy Jun 27, 2018

maicki Jun 21, 2018

maicki Jun 21, 2018

maicki Jun 21, 2018 •

edited

Loading

appleguy commented Jun 21, 2018

garrettmoon commented Jun 21, 2018

ghost commented Jun 27, 2018

nguyenhuy Jun 27, 2018

garrettmoon Jun 27, 2018

nguyenhuy Jun 27, 2018

maicki left a comment •

edited

Loading

Adds support for having multiple interface state delegates. #979

Adds support for having multiple interface state delegates. #979

Conversation

garrettmoon commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adlai-Holler Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenhuy Jun 21, 2018 • 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

maicki Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

appleguy commented Jun 21, 2018

garrettmoon commented Jun 21, 2018

ghost commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maicki left a comment • edited Loading

Choose a reason for hiding this comment

Adlai-Holler Jun 21, 2018 •

edited

Loading

nguyenhuy Jun 21, 2018 •

edited

Loading

maicki Jun 21, 2018 •

edited

Loading

maicki left a comment •

edited

Loading