Create a centralized configuration API #747

Adlai-Holler · 2018-01-15T20:50:04Z

Requirements:

We need to pull configuration from user, instead of hoping they push it to us at arbitrarily-early times.
We need a standard interface for configuring framework behavior to make it easier to find.
It would be nice to have an API for users to learn when the behavior is applied for the first time (useful for A/B metrics).
It needs to be crazy fast since even the hottest paths may need to use it.
It needs to have room to grow.

The premise:

ASConfiguration is a plain old object.
We pull one from the user and copy it. + (ASConfiguration *)textureConfiguration
- We declare this class method but ask the user to implement it in a category. This is a little strange but it gives us a compile-time location to draw the configuration from at our convenience.
delegate An optional delegate that we call out to about configuration.
Flags ASExperimentalFeature. Framework code checks/activates using ASActivateExperimentalFeature(myFeature). Later could have ASCheckExperimentalFeature for no-activate cases.
Delegate is called out to on a private serial queue.
Private singleton class ASConfigurationManager handles all the grunt work.
Tested!

The first two features in this config are the ASExperimentalGraphicsContexts from #741 and ASExperimentalTextNode.

Note: This is breaking API for users of the current experiments. Since these are experimental APIs anyway, and the migration path is simple, it's not worth providing backwards compatibility. Namely:

ASTEXTNODE_EXPERIMENT_GLOBAL_ENABLE macro is no longer needed. The new ASConfiguration approach is efficient enough to be used at runtime.
ASEnableNoCopyRendering() is simply removed since it's so recent.
[ASTextNode setExperimentOptions:] is removed.

…om clients safely

appleguy

@Adlai-Holler overall this is a really fantastic API to have at our disposal, and I think it will not only see significant use, but also will enable things that we will get value from that otherwise wouldn't have been done!

I think there may be some improvements we can make, but in lieu of detailed feedback I think it would be great to land this now and refine with new use cases. I have a number of things on my branch that I could port to using this right away, and submit PRs with integrations or refinements to this.

What do you think? If you'd prefer detailed feedback before landing, I'll try to get back to this later in the week.

Adlai-Holler · 2018-02-26T23:31:33Z

@appleguy Yep I agree, let's get the ball rolling on this and enjoy the new features. The implementation is reliable and we can tweak the interface later as needed.

appleguy · 2018-03-18T04:54:33Z

Source/ASExperimentalFeatures.h

