Change how we cache sizing across sizeThatFits and autolayout #277

kyleve · 2021-11-05T02:09:47Z

@n8chur recently added support for caching intrinsic content size, which made me wonder: Why don't we do this everywhere? This will provide benefits for any existing impl which calls sizeThatFits or relies on the other autolayout methods, since we now only do an expensive re-measure when content has actually changed.

Changes

sizeThatFits now also caches results.
intrinsicContentSize is now backed by sizeThatFits.
Added systemLayoutSizeFitting... overrides to call through to sizeThatFits.
We invalidate sizing when environment, safe area, etc, change.

One more thing!

The old impl of intrinsicContentSize would use an unconstrained width if we had a new element, but had not yet laid out. This seems really wrong to me, since it was very inconsistent and hard to predict, to the point of non-determinism: Had you laid out yet? Great! Your constraint is the view width! Had you not? Uhh, you have no width constraint. This is a behaviour change, and there was value to the old behaviour, so it might make sense to add an optional/mode for this. Thoughts?

kyleve · 2021-11-05T02:14:09Z

BlueprintUI/Sources/BlueprintView/BlueprintView.swift

@@ -226,18 +294,20 @@ public final class BlueprintView: UIView {
        setNeedsViewHierarchyUpdate()
    }

-    private func performUpdate() {


This was called in exactly one place and only did this, so I removed it.

kylebshr

Does this automatically solve our perf issues with preferredContentSize?

BlueprintUI/Sources/BlueprintView/BlueprintView.swift

kyleve · 2021-11-08T16:57:20Z

Does this automatically solve our perf issues with preferredContentSize?

Ish. Kinda. But not enough. In my testing, it seems to help with about 50% of the excess perf, since we now cache some calls to sizeThatFits, but because in many workflows, elements are re-rendered pretty often, the cache is still evicted too frequently. Even in the best case, this still only will get us down to 1 extra measurement, which can be 200-500ms+ for complex screens.

n8chur · 2021-11-08T19:16:50Z

BlueprintUI/Sources/BlueprintView/BlueprintView.swift

+            if bounds.width == 0 {
+                return .unconstrained
+            } else {
+                return .init(width: bounds.width)
+            }


Should we also be doing this for height?

We'd have to switch on the content hugging priority I think; since without a passed in constraint or layout priority, we'd have to base it on those.

n8chur · 2021-11-08T19:24:14Z

One more thing!

The old impl of intrinsicContentSize would use an unconstrained width if we had a new element, but had not yet laid out. This seems really wrong to me, since it was very inconsistent and hard to predict, to the point of non-determinism: Had you laid out yet? Great! Your constraint is the view width! Had you not? Uhh, you have no width constraint. This is a behaviour change, and there was value to the old behaviour, so it might make sense to add an optional/mode for this. Thoughts?

I'm not sure I follow—what is the functionality you'd be missing out on by omitting this mode? Couldn't you still get this same behavior by attempting a layout with a 0-width frame with the changes you have in this PR? I'm not sure I understand the value of the current state over what's here.

BlueprintUI/Sources/BlueprintView/BlueprintView.swift

kyleve · 2021-11-08T20:46:45Z

I'm not sure I follow—what is the functionality you'd be missing out on by omitting this mode?

I think (unintentionally or not), this previously allowed Swift previews to always display the natural size of an embedded blueprint view. @bencochran added the existing behaviour and could speak more to it

…anges * origin/main: Fix size assertion grammar. Prep 0.32.0 Update BlueprintUI/Sources/Layout/Builder.swift Add support for for loop and available checks in result builders. Update changelog Break a retain cycle in posts demo Break retain cycle in FocusState storage chore(ios): update changelog chore(ios): update tests fix(ios): adjust keyboard correctly Improve assertion messages for unconstrained values. Fix test warnings by adding .self references Go with 0.31.0 instead Prep 0.30.1 Comment in new methods on result builders Update CHANGELOG Improve error messages for result builders with optionals

kyleve · 2021-11-18T02:04:29Z

This should be good for a re-review.

watt

I'm a little concerned that the interactions between various functions, caches, and cache invalidations are intertwined and hard to reason about. Perhaps it was just as bad before and I just got used to it.

watt · 2021-11-18T02:38:38Z

BlueprintUI/Sources/Environment/Environment.swift

@@ -70,6 +70,11 @@ public struct Environment {
        }
    }

