[ASTextNode2] Simplify Caching by Adlai-Holler · Pull Request #832 · TextureGroup/Texture

Adlai-Holler · 2018-03-12T02:44:33Z

This moves the bucketing behavior into the NSCache itself.

The cache which is currently NSAttributedString -> ASTextCacheValue (a.k.a. [ASTextLayout]) becomes ASTextCacheKey -> ASTextLayout so that, instead of us doing the bucketing and scan, NSCache does it.

ASTextCacheKey is a pair of (NSAttributedString, ASTextContainer), similar to ASTextKitAttributes in the ASTextNode1 world.

The fact is, a given pair of (container, attributed string) generates one layout, which can be used against multiple (container, attributed string) pairs, and this diff makes the -hash and -isEqual: implementations reflect that.

When a text layout is generated, it is assigned back to the cache key that it was built for. Then in the future, -[ASTextCacheKey isEqual:] will really answer the question "is the layout valid for both of these keys?"

ghost · 2018-05-20T16:27:25Z

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

Generated by 🚫 Danger

appleguy · 2018-05-21T00:41:39Z

@@ -110,6 +110,7 @@ - (BOOL)isEqual:(ASTextNodeRendererKey *)object
 dispatch_once(&onceToken, ^{
   __rendererCache = [[NSCache alloc] init];
   __rendererCache.countLimit = 500; // 500 renders cache


I know this is for ASTextNode1...however, this might actually be quite a bit of memory. Do we dump this cache on memory warning? Is there anything simple we can do to make it more responsive to some combination of memory consumption (by objects in the cache), device memory size, or frequency of memory pressure?

I think there's a need for something like ASMemoryManager (or MemoryOverlord lol) that provides some conveniences around memory. Total memory capacity could be bucketed into enums with perhaps a low/med/high — especially iPads would mostly be in low or med, for example. Most valuably would be registering some blocks or observers to call upon memory pressure, and providing an indication of how severe or sustained the memory pressure is.

For example, repeated memory pressure should certainly reduce the countLimit, display range size, or other such parameters rather than just one-time cache dumps. Even if there's no mechanism to increase these again, this would allow us to choose more aggressive defaults without as much concern for apps that may have e.g. many large text blocks.

Filed a tracking task for this idea: #927

A memory management object is really interesting. Take a look at this, which provides 3 levels of severity. This is the hook that NSCache uses. Side note, I'm in favor of removing the explicit count limit here and letting NSCache decide.

https://developer.apple.com/documentation/dispatch/dispatch_source_memory_pressure_event_flags?language=objc

appleguy · 2018-05-21T00:46:11Z

+  // If nil, regenerate.
+  NSAttributedString *_preparedAttributedText;
+
+  // Just a fast cache. May not be valid but good place to check first.


Could you clarify this comment a bit, specifically what about it may not be valid (like if constrainedSize doesn't match?)

Same for "If nil, regenerate" -- could remove this comment, or make it a bit more clear how the variable is used and expectations for how it is maintained.

appleguy · 2018-05-21T00:48:19Z

-@end
-
 /**
 * If set, we will record all values set to attributedText into an array


It would be great to use this feature to collect a bunch of data and add it to our unit tests! This might collect some noise when iOS changes, but it would also be an excellent way to identify breakage / race conditions from core text classes changing (e.g. if we had an auto-scrolling collection showing ASTextCellNodes, measuring them in batches of 20 insertions to the data source).

Yes we've actually got some – AttributedStringsFixture0.plist that I captured from Pinterest a while back. Just need to build a test that uses them.

appleguy · 2018-05-21T00:51:38Z

+  ASTextLayout *layout = [self compatibleLayoutWithContainer:container text:[self l_preparedAttributedText]];

+  // Is this necessary?
  [self setNeedsDisplay];


Hmm, I think you should remove this (as long as the tests pass, or fix them if the tests are not too hard to fix).

As long as we have the layer's .needsDisplayOnBoundsChange set to YES, then this should not be a factor.

One could argue that we should use default-synchronous display whenever the layout changes while interfaceState is Visible. That would fix some of the stretching texture issues that often happen when changing a string and it resizes the frame. However, this would require something like setNeedsSynchronousDisplay, and it would be called in -layout instead of here.

Agree that we shouldn't need to call setNeedsDisplay here.

appleguy · 2018-05-21T00:52:00Z

- (void)prepareAttributedString:(NSMutableAttributedString *)attributedString
+- (NSAttributedString *)l_preparedAttributedText
 {
-  ASLockScopeSelf();


Pretty sure the lock should stay here!

Oops, meant to replace that with a lock assertion. I used the l_ prefix to indicate the method is called while the lock is held. That may be too easy to miss, so I could change it to locked_ although locked assertions will probably prevent people from missing it in reality (unit tests).

Let's use locked_.

appleguy · 2018-05-21T00:52:59Z

@@ -323,10 +320,17 @@ - (NSArray *)exclusionPaths
  return _textContainer.exclusionPaths;


In the method above this (won't let me comment on it), setExclusionPaths:...is it possible for _textContainer to not be allocated yet? Should we call self.textContainer or something?

No, the textContainer is created in -init and kept forever.

A good way to think of it is the text container actually IS the text node in raw form. It stores many of the nodes properties and it's nicely copyable with atomic accessors. The text node itself is a bridge between the container and the rest of ASDK.

appleguy · 2018-05-21T00:54:11Z

+    return _preparedAttributedText;
+  }
+
+  if (!_attributedText) {


Move this condition above the other one, and instead of early return, just do _preparedAttributedText = [[NSAttributedString alloc] init];

Then it will still early return, and _preparedAttributedText will be set.

appleguy · 2018-05-21T00:55:05Z

    shadow.shadowBlurRadius = _shadowRadius;
    [attributedString addAttribute:NSShadowAttributeName value:shadow range:NSMakeRange(0, attributedString.length)];
  }
+  _preparedAttributedText = [attributedString copy];


This second copy is not necessary, right? Since this code is called so much, I'd suggest we just let the mutable object live on, as anyone up-casting it can crash as much as they want :).

So my thing in cases like this, is if I think there's even one downstream copy that is likely, just go ahead and copy now to make that free, to avoid multiple copies of the same object, and to save my mental energy. This method will get called once per node per configuration (change of properties with a layout/render pass in between). So in one data controller batch, this should be called realistically <100 times (say, 25 items with 4 text nodes per).

In this case for example, the attributed string will get copied when we make an ASTextNodeCacheKey from it anyway. And in fact, it will get copied each layout/render pass, so we will do more copying if we don't change properties in between those passes. We could remove THAT copy too, but then we're putting a little crack in our abstractions. These copies will be fast, they will mostly happen off-main, and there will only be ~1 per text node in common cases so I think we should leave it.

appleguy · 2018-05-21T00:55:56Z

+    return [[NSAttributedString alloc] init];
+  }

+  NSMutableAttributedString *attributedString = [_attributedText mutableCopy];


Why was there not a copy here before? Is it definitely needed here, or should we be doing it at a higher level like setAttributedString?

appleguy · 2018-05-21T00:59:00Z

+           @"bgColor": self.backgroundColor ?: [NSNull null],
+           // Pass along self as a hack so that we can update _lastUsedLayout.
+           // We'll be careful
+           @"instance" : self


Hmm, lol. Should we use ASWeakProxy, or is that guaranteed to not make a difference in when we're deallocated?

Since it is possible to have two displays going on at the same time (the sentinel in ASDisplayNode tracks which one should win), I'm not sure it is safe to set _lastUsedLayout unless we're checking which display is intended to / applied as the winner?

Yeah it's definitely possible for the loser to be set – but lastUsedLayout is not considered authoritative. As the comment says, it's just a fast-path for the common case – a first place to check before going to the global layout cache. If we miss, it's not a tragedy.

appleguy · 2018-05-21T00:59:43Z

 *
- * NOTE: Be careful to copy `text` if needed.
+ * NOTE: The `container` you pass into this should be a copy, as it may be
+ * retained by the cache in the event of a cache miss.


Can't we just copy the container in the case of a cache miss? That would be a huge win in some workflows, if that's the only reason / case for the copy.

Agree. It's great if we can reduce the API complexity of this method, and get a perf win in some cases.

The problem is that currently the callers of this method already have to create a copy, in order to change the container size. So the copy is free. If I copy inside this method, we create two copies. Does that make sense?

appleguy · 2018-05-21T01:01:22Z

+  // On a cache miss, this method is going to run long and in some
+  // scenarios it may make sense to run concurrently with different
+  // sizes, so don't hold the lock the whole time.
+  auto lastLayout = ASLockedSelf(_lastUsedLayout);


Doesn't the caller need to have self locked this whole time? Otherwise, it will need to lock after it receives the layout, and re-check that it is still valid.

No, because we're inside a method with immutable parameters. The validity of the layout can't change out from under us – whatever text and container they passed in, that's what we need to respond with. It's up to the consumer to worry about whether those parameters remain valid throughout the method.

I really hated to turn this from a class method to an instance method because it's so unclear that this method simply does not rely on any instance state. It asks the instance for a quick candidate layout to consult first, and it puts the result into that bucket for the next call.

For instance, the layout system calls this with the lock held for the reason you mentioned. The display system is operating on fixed draw params, so it doesn't need to lock the node to call this.

nguyenhuy

Great work!

nguyenhuy · 2018-07-10T22:36:12Z

+  ASTextLayout *layout = [self compatibleLayoutWithContainer:container text:[self l_preparedAttributedText]];

+  // Is this necessary?
  [self setNeedsDisplay];


Agree that we shouldn't need to call setNeedsDisplay here.

nguyenhuy · 2018-07-10T22:38:24Z

- (void)prepareAttributedString:(NSMutableAttributedString *)attributedString
+- (NSAttributedString *)l_preparedAttributedText
 {
-  ASLockScopeSelf();


Let's use locked_.

nguyenhuy · 2018-07-10T22:55:59Z

 *
- * NOTE: Be careful to copy `text` if needed.
+ * NOTE: The `container` you pass into this should be a copy, as it may be
+ * retained by the cache in the event of a cache miss.


Agree. It's great if we can reduce the API complexity of this method, and get a perf win in some cases.

CLAassistant · 2019-07-09T22:01:43Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Adlai Holler seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

nguyenhuy · 2019-07-17T16:28:24Z

@Adlai-Holler Can you follow up on this diff?

Adlai-Holler added the Text label Mar 12, 2018

Adlai-Holler force-pushed the AHTextNode2Simpler branch 2 times, most recently from 3b87fd8 to de902b2 Compare March 13, 2018 03:41

TextureGroup deleted a comment Mar 13, 2018

Adlai-Holler force-pushed the AHTextNode2Simpler branch from e348f49 to 98a3546 Compare May 19, 2018 16:00

Simplify ASTextNode2

d332a8c

Adlai-Holler force-pushed the AHTextNode2Simpler branch from 98a3546 to d332a8c Compare May 19, 2018 19:52

TextureGroup deleted a comment May 19, 2018

Adlai-Holler requested review from appleguy, garrettmoon and maicki and removed request for appleguy and maicki May 19, 2018 19:54

Address CI warnings

c2dc9ad

TextureGroup deleted a comment May 20, 2018

appleguy reviewed May 21, 2018

View reviewed changes

nguyenhuy approved these changes Jul 10, 2018

View reviewed changes

		@@ -323,10 +320,17 @@ - (NSArray *)exclusionPaths
		return _textContainer.exclusionPaths;

Conversation

Adlai-Holler commented Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented May 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nguyenhuy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CLAassistant commented Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nguyenhuy commented Jul 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Adlai-Holler commented Mar 12, 2018 •

edited

Loading

CLAassistant commented Jul 9, 2019 •

edited

Loading