+ * A bit mask of features.
+ */
+typedef NS_OPTIONS(NSUInteger, ASExperimentalFeatures) {
+  ASExperimentalGraphicsContexts = 1 << 0,  // exp_graphics_contexts


Very exciting, can't wait to see this list grow :).

I wonder how we should handle configuration for other values, like floating points, for e.g. range tuning parameter default values?

Or even a pattern of a configuration payload that is passed into certain components, like ASCollectionNode, where there is a default and it can be overridden on a per-instance basis (optionally) -- that's the case where range tuning would be very useful.

For more complex values, I don't think it'll be hard. We just update the configuration schema to add more fields. Range tuning param sets would be probably be in their very own schema. Thoughts?

appleguy · 2018-03-18T04:56:47Z

Source/Base/ASAvailability.h

-// See ASTextNode+Beta.h declaration of ASTextNodeExperimentOptions for more details.
-#ifndef ASTEXTNODE_EXPERIMENT_GLOBAL_ENABLE
-  #define ASTEXTNODE_EXPERIMENT_GLOBAL_ENABLE 0
+#ifdef ASTEXTNODE_EXPERIMENT_GLOBAL_ENABLE


Any thoughts on how we can preserve the capability to turn on ASConfiguration options via compile flag? This seems pretty valuable / important, insofar as a JSON config may not be easily plumbed to all environments, like unit tests etc.

I'd say let's land this diff and iterate, although having a workaround for a compiler-driven flag is important for my use case in the short term (let's just say the reasons why are complex...). I do think it would make for an even better API if it were possible for us to flip on e.g. a static ASConfiguration bitfield that is defined at compile time to include the appropriate bit, if this #define is set.

Good point, I got you covered. I've updated the diff to support this. You can define AS_FIXED_CONFIG_JSON as a C-string, and we will implement the +textureConfiguration method by parsing that string. Does that solve your use case?

appleguy · 2018-03-18T04:59:40Z

examples/Kittens/Sample/ViewController.m

@@ -96,12 +94,6 @@ - (NSMutableArray *)createLitterWithSize:(NSInteger)litterSize
  return kittens;
 }

- (void)setKittenDataSource:(NSMutableArray *)kittenDataSource {
-  ASDisplayNodeAssert(!self.dataSourceLocked, @"Could not update data source when it is locked !");


Haha, this is so old! Thanks for fixing. Long story about the locked data source mode of ASTableView...truly off-main edit operations :)

appleguy

Let's roll! This is awesome. Several comments, the #define one is what I'd care most about if you're able to fix that before landing.

It would be a huge additional bonus to think about how we might handle floating point / non-boolean flags, too.

Can we offer a key-value API? So that for some use cases like tuning parameters, an entire dictionary can be included in the config JSON and just exposed as raw NSDictionary + NSNumbers?

# Conflicts: # AsyncDisplayKit.xcodeproj/project.pbxproj

…uration

Adlai-Holler · 2018-03-24T20:59:32Z

Note, updated the diff to bring interface state coalescing into the experiments system.

The experiment is on by default only because non-trivial interface state tests currently fail if the coalescing is disabled. This is a very serious situation #853 and we should probably revert the coalescing diff until those issues are ironed out.

cc @appleguy

ghost · 2018-03-25T19:18:12Z

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

Generated by 🚫 Danger

* Update the dangerfile * Make a trivial change to test new dangerfile * Try out the new value with another trivial change * Add a configuration API to make a unified place for pulling config from clients safely * Specify properties for delegate * Finish removing text experiment global enable * Generate the config file * Clean up configuration to fix tests * Work on making it serializable * Finish it up * Fix example code * Update sample project * Clean up a few things * Align with new project order * Make it faster and update license header * Add an option to specify your config at compile time * Update another license header * Add a version field, and bring interface state coalescing into configuration * Update CA queue code * Update CATransactionQueue tests * Turn transaction queue on by default (for now, see comment) * Update the tests * Update the tests AGAIN * Remove unused ordered set

Adlai Holler added 6 commits January 15, 2018 11:42

Update the dangerfile

2c7e050

Make a trivial change to test new dangerfile

1de26b4

Try out the new value with another trivial change

13fccb0

Add a configuration API to make a unified place for pulling config fr…

c4b4d09

…om clients safely

Specify properties for delegate

6968445

Finish removing text experiment global enable

348e5c2

Adlai-Holler requested review from maicki, garrettmoon, appleguy and nguyenhuy January 15, 2018 20:50

Adlai Holler added 2 commits January 15, 2018 13:23

Generate the config file

f7bd45a

Clean up configuration to fix tests

2233852

TextureGroup deleted a comment Jan 15, 2018

Work on making it serializable

1044620

Adlai-Holler changed the base branch from AHDangerfile to master January 15, 2018 23:14

Adlai-Holler and others added 3 commits January 15, 2018 15:14

Merge branch 'master' into AHExperiments

d02b36f

Finish it up

75922a3

Fix example code

9801560

TextureGroup deleted a comment Jan 16, 2018

Update sample project

99b3a70

TextureGroup deleted a comment Jan 16, 2018

Merge branch 'master' into AHExperiments

241eec9

Adlai-Holler mentioned this pull request Jan 30, 2018

Allow creating a ASPINRemoteImageDownloader that ignores caches #12

Closed

appleguy approved these changes Feb 26, 2018

View reviewed changes

Merge branch 'master' into AHExperiments

e4f6aac

Clean up a few things

042d338

appleguy reviewed Mar 18, 2018

View reviewed changes

appleguy approved these changes Mar 18, 2018

View reviewed changes

Adlai Holler added 2 commits March 24, 2018 10:58

Merge branch 'master' into AHExperiments

50383b5

# Conflicts: # AsyncDisplayKit.xcodeproj/project.pbxproj

Align with new project order

a548185

TextureGroup deleted a comment Mar 24, 2018

Adlai Holler added 4 commits March 24, 2018 11:40

Make it faster and update license header

9ab0562

Add an option to specify your config at compile time

0edeb2f

Update another license header

9938217

Add a version field, and bring interface state coalescing into config…

4363c6c

…uration

Adlai Holler added 4 commits March 25, 2018 10:10

Update CA queue code

1d4ba50

Update CATransactionQueue tests

5c5e93d

Turn transaction queue on by default (for now, see comment)

8cb7b8e

Update the tests

681bebd

TextureGroup deleted a comment Mar 25, 2018

Update the tests AGAIN

16fab3f

TextureGroup deleted a comment Mar 25, 2018

Adlai-Holler and others added 2 commits March 25, 2018 10:47

Merge branch 'master' into AHExperiments

331d70d

Remove unused ordered set

73efad5

TextureGroup deleted a comment Mar 25, 2018

Adlai-Holler merged commit 27fac9f into master Mar 25, 2018

Adlai-Holler deleted the AHExperiments branch March 25, 2018 19:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a centralized configuration API #747

Create a centralized configuration API #747

Adlai-Holler commented Jan 15, 2018 •

edited

Loading

appleguy left a comment

Adlai-Holler commented Feb 26, 2018 •

edited

Loading

appleguy Mar 18, 2018

Adlai-Holler Mar 24, 2018

appleguy Mar 18, 2018 •

edited

Loading

Adlai-Holler Mar 24, 2018

appleguy Mar 18, 2018

appleguy left a comment

Adlai-Holler commented Mar 24, 2018 •

edited

Loading

ghost commented Mar 25, 2018

Create a centralized configuration API #747

Create a centralized configuration API #747

Conversation

Adlai-Holler commented Jan 15, 2018 • edited Loading

appleguy left a comment

Choose a reason for hiding this comment

Adlai-Holler commented Feb 26, 2018 • edited Loading

appleguy Mar 18, 2018

Choose a reason for hiding this comment

Adlai-Holler Mar 24, 2018

Choose a reason for hiding this comment

appleguy Mar 18, 2018 • edited Loading

Choose a reason for hiding this comment

Adlai-Holler Mar 24, 2018

Choose a reason for hiding this comment

appleguy Mar 18, 2018

Choose a reason for hiding this comment

appleguy left a comment

Choose a reason for hiding this comment

Adlai-Holler commented Mar 24, 2018 • edited Loading

ghost commented Mar 25, 2018

Adlai-Holler commented Jan 15, 2018 •

edited

Loading

Adlai-Holler commented Feb 26, 2018 •

edited

Loading

appleguy Mar 18, 2018 •

edited

Loading

Adlai-Holler commented Mar 24, 2018 •

edited

Loading