Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reusability hint to improve reuse performance when composite components are present. #146

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

andersio
Copy link
Contributor

@andersio andersio commented Apr 9, 2019

The purpose of this change is pretty much summed up in the documentation for Renderable.makeReusabilityHint(using:).

The design is inspired by how Hashable handles nested hashing.

Do also note that implementing makeReusabilityHint(using:) does not imply that a composite component no longer needs to handle type mismatch in view hierarchy.

@andersio andersio added the ready for review Reviewers can now review the PR label Apr 9, 2019
@andersio andersio self-assigned this Apr 9, 2019
@andersio andersio removed the ready for review Reviewers can now review the PR label Apr 9, 2019
@andersio andersio added the ready for review Reviewers can now review the PR label Apr 9, 2019
}

public extension Renderable {
func makeReusabilityHint(using combiner: inout ReusabilityHintCombiner) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it scan it through the hierarchy? e.g C(children: [C(children: [A()], B()]) vs C(children: [C(children: [B()], A()])?

Copy link
Contributor Author

@andersio andersio Apr 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn’t do so automatically. When you say combiner.combine(child), A’s part of the call it would eventually invoke makeReusabilityHint(using:) on the child. So as long as you combine every child using the combiner, and that all descendants implement this, the whole hierarchy would be walked over.

This can be made less crumblesome if we have a protocol for composite component e.g. CompositeRenderable which is also useful for auto propagation of messages like *LifecycleAware. But this can be done after this PR which lays the cornerstone.

if let view = containedView, type(of: view) == component.viewType {
if let oldComponent = oldComponent,
let view = containedView,
oldComponent.reusabilityHintCombiner.isCompatible(with: component.reusabilityHintCombiner) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not usually happen, should it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All visible items hit this path when the state changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean oldComponent.reusabilityHintCombiner.isCompatible(with: component.reusabilityHintCombiner) to be false. As it'f false it means that we are recreating the view, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeh

@andersio
Copy link
Contributor Author

andersio commented Apr 9, 2019

I actually realise when answering Sergey’s questions about walking the hierarchy, that we need to generate the reuse identifier with not only , but brackets that indicate containment. Otherwise, A([A([B(), C()])]) would resolve to the same symbol as A([A(), B(), C()]) which has a rather different view hierarchy.

That’s also reiterate the value of closing off custom reuseIdentifier, because who knows what surprises you could have when you have components from different authors. 👻

@andersio andersio added 🚧 work in progress and removed ready for review Reviewers can now review the PR labels May 4, 2019
@andersio
Copy link
Contributor Author

andersio commented May 4, 2019

Need to think about how this should integrate with #162.

@andersio andersio mentioned this pull request Jun 17, 2019
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants