New runloop queue to coalesce Interface state update calls. #788

wsdwsd0829 · 2018-02-05T01:21:33Z

Coalesce Interface state update calls.

This PR introduce a new run loop queue that will execute before CATransaction commit.
Each node will only be called once per transaction commit to reflect interface change.
This will help reducing calls to update to interfaceState like didEnterPreload, didEnterVisible during controller transitions like push and help apps that use these api to accurately log view visible events.

For example:
Before changes, ASCollectionView example will call didEnterVisibleState 47 times when “Push Another Copy” clicked, while only 27 times with new queue.

…nsaction commit. This will avoid duplicate interface state delegate calls caused by view repeatly added/removed to/from hierarchy during controller animation transition.

This reverts commit f807efa.

ghost · 2018-02-05T02:49:21Z

🚫 CI failed with log

appleguy · 2018-02-05T08:57:11Z

Thanks for posting this, Max! @maicki, @Adlai-Holler, @nguyenhuy — this is a significant enough change that it would be great to have multiple eyes on it.

We are running with this internally for the past few weeks. Still, it is possible it can break some unit tests (e.g. at the app layer) where they are not very realistic in simulating the UIKit environment, where a CATransaction commit does not occur. This can be most easily fixed by disabling interfaceState coalescing in the test setup method, or for more accurate tests, by ensuring a key & visible window is used and the runloop is allowed to turn before test expectations are verified.

appleguy · 2018-02-05T08:59:21Z

Source/ASDisplayNode.mm

