Lift memoization out of core - #678
Conversation
|
@facebook-github-bot import |
|
@ocrickard has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@ocrickard updated the pull request - view changes - changes since last import |
f833e6b to
4b2710b
Compare
|
@ocrickard updated the pull request - view changes - changes since last import |
|
@darknoon feel free to take a look! |
|
This looks great! Good to see it being moved out of the core. |
eczarny
left a comment
eczarny
left a comment
There was a problem hiding this comment.
@ocrickard I think this is a great change! I think this is largely fine as-is but it would be nice to better consolidate the public-facing aspects of the memoization API.
| restrictedToSize:size | ||
| relativeToParentSize:parentSize]; | ||
| }); | ||
| } |
There was a problem hiding this comment.
It may be a good idea to add these comments to the CKMemoizeLayout function itself.
How much of this header is actually part of the public-facing API? I ask because there is a fair amount of stuff here and we should take this opportunity to clean this up a bit if we want to.
There was a problem hiding this comment.
Hmm, I'd like to handle hiding this stuff later. There are a few functions we can move out-of-line, but most of it has to be public.
| CKComponentLayout CKMemoizeOrComputeLayout(CKComponent *component, CKSizeRange constrainedSize, const CKComponentSize& size, CGSize parentSize) | ||
| CKComponentLayout CKMemoizeLayout(CKComponent *component, CKSizeRange constrainedSize, const CKComponentSize& size, CGSize parentSize, CKComponentLayout (^block)()) | ||
| { | ||
| if (component && [component shouldMemoizeLayout]) { |
There was a problem hiding this comment.
This checks if the component is non-nil. Is that something we still need to guarantee?
There was a problem hiding this comment.
Sure, I can add it back. I don't think it matters though. Pretty sure we're nil-safe here.
|
@ocrickard back to you for my feedback and resolving merge conflicts. |
4b2710b to
a6cd990
Compare
|
@ocrickard updated the pull request - view changes - changes since last import |
facebook-github-bot
added
GH Review: review-needed
and removed
GH Review: needs-revision
labels
a6cd990 to
e62a1dc
Compare
|
@ocrickard updated the pull request - view changes - changes since last import |
e62a1dc to
5b84cd2
Compare
|
@ocrickard updated the pull request - view changes - changes since last import |
|
@facebook-github-bot import |
|
Alright, this is landing internally. Travis seems super backed up, but I'll wait for a green build here before merging. |
6 participants
My goal here is to reduce the need for customizations to the build and layout calls in every rendering context that wants memoization. Here's an example of what I'm trying to avoid:
#675
If your component hierarchy needs memoization, it should work regardless of how you configure your components, and ideally we wouldn't have to do this type of change to every place that builds components.
In this diff I introduce CKMemoizingComponent which prepares the thread-local state for component and layout memoization for its children. This component stores its memoization state into its scope's state.
Due to the nature of the thread-local stack that memoizers are built with, it should be fine to have multiple memoization components in the hierarchy with little cost, and everything "just works". This is important, since if you embed a sub-hierarchy that adds one of these so it can memoize in a different context, you don't want it to conflict with a higher-level memoizing component wrapper.
I'd like to land this first, then look at fixing up the scopes/states as in #348 after this is in master. Looking at that diff, we can apply basically the same process here.