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

[Kotlin] Improve and unify Worklow & Worker type comparisons #920

Closed
zach-klippenstein opened this issue Jan 27, 2020 · 1 comment
Closed
Labels
kotlin Affects the Kotlin library. proposal Issue or PR that proposes something and invites comment on whether or not it's a good idea.
Milestone

Comments

@zach-klippenstein
Copy link
Collaborator

zach-klippenstein commented Jan 27, 2020

Background

Currently, when you render children or run workers, both are compared between render passes to determine if they were already rendered/ran on the last pass. But the comparison is implemented differently for workflows and workers: Workflows are compared by concrete type, whereas Workers are compared by the doesSameWorkAs method. Implementations of Worker are responsible for comparing by type.

This approach has a number of issues:

  1. Consistency: You can render two Workflow<Foo, Bar, Baz> workflows that happen to have different type implementations, and it's fine. You can run two Worker<Fizz> workers and the implementation of the first one's doesSameWorkAs determines if that is valid or not. The mental model is complicated.
  2. Abstraction: If you want to build a generic workflow that renders some children, and the children are defined by generic types, it's not clear whether the children need keys or not to be deduped.
  3. Efficiency: Since the runtime has no knowledge of worker types, the only way to compare workers is to do a linear search of the previous ones. The Swift implementation, which does know everything about worker type parameters, uses those types to store workers in a map and do constant-time lookups, which is much more efficient for large numbers of workers.

Proposed Solution

I think we can solve most of these problems in Kotlin using a special Kotlin-only reflection feature called KType. A KType is similar to a Type in java. It represents not only a KClass, but also all the type parameters and their variances and bounds. Since this information is only known at compile time, a KType can only be derived at compile-time, using the inline function typeOf<Foo>().

Implementation Strategy

  1. Make the core renderChild and runningWorker methods take an additional KType parameter for their workflow/worker types. These methods should be internal, since they would be very easy to misuse if public (KType has no type parameters of its own, and we can't control what you pass in).
  2. Since interfaces can't have internal/protected methods, make RenderContext an abstract class instead of an interface.
  3. Convert the current core renderChild and runningWorker methods to extension functions, siblings to all the other convenience overloads.
  4. Make all public-facing renderChild/runningWorker inline, so they can use typeOf.

These steps can be done incrementally, e.g. #908 converted TypedWorker to use KType instead of KClass. The rest of the worker API could be converted next, followed by workflows.

Further Work

Once this is done, a few more changes could be made:

  • Workers could be keyed by their type+key, like in Swift, since the runtime now always knows worker types.
  • The worker conversion functions for reactive types would no longer need to be inline, since they don't need to know their own types.

Open Questions

  1. Currently the snapshotting machinery writes KClass names for workflows, so it can look up those KClasses again on rehydration and use them as keys for the snapshot cache.
    • I think to solve this we can just store either the KType.toString() or the KType itself, and compare whatever we have available. The implementation should be mutable so that equivalent WorkflowIds will converge on all pointing to the same objects, to make comparisons more efficient.
  2. Infuriatingly, Nothing can't be used as a reified type parameter, which means rendering Workflow<*, Nothing, *> and running Worker<Nothing> would need some tricks.
  3. I'm not sure what the runtime performance overhead of calling typeOf all the time is. Worst case it's allocating a whole bunch of reflection-y objects on every call, which would happen many times per render pass. If instead it generates that tree statically at compile time, it's not going to be that expensive. Need to check the generated bytecode.
@zach-klippenstein zach-klippenstein added the proposal Issue or PR that proposes something and invites comment on whether or not it's a good idea. label Jan 27, 2020
@JustinDSN JustinDSN added the kotlin Affects the Kotlin library. label Feb 27, 2020
@JustinDSN JustinDSN added this to the v1.0.0 milestone Mar 3, 2020
@zach-klippenstein
Copy link
Collaborator Author

This is made obsolete by GUWT (#1021), which makes "worker" just another type of workflow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kotlin Affects the Kotlin library. proposal Issue or PR that proposes something and invites comment on whether or not it's a good idea.
Projects
None yet
Development

No branches or pull requests

2 participants