Compose in the Runtime Take 3

[steve-the-edwards]

To address #665

[zach-klippenstein]

We usually discourage this, but since this is framework code that is not public API and will be run a lot, it might be more efficient to just pass in the MutableState itself so Compose doesn't have to memoize this lambda for you.

[steve-the-edwards]

Done.

[zach-klippenstein]

If you're comparing props anyway, I don't see the reason for putting this in a key function (and if it is necessary, it should probably be an implementation detail of UpdatePropsAndState).

[steve-the-edwards]

removed.

[zach-klippenstein]

Nit: Compose conventions discourage naming unit-returning composable functions as verbs.

[steve-the-edwards]

Worthwhile to break convention here I think.

[zach-klippenstein]

I hope everyone heeded our advice to not perform side effects in this function 😅

[steve-the-edwards]

I think we can protect against that anyway like this:

    val newState = produceState(initialValue = state.value, newProps) {
      if (newProps != lastProps) {
        interceptor.intercept(workflow, this@WorkflowNode)
          .onPropsChanged(lastProps, newProps, state.value)
      }
    }
    state.value = newState.value

[steve-the-edwards]

NVM. that does not work because there is nothing actually reproducing the state!

This works though:

key (newProps) {
      if (newProps != lastProps) {
        state.value = interceptor.intercept(workflow, this@WorkflowNode)
          .onPropsChanged(lastProps, newProps, state.value)
      }
    }

[zach-klippenstein]

Nit: I don't see you using the delegate syntax for MutableStates very much, it would make a lot of the code in this PR more idiomatic and a teeny bit cleaner.

[steve-the-edwards]

The delegate syntax continues to confuse me and I don't necessarily like the syntactic sugar obfuscating what is happening.

In this case currentProps is a class property. How would I use the delegate syntax here as I thought that's only when using remember? And remember is only for within a @Composable?

[zach-klippenstein]

Remember has nothing to do with delegation. Property delegation is a well-documented Kotlin language feature. It's supported for MutableState because it has getValue and setValue extension methods. More info on how it works here.

[steve-the-edwards]

Ah I was getting confused by all the examples including the remember in the delegate. I didn't realize that even outside of that it was actually MutableState that had the getValue/setValue so it could be used with delegation.

I see now that you can just use:

val currentProps: PropsT by mutableStateOf(props.value)

that makes a lot more sense.

[zach-klippenstein]

I don't believe this is safe to call from composition since it modifies non-snapshot state in this class.

[steve-the-edwards]

given that SideEffect only occurs after Composition though and we need this stagedChild to call the Rendering waht are the options here?

I thought remember would effectively avoid calling this multiple times across recomposition?

Do you mean that the StagingList needs to be State?

[zach-klippenstein]

Remember will prevent calling it again on subsequent compositions, but it will still be called if the initial composition is discarded and never applied. It is probably enough to make the staged child list a SnapshotStateList.

[steve-the-edwards]

So if I am understanding you correctly, we would make a @Composable StagedChild() that would have a SideEffect that would create the child node and add it to the staging list.

That way that would only be called if StagedChild was recomposed successfully, but we could still rely on its side effect by the time the Rendering is composed.

[steve-the-edwards]

I think i'm just going to have to re-write or modify ActiveStagingList in a Snapshot-State-friendly way

[steve-the-edwards]

  /**
   * Prepare the staged child while only modifying [children] in a SideEffect. This will ensure
   * that we do not inappropriately modify non-snapshot state.
   */
  @Composable
  private fun <ChildPropsT, ChildOutputT, ChildRenderingT> StagedChild(
    child: Workflow<ChildPropsT, ChildOutputT, ChildRenderingT>,
    props: ChildPropsT,
    key: String,
    handler: (ChildOutputT) -> WorkflowAction<PropsT, StateT, OutputT>
  ): WorkflowChildNode<*, *, *, *, *> {
    val childState = remember (child, key, props, handler) {
      children.forEachStaging {
        require(!(it.matches(child, key))) {
          "Expected keys to be unique for ${child.identifier}: key=\"$key\""
        }
      }
      mutableStateOf(children.firstActiveMatchingOrNull {
        it.matches(child, key)
      } ?: createChildNode(child, props, key, handler))
    }

    SideEffect {
      children.swapOrStage(
        predicate = {it.matches(child, key)},
        child = childState.value)
    }
    return childState.value
  }

[steve-the-edwards]

OK so I have updated this with a recent commit that isolates all of teh Compose optimizations as a plugin module

[steve-the-edwards]

1. Trying to call into the Compose runtime directly (without it being a Composable) was a dead end for now as I worked on it for a bit with Zach and we got past his knowledge and idea of where to go next, so letting that go. The impact of this is we need a StatefulWorkflow/StatelessWorkflow extension to add the @composable Rendering - but I think i did that relatively nicely.
2. This update is passing all the tests locally and is behind a warning annotation.
3. You would add the workflow-core-compose module and plugin the optimization to the runtime when calling renderWorkflowIn, as I do in performance poetry (and it shows the same rendering oprtimizations on the tests).
4. I like where this is at for pausing as its all isolated and I could likely rebase this without too much difficulty. (Obviously it doesn't have any compose runtime specific tests yet so I would want to add that when picking it back up).
5. Unfortunately I had to pull all of our runtime.internal classes into public land because we (still I think?) don't have good 'friend path' support for kotlin modules - and since I'm extending them all in the compose-core module I need access.
6. What I would like to do with this is turn off the publishing of these and the explicit API mode. So they can still be public for the other module but won't be part of the published API (haven't done this yet). Unless someone has a good idea for package-protected in kotlin?
7. Some of the polymorphism got a bit messier than at other times - would really appreciate ideas as to how it could be cleaner at points. WorkflowNode initialization is really the ugliest one as I had to add a startSession so that the initialization happens after the child class is instantiated and not just after the parent class.

[steve-the-edwards]

Superseded by #886