@@ -2741,7 +2741,9 @@ - (void)setHierarchyState:(ASHierarchyState)newState
  // Entered or exited range managed state.
  if ((newState & ASHierarchyStateRangeManaged) != (oldState & ASHierarchyStateRangeManaged)) {
    if (newState & ASHierarchyStateRangeManaged) {
-      [self enterInterfaceState:self.supernode.interfaceState];
+      if (self.supernode) {
+        [self enterInterfaceState:self.supernode->_pendingInterfaceState];


@wsdwsd0829 In order to do this, we would need to grab the instanceLock of the supernode, which shouldn't be done manually.

To correctly fix this case, we'll need to add an accessor method for - (ASInterfaceState)pendingInterfaceState which uses a MutexLocker and returns the _pendingInterfaceState.

This method can be declared in ASDisplayNode+FrameworkPrivate.h, so it can be used by various internal subclasses without being made public right away (we may eventually want to make this public, but not until there is a use case — I'm optimistic that there will never be a need to do this).

appleguy · 2018-02-05T09:02:32Z

Source/ASDisplayNode.mm

+    };
+
+    if ([[ASCATransactionQueue sharedQueue] disabled]) {
+      dispatch_async(dispatch_get_main_queue(), exitVisibleInterfaceState);


For reviewers: Note that this change intentionally alters the behavior of the -disabled state as well. After the change, the -didExitVisibleState call is held for a dispatch_async to confirm the node is still invisible, for both range-managed and not range-managed nodes (previously there was a check distinguishing these).

This will decrease the accuracy of the call slightly, compared to before the change and also compared to interfaceState coalescing which guarantees the call is done on the same runloop / implicit transaction.

However it will also address a very common issue where visibility calls thrash to invisible and visible twice during the UIViewController transition process, and this feels like a better / safer behavior to prefer rather than the utmost accuracy of the invisibility call.

The visibility call remains un-delayed, and that one is much more important. Gating the invisibility is sufficient to avoid the thrashing since the visibility call won't re-fire if the interfaceState remains visible.

ghost · 2018-02-06T17:12:26Z

🚫 CI failed with log

Adlai-Holler

We've been waiting for a feature like this for a long time. I love that it's battle-tested and it brings clear benefits.

There is a lot of duplicated code in our runloop queue classes now. We should work harder to elevate common behavior into the superclass, but I won't block this diff for that.

Adlai-Holler · 2018-02-07T20:00:00Z

Source/ASDisplayNode.mm

+      _pendingInterfaceState = newState;
+      [[ASCATransactionQueue sharedQueue] enqueue:self];
+    }
+  }


Remove the lock at the top of this method. It isn't needed and applyPendingInterfaceState asserts that we aren't locked. (_unlocked_applyPendingInterfaceState:?)

Yes, this is a mis-merge, thanks for spotting that.

Adlai-Holler · 2018-02-07T20:01:56Z

Source/ASRunLoopQueue.mm

+
+#if ASRunLoopQueueLoggingEnabled
+  NSTimer *_runloopQueueLoggingTimer;
+#endif


Can we not move the duplicated ivars up into ASAbstractRunLoopQueue?

The comment on _internalQueue doesn't apply to this class; we always use it in strong mode.

Adlai-Holler · 2018-02-07T20:06:22Z

Source/ASRunLoopQueue.mm

+  if (CFRunLoopContainsSource(_runLoop, _runLoopSource, kCFRunLoopCommonModes)) {
+    CFRunLoopRemoveSource(_runLoop, _runLoopSource, kCFRunLoopCommonModes);
+  }
+  CFRelease(_runLoopSource);


We can unconditionally remove the source – documentation says that if the source isn't present the function does nothing.

Adlai-Holler · 2018-02-07T20:08:45Z

Source/ASRunLoopQueue.h

+ * to get last chance of updating/coalesce info like interface state.
+ * Each node will only be called once per transaction commit to reflect interface change.
+ */
+ (ASCATransactionQueue *)sharedQueue;


Declare this as an atomic readonly class property.

Adlai-Holler · 2018-02-07T20:10:07Z

Source/ASRunLoopQueue.h

+@interface ASCATransactionQueue : ASAbstractRunLoopQueue
+
+@property (nonatomic, readonly) BOOL isEmpty;
+@property (nonatomic, readonly) BOOL disabled;


These properties need to be atomic.

I guess ASRunloopQueue's isEmpty needs to be atomic also.

Adlai-Holler · 2018-02-07T20:27:48Z

Source/ASRunLoopQueue.mm

+
+// CoreAnimation commit order is 2000000, the goal of this is to process shortly beforehand
+// but after most other scheduled work on the runloop has processed.
+static int const kASASCATransactionQueueOrder = 1000000;


We can always update this later, since the current version of the diff brings obvious benefits and has been productionized, but in an ideal world would we not want to do this work at the end of the layout phase of the commit?

For example, collection views will not update their visible cells until the layout pass, so we'd have to wait for the next go-round for the interface state to be updated to reflect the truth. For a ridiculous case, imagine I have a cell node that is red whenever it's invisible and blue whenever it's visible. The current implementation would show red for 1 frame and then jump to blue.

One weird but viable option is for us to do something like this in the observer:

while (true) { // Layout visible windows (CA is about to do this anyway) for (window in app.windows) { if (!window.hidden) { [window layoutIfNeeded]; } } // If we have no work to do, break. if ([self isEmpty]) { break; } // Otherwise notify observers and loop again. [self processQueue]; }

That way any layout invalidations that happen due to interface states, and any interface state changes due to layout, are all settled. We would also use a loop counter to limit the maximum recursive dirtying (just like CA).

Is this suggest trying to solve the case when user trying to adjust layout in interfaceState change methods (e.g. didEnterVisible), so that we can do all work in one runloop instead of delaying layout work to next loop?
May be some comment in api will let the user to call layoutIfNeed on their changes? My concern is that if we are doing to much work here, it may cause frame drop and user cannot do much about it.

Adlai-Holler · 2018-02-07T20:29:15Z

Source/ASRunLoopQueue.mm

+    return;
+  }
+
+  if (_disableInterfaceStateCoalesce == YES) {


Nit: remove == YES, I know it's used in other places but we can stop the madness.

Adlai-Holler · 2018-02-07T20:30:54Z

Source/ASRunLoopQueue.h

+/**
+ * @abstract Apply a node's interfaceState immediately rather than adding to the queue.
+ */
+- (void)disableInterfaceStateCoalesce;


I would just call this method -disable, since this queue isn't supposed to know anything about interface states per-se.

appleguy · 2018-02-08T03:25:33Z

Thanks for the feedback @Adlai-Holler ! BTW @nguyenhuy, regarding the commit message in ea54727 -- something in the direction of this change could help.

However the trouble is that there are two use cases, one that may require layout to be complete, and the other which doesn't:

Normal use of the visibility call to create an expensive child and add it, catching the layout pass (before layout).
The impression logging case where it needs to look at layout information.

One thing I wonder is whether the impression logging case could be changed to read .calculatedLayout instead. If not, we may need two calls: willEnterVisibleState and didEnterVisibleState (but that would be a breaking semantic change), or didEnterVisibleState[...with layout complete].

…88/\#pullrequestreview-94849919

wsdwsd0829 · 2018-02-08T07:56:46Z

Thanks for reviewing. @Adlai-Holler
I changed most suggests. I am thinking to do a new pr on extracting common behavior to AbstractQueue because it may need to take a closer look. The two queues are different in that the ASRunloopQueue is call back/block based while ASCATransaction queue is delegate based.
Also a test fail in CI should be resolved here #790.

ghost · 2018-02-08T12:01:30Z

🚫 CI failed with log

nguyenhuy · 2018-02-08T19:47:28Z

@wsdwsd0829 Thanks for the diff. Very nice work!

I have to agree with @Adlai-Holler that it'd be much better to fire off visibility events after the layout phase of the CA transaction. I have first-hand experience dealing with Pinterest problems, and this is the best approach I can think of to fix them once and for all.

I think the most important agreement we need to make now is that our definition of "visible" should be consistent with CA's, that is "after the layout pass". And with that, didEnterVisibleState should be called afterward.

I do understand the use case for handling an expensive child upon visibility. It wouldn't be terrible to add another event type, say "pre visible" or "will be visible", and provide a new API that will be called before the layout pass for any changes that should be performed in response to such event.

Having said that, it seems to me that such API will add complexity without much benefits. If an expensive child needs to be prepared on main, it will cause frame drops regardless of whether the preparation happens before or after the layout phase. Perhaps, users should minimize the work on main and/or split it up across multiple interface states (preload and render).

Adlai-Holler

Scott persuaded me that since the current diff is production-tested we ought to land it and then make a separate experiment to improve the visibility notification on top of it.

Lets land this and enjoy the reduced thrash, then go from there. ✅

nguyenhuy

👌

…ed and before next runloop started.

ghost · 2018-02-09T12:44:30Z

🚫 CI failed with log

maicki

LGTM - Seems like CI has some problems running the tests from time to time, I restarted it and they should succeed in a bit and than ready to land.

ghost · 2018-02-09T16:59:39Z

🚫 CI failed with log

ghost · 2018-02-09T17:06:36Z

🚫 CI failed with log

wsdwsd0829 · 2018-02-10T00:19:35Z

Hi, I made some final changes by applying interface immediately for any interface updates during processing of queue. Please talk another look at and then we should ready to ship.
thanks.

ghost · 2018-02-10T17:33:22Z

🚫 CI failed with log

ghost · 2018-02-11T13:10:59Z

🚫 CI failed with log

cesteban · 2018-02-12T21:35:00Z

Source/ASRunLoopQueue.mm

+      // Inserting a NULL here ensures the compaction will take place.
+      // See http://www.openradar.me/15396578 and https://stackoverflow.com/a/40274426/1136669
+      [_internalQueue addPointer:NULL];
+    }


If the queue only holds strong references, this trick shouldn't be necessary.

@cesteban awesome catch, thanks for reviewing! @wsdwsd0829 this would be a good change to make, since we expect this queue to always be strong (otherwise the node could dealloc before getting the invisible state) - so I think we can just delete these few lines.

Thanks, fixed.

ghost · 2018-02-13T14:05:42Z

2 Warnings

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

⚠️

Please ensure license is correct for ASRunLoopQueueTests.m:

//
//  ASRunLoopQueueTests.m
//  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

Adlai-Holler · 2018-02-13T20:09:22Z

~~@wsdwsd0829 One more thing is just to update the license header. It should skip CI and we can land today.~~

Actually that danger message is in error. Landing!

…788)" This reverts commit 2618c50.

…roup#788) * fix SIMULATE_WEB_RESPONSE not imported TextureGroup#449 * Coalesce interface state updates to ASCATransactionQueue before CATransaction commit. This will avoid duplicate interface state delegate calls caused by view repeatly added/removed to/from hierarchy during controller animation transition. * fix tests for new run loop queue * Support for disabling ASCATransactionQueue * Fix didExitHierarchy to use ASCATransactionQueue. * merge range managed and none range managed for didExitHierarchy * Revert "merge range managed and none range managed for didExitHierarchy" This reverts commit f807efa. * merge range managed and none range managed for didExitHierarchy * remove metadata * abstract queue to impl class methods * Add tests * Fix test fail because of shared object. * guard _pendingInterfaceState access with lock * name refactor * Refactor from comments https://github.com/TextureGroup/Texture/pull/788/\#pullrequestreview-94849919 * Apply InterfaceState immediately after ASCATranactionQueue is processed and before next runloop started. * refactor * no op to start CI build * remove unused var and kick off tests * change lisence * remove code for weak ref * add change log and adjust license

wsdwsd0829 added 16 commits July 16, 2017 16:59

fix SIMULATE_WEB_RESPONSE not imported TextureGroup#449

929d118

Merge branch 'master' of github.com:TextureGroup/Texture

dd24d8f

Merge branch 'master' of github.com:TextureGroup/Texture

b8eaffa

Merge branch 'master' of github.com:TextureGroup/Texture

2918ea0

Merge branch 'master' of github.com:TextureGroup/Texture

9c42266

Merge branch 'master' of github.com:TextureGroup/Texture

3529cc7

Coalesce interface state updates to ASCATransactionQueue before CATra…

ee44725

…nsaction commit. This will avoid duplicate interface state delegate calls caused by view repeatly added/removed to/from hierarchy during controller animation transition.

fix tests for new run loop queue

cdd2e9b

Support for disabling ASCATransactionQueue

e99abf7

Fix didExitHierarchy to use ASCATransactionQueue.

6f2ba8e

merge range managed and none range managed for didExitHierarchy

f807efa

Revert "merge range managed and none range managed for didExitHierarchy"

bd8cf01

This reverts commit f807efa.

merge range managed and none range managed for didExitHierarchy

7306dc5

remove metadata

cfd083a

abstract queue to impl class methods

1ec63b6

Add tests

58effec

Fix test fail because of shared object.

cace535

appleguy reviewed Feb 5, 2018

View reviewed changes

guard _pendingInterfaceState access with lock

0631372

wsdwsd0829 mentioned this pull request Feb 5, 2018

Fix rangemode update #791

Closed

name refactor

f3c8507

Adlai-Holler reviewed Feb 7, 2018

View reviewed changes

Refactor from comments https://github.com/TextureGroup/Texture/pull/7…

e2c7d06

…88/\#pullrequestreview-94849919

Adlai-Holler approved these changes Feb 8, 2018

View reviewed changes

nguyenhuy approved these changes Feb 8, 2018

View reviewed changes

Apply InterfaceState immediately after ASCATranactionQueue is process…

7556ed7

…ed and before next runloop started.

maicki approved these changes Feb 9, 2018

View reviewed changes

wsdwsd0829 added 2 commits February 9, 2018 14:10

refactor

0904422

merge master from up stream

786ef37

no op to start CI build

1efd5e8

wsdwsd0829 added 2 commits February 11, 2018 10:09

remove unused var and kick off tests

632c69f

change lisence

9dad5a5

cesteban mentioned this pull request Feb 12, 2018

automaticallyAdjustsContentOffset does not work anymore #23

Open

cesteban reviewed Feb 12, 2018

View reviewed changes

wsdwsd0829 added 2 commits February 12, 2018 21:51

remove code for weak ref

a6a7e0a

add change log and adjust license

19bdef2

TextureGroup deleted a comment Feb 13, 2018

Adlai-Holler merged commit 2618c50 into TextureGroup:master Feb 13, 2018

garrettmoon added a commit that referenced this pull request Mar 14, 2018

Revert "New runloop queue to coalesce Interface state update calls. (#…

29076d1

…788)" This reverts commit 2618c50.