+    /// If the `Environment` contains any values.
+    public var isEmpty: Bool {


I don't think this should be public. An Environment is never really "empty", because every key has a default. The presence of overrides is intentionally opaque.

Hmm, true; though in that case there's no values stored; so in the cases where this is used the default values would always be the same anyway. I guess this probably isn't useful outside of Blueprint though so I can make it internal.

watt · 2021-11-18T02:40:20Z

BlueprintUI/Sources/BlueprintView/BlueprintView.swift

    private func setNeedsViewHierarchyUpdate() {
-        guard !needsViewHierarchyUpdate else { return }
+
+        invalidateIntrinsicContentSize()
+        sizesThatFit.removeAll()
+
+        if needsViewHierarchyUpdate { return }
+
        needsViewHierarchyUpdate = true

-        /// We currently rely on CA's layout pass to actually perform a hierarchy update.
+        /// We use `UIView`'s layout pass to actually perform a hierarchy update.
+        /// If a manual update is required, call `layoutIfNeeded()`.
        setNeedsLayout()
    }


This seems like an unexpected set of responsibilities to me. Why would setNeedsViewHierarchyUpdate have a cache invalidation side effect, rather than just setting a flag? It could get called a lot before the render pass actually runs. Some docs, maybe?

I had them separated before, but Kyle rightly pointed out that we called the two methods in all the same places – so not unifying to one method was less error-prone for future callsites. Maybe worth renaming this method instead? Or something...

watt · 2021-11-18T02:45:43Z

BlueprintUI/Sources/BlueprintView/BlueprintView.swift

-    public override func invalidateIntrinsicContentSize() {
-        cachedIntrinsicContentSize = nil
-        super.invalidateIntrinsicContentSize()
+        return sizeThatFits(constraint.maximum)


I don't like the idea of intentionally round-tripping SizeConstraint.unconstrained through .greatestFiniteMagnitude back to SizeConstraint.unconstrained. Can you extract out the part of measurement that takes in a constraint?

Sure; will do; though note that sizeThatFits already handles this case because people call sizeThatFits with tons of weird "unconstrained" values.

watt · 2021-11-18T02:54:29Z

BlueprintUI/Sources/BlueprintView/BlueprintView.swift

@@ -36,7 +36,7 @@ public final class BlueprintView: UIView {

    private let rootController: NativeViewController

-    private var cachedIntrinsicContentSize: CGSize? = nil
+    private var sizesThatFit: [SizeConstraint: CGSize] = [:]


Can you use the CacheTree for this? Caching multiple measurements/layouts on the same elements was one of the follow-up goals from #209, and this seems like it does exactly that!

We could; but those are currently all short-lived because they depend on the exact element state at call time (eg, the subtrees are stored by element index); so it seemed to make more sense to introduce a top-level cache that holds "inert" measurement values. Eg, we'd still need to invalidate the CacheTree in all the same places we do here, and keeping one around seems to potentially invite mis-use when all we really care about here is the top-level size; not digging into child sizes.

we'd still need to invalidate the CacheTree in all the same places we do here

Yep, that's exactly what I had in mind. The CacheTree has an advantage in that a top-level miss can still have nested cache hits.

This is fine as-is though; I'll explore converting to CacheTree later.

BlueprintUI/Sources/BlueprintView/BlueprintView.swift

watt · 2021-11-19T02:11:28Z

BlueprintUI/Sources/BlueprintView/BlueprintView.swift

+
+    /// Measures the size needed to display the view within the given `SizeConstraint`.
+    /// by measuring the current `element` of the `BlueprintView`.
+    public func sizeThatFits(_ constraint: SizeConstraint) -> CGSize {


It felt reasonable to expose publicly; since the SizeConstraint through its types (eg, Axis.unconstrained) is clearer about how to use the provided SizeConstraint vs the regular CGSize magic numbers for unconstrained measurements.

Yeah, I like this for the same reason as the original suggestion to make it explicit. That is, for consumers who know they have a blueprint view, they can do unconstrained measurement without jumping through greatest finite magnitude weirdness.

kyleve requested review from bencochran, watt, n8chur, tyten and kylebshr November 5, 2021 02:09

kyleve force-pushed the kve/sizing-caching-changes branch from 6d085fb to 9e22cfd Compare November 5, 2021 02:11

kyleve commented Nov 5, 2021

View reviewed changes

Change how we cache sizing across sizeThatFits and autolayout.

af9e981

kyleve force-pushed the kve/sizing-caching-changes branch from 9e22cfd to af9e981 Compare November 5, 2021 02:20

kylebshr approved these changes Nov 7, 2021

View reviewed changes

BlueprintUI/Sources/BlueprintView/BlueprintView.swift Outdated Show resolved Hide resolved

BlueprintUI/Sources/BlueprintView/BlueprintView.swift Outdated Show resolved Hide resolved

n8chur reviewed Nov 8, 2021

View reviewed changes

BlueprintUI/Sources/BlueprintView/BlueprintView.swift Show resolved Hide resolved

kyleve added 3 commits November 17, 2021 17:42

Code review & updates

d7d1e5d

CHANGELOG and docs

d23199a

watt reviewed Nov 18, 2021

View reviewed changes

Code review from Andrew

4564648

watt approved these changes Nov 19, 2021

View reviewed changes

Merge branch 'main' into kve/sizing-caching-changes

9d0cdd5

kyleve merged commit 738aecd into main Nov 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change how we cache sizing across sizeThatFits and autolayout #277

Change how we cache sizing across sizeThatFits and autolayout #277

kyleve commented Nov 5, 2021

kyleve Nov 5, 2021

kylebshr left a comment

kyleve commented Nov 8, 2021

n8chur Nov 8, 2021

kyleve Nov 8, 2021

n8chur commented Nov 8, 2021

kyleve commented Nov 8, 2021

kyleve commented Nov 18, 2021

watt left a comment

watt Nov 18, 2021

kyleve Nov 18, 2021

kyleve Nov 18, 2021

watt Nov 18, 2021

kyleve Nov 18, 2021 •

edited

Loading

watt Nov 18, 2021

kyleve Nov 18, 2021

watt Nov 18, 2021

kyleve Nov 18, 2021

watt Nov 19, 2021

watt Nov 19, 2021

kyleve Nov 19, 2021

bencochran Nov 19, 2021

Change how we cache sizing across sizeThatFits and autolayout #277

Change how we cache sizing across sizeThatFits and autolayout #277

Conversation

kyleve commented Nov 5, 2021

Changes

One more thing!

Choose a reason for hiding this comment

kylebshr left a comment

Choose a reason for hiding this comment

kyleve commented Nov 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n8chur commented Nov 8, 2021

kyleve commented Nov 8, 2021

kyleve commented Nov 18, 2021

watt 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

kyleve Nov 18, 2021 • 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

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

kyleve Nov 18, 2021 •

edited

Loading