[Caching] Adds changes to Environment and EnvironmentKey needed for cross-render caching #382

kdaker · 2022-11-03T20:09:04Z

This PR adds updates to Environment and EnvironmentKey that needed for cross-render caching.

Summary of changes:

Makes Environment equatable and adds equivalency to EnvironmentKey
Adds default equivalency implementations to Environment and EnvironmentKeys
Adds ability to get a Subset of keys from Environment and compare an Environment with a Subset
We can now subscribe to Environment reads through subscribeToReads()

BlueprintUI/Sources/Environment/Environment.swift

BlueprintUI/Sources/Environment/EnvironmentKey.swift

BlueprintUI/Tests/EnvironmentKeyTests.swift

…r caching * Adds equatability to Environment and EnvironmentKey * Adds ability to subscribe to environment reads

n8chur · 2022-11-04T20:45:57Z

BlueprintUI/Sources/Environment/Environment.swift

+        }
+    }
+
+    struct StorageKey: Hashable {


Are we able to just use any EnvronmentKey.Type once we bump to Xcode 14.1?

I don't think so, since .Types aren't Hashable. We can likely heavily simplify this type though!

n8chur · 2022-11-04T21:07:00Z

BlueprintUI/Sources/Environment/Environment.swift

-    private var values: [ObjectIdentifier: Any] = [:]
+    /// When enabled, notify any subscribers when a value is read
+    /// from the environment
+    var readNotificationsEnabled: Bool = true


When would you want to disable this? Could we just rely on the presence of subscriptions instead?

We turn this off when adapting the environment so erroneous reads aren't fired; which is coming in a later PR: https://github.com/square/Blueprint/pull/367/files#diff-07c71c5dcdc5da4c2ddf51b560695e562af396d4afafe3908b35656a0907459aR568

Update: Since this a relatively small optimization; we just removed this in a later PR.

n8chur · 2022-11-04T21:14:53Z

BlueprintUI/Sources/Environment/EnvironmentKey.swift

+}
+
+extension EnvironmentKey {
+    static func areValuesEqual(_ lhs: Any?, _ rhs: Any?) -> Bool {


Nit/thought:

Suggested change

static func areValuesEqual(_ lhs: Any?, _ rhs: Any?) -> Bool {

@_disfavoredOverload

static func isEquivalent(_ lhs: Any?, _ rhs: Any?) -> Bool {

We think it's important to include the word Value in here, since the type signature is just two Anys, which could be construed to be the keys. Perhaps areValuesEquivalent?

n8chur · 2022-11-04T21:20:50Z

BlueprintUI/Sources/Environment/Environment.swift

+    enum LayoutPass: Equatable {
+        case measurement
+        case layout
+    }


We can make this change later if we decide it's worth it, but if we wanted to promote these changes to MarketEnvironment it would be nice if this was an extendible "scope" instead of being tied to measure and layout.

Had the same thought; it should be easy enough to do this when we extract the environment, but for now I think this is sufficient.

BlueprintUICommonControls/Sources/AttributedLabel+Environment.swift

n8chur · 2022-11-04T21:26:14Z

BlueprintUICommonControls/Sources/AttributedLabel+Environment.swift

@@ -41,4 +50,8 @@ struct ClosureURLHandler: URLHandler {
    func onTap(url: URL) {
        onTap(url)
    }
+
+    func isEquivalent(to other: URLHandler) -> Bool {


If I change the logic for the onTap between updates, will this handler still be re-applied to the backing view (even though it's considered equal)?

Yes it will get re-applied. The equality check is only used to cache the ElementState however the viewDescription will always be called regardless of the element's cache state.

n8chur · 2022-11-07T20:41:21Z

BlueprintUI/Sources/Environment/EnvironmentKey.swift

+
+    /// Equivalency check on the `Value`s of `EnvironmentKey`s. This should return false if the
+    /// difference in values affects the measurement or layout of consuming elements.
+    static func isEquivalent(_ lhs: Value, _ rhs: Value) -> Bool


Thought: Should we name these functions so that they describe that they're comparing only for layout/measurement purposes and not for strict content equability? isEquivalentForLayout(...)? I suppose not as I can't think of a good name...

I suppose this question might be more relevant in the next PR (for the ComparableElement function)

…-environment-equatability * origin/feature/caching: Add debugDescription tests ElementIdentifier test updates per code review Add caching for ElementIdentifier

BlueprintUI/Tests/EnvironmentTests.swift

n8chur

Approving merge to feature branch to help make iterating on further work easier, but I'd like to do a full pass at the feature branch before it lands!

n8chur · 2022-11-09T21:10:36Z

BlueprintUI/Sources/Environment/Environment.swift


    /// Gets or sets an environment value by its key.
-    public subscript<Key>(key: Key.Type) -> Key.Value where Key: EnvironmentKey {
+    public subscript<KeyType: EnvironmentKey>(key: KeyType.Type) -> KeyType.Value {


Nit: prefer Key over KeyType (Key.Type is the Type)

n8chur · 2022-11-09T21:38:39Z

BlueprintUI/Sources/Environment/Environment.swift

+    private var measurementDidRead: OnDidRead?
+    private var layoutDidRead: [OnDidRead] = []


Feeling pretty dumb not being able to keep the whole picture in my head, but I'll just ask the stupid questions anyway:

Why do we use an array of read observations for measurement and not layout? I naively expected the opposite because measurement can happen many times while layout should happen once per pass...

A very good question!

We differentiate these because measurements are "flat", eg we only care about the dependencies read during the direct element's measurement – because every layer of the tree that can caches its own measurements. However for layouts, we cache entire trees of layout – including our children – so we want to record and track the environment dependencies of our children as well, and re-perform the layouts if those values change as well.

Does that make sense? I feel like I should probably describe this both better in the code docs here, and in some explanatory tests

Does that make sense?

I think so!

I feel like I should probably describe this both better in the code docs here, and in some explanatory tests

1

If we used an array to describe the measurement subscriptions (just to simplify the implementation a bit within environment and perhaps prepare the shape better for supporting "scopes" rather than enum LayoutPass) would we need to manually clear them out for some reason, or would there just end up being only 1 subscription of that scope?

I think we can simplify this to a single array!

Thinking forward through this more, we might actually want to simplify measurement to be append as well. In theory, once we allow measurements and layouts to read the environment, downstream elements could pull a value from the environment that would affect their size, which when the parent measures its children, would affect its final size.

kdaker changed the base branch from main to feature/caching November 3, 2022 20:09

kdaker changed the title ~~Adds changes to Environment and EnvironmentKey needed for cross-render caching~~ [Caching] Adds changes to Environment and EnvironmentKey needed for cross-render caching Nov 3, 2022

kdaker changed the title ~~[Caching] Adds changes to Environment and EnvironmentKey needed for cross-render caching~~ [WIP][DNR][Caching] Adds changes to Environment and EnvironmentKey needed for cross-render caching Nov 3, 2022

kdaker force-pushed the kareem/add-environment-equatability branch from 741de67 to 71d1175 Compare November 3, 2022 22:56

kdaker changed the title ~~[WIP][DNR][Caching] Adds changes to Environment and EnvironmentKey needed for cross-render caching~~ [Caching] Adds changes to Environment and EnvironmentKey needed for cross-render caching Nov 3, 2022

kdaker force-pushed the kareem/add-environment-equatability branch 2 times, most recently from 35641f0 to 4c97689 Compare November 3, 2022 23:30

kdaker marked this pull request as ready for review November 3, 2022 23:32

kdaker requested a review from a team as a code owner November 3, 2022 23:32