Adlai-Holler pushed a commit that referenced this pull request Mar 28, 2018

Revert "New runloop queue to coalesce Interface state update calls. (#…

80efb63

…788)" This reverts commit 2618c50.

Adlai-Holler mentioned this pull request Mar 28, 2018

Revert #788 – Interface State Runloop Queue #861

Closed

wsdwsd0829 mentioned this pull request Mar 28, 2018

Disable interface coalescing #862

Merged

New runloop queue to coalesce Interface state update calls. #788

New runloop queue to coalesce Interface state update calls. #788

Conversation

wsdwsd0829 commented Feb 5, 2018

ghost commented Feb 5, 2018

appleguy commented Feb 5, 2018

appleguy Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

appleguy Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

ghost commented Feb 6, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appleguy commented Feb 8, 2018

wsdwsd0829 commented Feb 8, 2018

ghost commented Feb 8, 2018

nguyenhuy commented Feb 8, 2018 • edited Loading

Adlai-Holler left a comment

Choose a reason for hiding this comment

nguyenhuy left a comment

Choose a reason for hiding this comment

ghost commented Feb 9, 2018

maicki left a comment • edited Loading

Choose a reason for hiding this comment

ghost commented Feb 9, 2018

ghost commented Feb 9, 2018

wsdwsd0829 commented Feb 10, 2018

ghost commented Feb 10, 2018

ghost commented Feb 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Feb 13, 2018

Adlai-Holler commented Feb 13, 2018 • edited Loading

appleguy Feb 5, 2018 •

edited

Loading

appleguy Feb 5, 2018 •

edited

Loading

nguyenhuy commented Feb 8, 2018 •

edited

Loading

maicki left a comment •

edited

Loading

Adlai-Holler commented Feb 13, 2018 •

edited

Loading