From c831621b770a58aeb366a4a85eb8333182f8bfc2 Mon Sep 17 00:00:00 2001 From: Stephen Edwards Date: Fri, 9 Dec 2022 16:06:23 -0800 Subject: [PATCH] Cache WorkflowIdentifier Disable the strict memory model tests now that we are on Kotlin 1.7.20. --- .github/workflows/kotlin.yml | 8 ---- .../hellocomposeworkflow/ComposeWorkflow.kt | 6 ++- workflow-core/api/workflow-core.api | 14 ++++++- .../com/squareup/workflow1/IdCacheable.kt | 14 +++++++ .../squareup/workflow1/ImpostorWorkflow.kt | 2 +- .../squareup/workflow1/StatefulWorkflow.kt | 11 +++++- .../squareup/workflow1/StatelessWorkflow.kt | 11 +++++- .../squareup/workflow1/WorkflowIdentifier.kt | 39 +++++++++++++++++++ 8 files changed, 91 insertions(+), 14 deletions(-) create mode 100644 workflow-core/src/commonMain/kotlin/com/squareup/workflow1/IdCacheable.kt diff --git a/.github/workflows/kotlin.yml b/.github/workflows/kotlin.yml index 9203b2cdb..8c9621fe1 100644 --- a/.github/workflows/kotlin.yml +++ b/.github/workflows/kotlin.yml @@ -149,14 +149,6 @@ jobs : iosX64Test --stacktrace cache-read-only: false - ## iOS Specific Tests w/ strict memory model (for KMP ios actuals in core and runtime). - - uses: gradle/gradle-build-action@v2 - name : Check with Gradle - with : - arguments : | - iosX64Test -Pkotlin.native.binary.memoryModel=strict --stacktrace - cache-read-only: false - # Report as Github Pull Request Check. - name : Publish Test Report uses : mikepenz/action-junit-report@v3 diff --git a/samples/compose-samples/src/main/java/com/squareup/sample/compose/hellocomposeworkflow/ComposeWorkflow.kt b/samples/compose-samples/src/main/java/com/squareup/sample/compose/hellocomposeworkflow/ComposeWorkflow.kt index da39ae62f..8ca7ab0ea 100644 --- a/samples/compose-samples/src/main/java/com/squareup/sample/compose/hellocomposeworkflow/ComposeWorkflow.kt +++ b/samples/compose-samples/src/main/java/com/squareup/sample/compose/hellocomposeworkflow/ComposeWorkflow.kt @@ -1,10 +1,12 @@ package com.squareup.sample.compose.hellocomposeworkflow import androidx.compose.runtime.Composable +import com.squareup.workflow1.IdCacheable import com.squareup.workflow1.RenderContext import com.squareup.workflow1.Sink import com.squareup.workflow1.StatefulWorkflow import com.squareup.workflow1.Workflow +import com.squareup.workflow1.WorkflowIdentifier import com.squareup.workflow1.ui.ViewEnvironment import com.squareup.workflow1.ui.WorkflowUiExperimentalApi import com.squareup.workflow1.ui.compose.ComposeScreen @@ -29,7 +31,7 @@ import com.squareup.workflow1.ui.compose.ComposeScreen */ @WorkflowUiExperimentalApi abstract class ComposeWorkflow : - Workflow { + Workflow, IdCacheable { /** * Renders [props] by emitting Compose UI. This function will be called to update the UI whenever @@ -48,6 +50,8 @@ abstract class ComposeWorkflow : override fun asStatefulWorkflow(): StatefulWorkflow = ComposeWorkflowImpl(this) + + override var cachedIdentifier: WorkflowIdentifier? = null } /** diff --git a/workflow-core/api/workflow-core.api b/workflow-core/api/workflow-core.api index 5a896de3e..74ad42fee 100644 --- a/workflow-core/api/workflow-core.api +++ b/workflow-core/api/workflow-core.api @@ -48,6 +48,11 @@ public final class com/squareup/workflow1/BaseRenderContext$DefaultImpls { public static synthetic fun renderChild$default (Lcom/squareup/workflow1/BaseRenderContext;Lcom/squareup/workflow1/Workflow;Ljava/lang/Object;Ljava/lang/String;Lkotlin/jvm/functions/Function1;ILjava/lang/Object;)Ljava/lang/Object; } +public abstract interface class com/squareup/workflow1/IdCacheable { + public abstract fun getCachedIdentifier ()Lcom/squareup/workflow1/WorkflowIdentifier; + public abstract fun setCachedIdentifier (Lcom/squareup/workflow1/WorkflowIdentifier;)V +} + public abstract interface class com/squareup/workflow1/ImpostorWorkflow { public abstract fun describeRealIdentifier ()Ljava/lang/String; public abstract fun getRealIdentifier ()Lcom/squareup/workflow1/WorkflowIdentifier; @@ -115,12 +120,14 @@ public final class com/squareup/workflow1/Snapshots { public static final fun writeUtf8WithLength (Lokio/BufferedSink;Ljava/lang/String;)Lokio/BufferedSink; } -public abstract class com/squareup/workflow1/StatefulWorkflow : com/squareup/workflow1/Workflow { +public abstract class com/squareup/workflow1/StatefulWorkflow : com/squareup/workflow1/IdCacheable, com/squareup/workflow1/Workflow { public fun ()V public final fun asStatefulWorkflow ()Lcom/squareup/workflow1/StatefulWorkflow; + public fun getCachedIdentifier ()Lcom/squareup/workflow1/WorkflowIdentifier; public abstract fun initialState (Ljava/lang/Object;Lcom/squareup/workflow1/Snapshot;)Ljava/lang/Object; public fun onPropsChanged (Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object; public abstract fun render (Ljava/lang/Object;Ljava/lang/Object;Lcom/squareup/workflow1/StatefulWorkflow$RenderContext;)Ljava/lang/Object; + public fun setCachedIdentifier (Lcom/squareup/workflow1/WorkflowIdentifier;)V public abstract fun snapshotState (Ljava/lang/Object;)Lcom/squareup/workflow1/Snapshot; } @@ -141,10 +148,12 @@ public final class com/squareup/workflow1/StatefulWorkflow$RenderContext : com/s public fun runningSideEffect (Ljava/lang/String;Lkotlin/jvm/functions/Function2;)V } -public abstract class com/squareup/workflow1/StatelessWorkflow : com/squareup/workflow1/Workflow { +public abstract class com/squareup/workflow1/StatelessWorkflow : com/squareup/workflow1/IdCacheable, com/squareup/workflow1/Workflow { public fun ()V public final fun asStatefulWorkflow ()Lcom/squareup/workflow1/StatefulWorkflow; + public fun getCachedIdentifier ()Lcom/squareup/workflow1/WorkflowIdentifier; public abstract fun render (Ljava/lang/Object;Lcom/squareup/workflow1/StatelessWorkflow$RenderContext;)Ljava/lang/Object; + public fun setCachedIdentifier (Lcom/squareup/workflow1/WorkflowIdentifier;)V } public final class com/squareup/workflow1/StatelessWorkflow$RenderContext : com/squareup/workflow1/BaseRenderContext { @@ -284,6 +293,7 @@ public final class com/squareup/workflow1/Workflows { public static synthetic fun action$default (Ljava/lang/String;Lkotlin/jvm/functions/Function1;ILjava/lang/Object;)Lcom/squareup/workflow1/WorkflowAction; public static final fun applyTo (Lcom/squareup/workflow1/WorkflowAction;Ljava/lang/Object;Ljava/lang/Object;)Lkotlin/Pair; public static final fun contraMap (Lcom/squareup/workflow1/Sink;Lkotlin/jvm/functions/Function1;)Lcom/squareup/workflow1/Sink; + public static final fun getComputedIdentifier (Lcom/squareup/workflow1/Workflow;)Lcom/squareup/workflow1/WorkflowIdentifier; public static final fun getIdentifier (Lcom/squareup/workflow1/Workflow;)Lcom/squareup/workflow1/WorkflowIdentifier; public static final fun mapRendering (Lcom/squareup/workflow1/Workflow;Lkotlin/jvm/functions/Function1;)Lcom/squareup/workflow1/Workflow; public static final fun renderChild (Lcom/squareup/workflow1/BaseRenderContext;Lcom/squareup/workflow1/Workflow;Ljava/lang/Object;Ljava/lang/String;)Ljava/lang/Object; diff --git a/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/IdCacheable.kt b/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/IdCacheable.kt new file mode 100644 index 000000000..af6d944fb --- /dev/null +++ b/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/IdCacheable.kt @@ -0,0 +1,14 @@ +package com.squareup.workflow1 + +/** + * If your Workflow caches its [WorkflowIdentifier] (to avoid frequent lookups) then implement + * this interface. Note that [StatefulWorkflow] and [StatelessWorkflow] already implement this, + * so you only need to do so if you do not extend one of those classes. + * + * Your Workflow can just assign null to this value as the [identifier] extension will use it + * for caching. + */ +public interface IdCacheable { + + public var cachedIdentifier: WorkflowIdentifier? +} diff --git a/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/ImpostorWorkflow.kt b/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/ImpostorWorkflow.kt index 4e75c8f7d..9d6901f6d 100644 --- a/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/ImpostorWorkflow.kt +++ b/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/ImpostorWorkflow.kt @@ -18,7 +18,7 @@ import kotlin.jvm.JvmName public interface ImpostorWorkflow { /** * The [WorkflowIdentifier] of another workflow to be combined with the identifier of this - * workflow, as obtained by [Workflow.identifier]. + * workflow, as obtained by [Workflow.computedIdentifier]. * * For workflows that implement operators, this should be the identifier of the upstream * [Workflow] that this workflow wraps. diff --git a/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/StatefulWorkflow.kt b/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/StatefulWorkflow.kt index 031235ff4..a91e79252 100644 --- a/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/StatefulWorkflow.kt +++ b/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/StatefulWorkflow.kt @@ -6,6 +6,7 @@ package com.squareup.workflow1 import com.squareup.workflow1.StatefulWorkflow.RenderContext import com.squareup.workflow1.WorkflowAction.Companion.toString +import kotlin.LazyThreadSafetyMode.NONE import kotlin.jvm.JvmMultifileClass import kotlin.jvm.JvmName @@ -69,7 +70,7 @@ public abstract class StatefulWorkflow< StateT, out OutputT, out RenderingT - > : Workflow { + > : Workflow, IdCacheable { public inner class RenderContext internal constructor( baseContext: BaseRenderContext @@ -129,6 +130,14 @@ public abstract class StatefulWorkflow< context: RenderContext ): RenderingT + /** + * Use a lazy delegate so that any [ImpostorWorkflow.realIdentifier] will have been computed + * before this is initialized and cached. + * + * We use [LazyThreadSafetyMode.NONE] because access to these identifiers is thread-confined. + */ + override var cachedIdentifier: WorkflowIdentifier? = null + /** * Called whenever the state changes to generate a new [Snapshot] of the state. * diff --git a/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/StatelessWorkflow.kt b/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/StatelessWorkflow.kt index f6545524e..68848afe9 100644 --- a/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/StatelessWorkflow.kt +++ b/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/StatelessWorkflow.kt @@ -3,6 +3,7 @@ package com.squareup.workflow1 +import kotlin.LazyThreadSafetyMode.NONE import kotlin.jvm.JvmMultifileClass import kotlin.jvm.JvmName @@ -24,7 +25,7 @@ import kotlin.jvm.JvmName * @see StatefulWorkflow */ public abstract class StatelessWorkflow : - Workflow { + Workflow, IdCacheable { @Suppress("UNCHECKED_CAST") public inner class RenderContext internal constructor( @@ -57,6 +58,14 @@ public abstract class StatelessWorkflow context: RenderContext ): RenderingT + /** + * Use a lazy delegate so that any [ImpostorWorkflow.realIdentifier] will have been computed + * before this is initialized and cached. + * + * We use [LazyThreadSafetyMode.NONE] because access to these identifiers is thread-confined. + */ + override var cachedIdentifier: WorkflowIdentifier? = null + /** * Satisfies the [Workflow] interface by wrapping `this` in a [StatefulWorkflow] with `Unit` * state. diff --git a/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/WorkflowIdentifier.kt b/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/WorkflowIdentifier.kt index a15583453..06ebb9392 100644 --- a/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/WorkflowIdentifier.kt +++ b/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/WorkflowIdentifier.kt @@ -113,6 +113,13 @@ public class WorkflowIdentifier internal constructor( return result } + /** + * Used to detect when this [WorkflowIdentifier] is a deeply stubbed mock. Stubs are provided + * until a primitive, in this case the typeName. This lets us determine if we are a mock + * object, or a real one. Mea culpa. + */ + internal val deepNameCheck = type.typeName + public companion object { private const val NO_PROXY_IDENTIFIER_TAG = 0.toByte() private const val PROXY_IDENTIFIER_TAG = 1.toByte() @@ -145,8 +152,40 @@ public class WorkflowIdentifier internal constructor( /** * The [WorkflowIdentifier] that identifies this [Workflow]. + * + * This can carry some cost if the Workflow class does not implement [IdCacheable]. Note that + * [StatelessWorkflow] and [StatefulWorkflow] do implement the caching. */ public val Workflow<*, *, *>.identifier: WorkflowIdentifier + get() { + return when (this) { + is IdCacheable -> { + // The following lines look more complex than they need to be. If we have not yet cached + // the identifier, we do. But we return the [computedIdentifier] value directly as in the + // case of tests which use mocks we want to ensure that we return what is on line 180 - + // "WorkflowIdentifier(Snapshottable(this::class))" as that depends solely on types. + // We do the 'senseless' comparison of .type.typeName here to detect the case where we + // we have mocks with deep stubs but the name itself (a String) is null. + // The reason this is so complicated is this caching has been added afterword via the + // [IdCacheable] interface so that the [Workflow] interface itself remains unchanged. + @Suppress("SENSELESS_COMPARISON") + if (cachedIdentifier == null || cachedIdentifier!!.deepNameCheck == null) { + return computedIdentifier.also { + cachedIdentifier = it + } + } + cachedIdentifier!! + } + else -> computedIdentifier + } + } + +/** + * The computed [WorkflowIdentifier] for this Workflow. Any [IdCacheable] Workflow should call this + * and then store the value in the cachedIdentifier property so as to prevent + * the extra work needed to create the [WorkflowIdentifier] and look up the class name each time. + */ +public val Workflow<*, *, *>.computedIdentifier: WorkflowIdentifier get() { val maybeImpostor = this as? ImpostorWorkflow return WorkflowIdentifier(