Separate tracking changes inside record scope #1574
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
igordmn
reviewed
Sep 18, 2024
| @@ -316,7 +316,7 @@ internal class RootNodeOwner( | |||
| override val inputModeManager get() = platformContext.inputModeManager | |||
| override val clipboardManager = PlatformClipboardManager() | |||
| override val accessibilityManager = DefaultAccessibilityManager() | |||
| override val graphicsContext: GraphicsContext = GraphicsContext() | |||
| override val graphicsContext = GraphicsContext(this@RootNodeOwner.snapshotObserver.observer) | |||
There was a problem hiding this comment.
It seems we can create own observer, no need to reuse the one from OwnerSnapshotObserver
There was a problem hiding this comment.
Let's do this in a separate PR
igordmn
approved these changes
Sep 18, 2024
igordmn
pushed a commit
that referenced
this pull request
Sep 18, 2024
Fixes https://youtrack.jetbrains.com/issue/CMP-6695 ## Testing Test case from the issue. This should be tested by QA ## Release Notes N/A (regression after #1555) wasn't released yet
MatkovIvan
added a commit
that referenced
this pull request
Sep 19, 2024
As @igordmn [noticed](#1574 (comment)) it's better to use separate observer. Current solution might cause ``` Exception in thread "AWT-EventQueue-0" java.lang.ClassCastException: class androidx.compose.ui.graphics.layer.GraphicsLayer cannot be cast to class androidx.compose.ui.node.OwnerScope (androidx.compose.ui.graphics.layer.GraphicsLayer and androidx.compose.ui.node.OwnerScope are in unnamed module of loader 'app') at androidx.compose.ui.node.OwnerSnapshotObserver$clearInvalidObservations$1.invoke(OwnerSnapshotObserver.kt:137) at androidx.compose.ui.node.OwnerSnapshotObserver$clearInvalidObservations$1.invoke(OwnerSnapshotObserver.kt:137) at androidx.compose.runtime.snapshots.SnapshotStateObserver$ObservedScopeMap.removeScopeIf(SnapshotStateObserver.kt:541) at androidx.compose.runtime.snapshots.SnapshotStateObserver.clearIf(SnapshotStateObserver.kt:307) at androidx.compose.ui.node.OwnerSnapshotObserver.clearInvalidObservations$ui(OwnerSnapshotObserver.kt:137) at androidx.compose.ui.node.RootNodeOwner.clearInvalidObservations(RootNodeOwner.skiko.kt:188) at androidx.compose.ui.node.RootNodeOwner.draw(RootNodeOwner.skiko.kt:228) at androidx.compose.ui.scene.CanvasLayersComposeSceneImpl.draw(CanvasLayersComposeScene.skiko.kt:250) at androidx.compose.ui.scene.BaseComposeScene.render(BaseComposeScene.skiko.kt:190) at androidx.compose.ui.scene.ComposeSceneMediator$onRender$1$1.invoke(ComposeSceneMediator.desktop.kt:574) at androidx.compose.ui.scene.ComposeSceneMediator$onRender$1$1.invoke(ComposeSceneMediator.desktop.kt:572) at androidx.compose.ui.viewinterop.SwingInteropContainer.postponingExecutingScheduledUpdates(SwingInteropContainer.desktop.kt:229) at androidx.compose.ui.scene.ComposeSceneMediator.onRender(ComposeSceneMediator.desktop.kt:572) at org.jetbrains.skiko.SkiaLayer.update$skiko(SkiaLayer.awt.kt:533) at org.jetbrains.skiko.redrawer.AWTRedrawer.update(AWTRedrawer.kt:54) at org.jetbrains.skiko.redrawer.MetalRedrawer$frameDispatcher$1.invokeSuspend(MetalRedrawer.kt:83) at org.jetbrains.skiko.redrawer.MetalRedrawer$frameDispatcher$1.invoke(MetalRedrawer.kt) at org.jetbrains.skiko.redrawer.MetalRedrawer$frameDispatcher$1.invoke(MetalRedrawer.kt) at org.jetbrains.skiko.FrameDispatcher$job$1.invokeSuspend(FrameDispatcher.kt:33) ``` Fixes https://youtrack.jetbrains.com/issue/CMP-6705 ## Testing No local reproduction 😢 , just report from @chrisbanes This should be tested by QA
igordmn
pushed a commit
that referenced
this pull request
Sep 19, 2024
As @igordmn [noticed](#1574 (comment)) it's better to use separate observer. Current solution might cause ``` Exception in thread "AWT-EventQueue-0" java.lang.ClassCastException: class androidx.compose.ui.graphics.layer.GraphicsLayer cannot be cast to class androidx.compose.ui.node.OwnerScope (androidx.compose.ui.graphics.layer.GraphicsLayer and androidx.compose.ui.node.OwnerScope are in unnamed module of loader 'app') at androidx.compose.ui.node.OwnerSnapshotObserver$clearInvalidObservations$1.invoke(OwnerSnapshotObserver.kt:137) at androidx.compose.ui.node.OwnerSnapshotObserver$clearInvalidObservations$1.invoke(OwnerSnapshotObserver.kt:137) at androidx.compose.runtime.snapshots.SnapshotStateObserver$ObservedScopeMap.removeScopeIf(SnapshotStateObserver.kt:541) at androidx.compose.runtime.snapshots.SnapshotStateObserver.clearIf(SnapshotStateObserver.kt:307) at androidx.compose.ui.node.OwnerSnapshotObserver.clearInvalidObservations$ui(OwnerSnapshotObserver.kt:137) at androidx.compose.ui.node.RootNodeOwner.clearInvalidObservations(RootNodeOwner.skiko.kt:188) at androidx.compose.ui.node.RootNodeOwner.draw(RootNodeOwner.skiko.kt:228) at androidx.compose.ui.scene.CanvasLayersComposeSceneImpl.draw(CanvasLayersComposeScene.skiko.kt:250) at androidx.compose.ui.scene.BaseComposeScene.render(BaseComposeScene.skiko.kt:190) at androidx.compose.ui.scene.ComposeSceneMediator$onRender$1$1.invoke(ComposeSceneMediator.desktop.kt:574) at androidx.compose.ui.scene.ComposeSceneMediator$onRender$1$1.invoke(ComposeSceneMediator.desktop.kt:572) at androidx.compose.ui.viewinterop.SwingInteropContainer.postponingExecutingScheduledUpdates(SwingInteropContainer.desktop.kt:229) at androidx.compose.ui.scene.ComposeSceneMediator.onRender(ComposeSceneMediator.desktop.kt:572) at org.jetbrains.skiko.SkiaLayer.update$skiko(SkiaLayer.awt.kt:533) at org.jetbrains.skiko.redrawer.AWTRedrawer.update(AWTRedrawer.kt:54) at org.jetbrains.skiko.redrawer.MetalRedrawer$frameDispatcher$1.invokeSuspend(MetalRedrawer.kt:83) at org.jetbrains.skiko.redrawer.MetalRedrawer$frameDispatcher$1.invoke(MetalRedrawer.kt) at org.jetbrains.skiko.redrawer.MetalRedrawer$frameDispatcher$1.invoke(MetalRedrawer.kt) at org.jetbrains.skiko.FrameDispatcher$job$1.invokeSuspend(FrameDispatcher.kt:33) ``` Fixes https://youtrack.jetbrains.com/issue/CMP-6705 ## Testing No local reproduction 😢 , just report from @chrisbanes This should be tested by QA
igordmn
added a commit
that referenced
this pull request
Sep 24, 2024
Fixes https://youtrack.jetbrains.com/issue/CMP-6729/iOS-Runtime-crash.-Unsupported-concurrent-change-during-composition Regression after #1574 The draw call is similar to: ``` // NodeCoordinator.drawBlock observer.observeReads(Unit, { RenderNodeLayer.invalidate() drawState.value = Unit }) { RenderNodeLayer.drawLayer() drawState.value } ``` So, triggering the observer in response to the state change also changes this state, which triggers observer again. It is sometimes manifested via this call: ``` performRecompose() val snapshot = Snapshot.takeMutableSnapshot() Snapshot.sendApplyNotifications() // triggers drawState.value = Unit snapshot.enter { ... Snapshot.sendApplyNotifications() // can be called anywhere, triggers the observer of `drawState` because we changed `drawState` during the previous `sendApplyNotifications` } snapshot.apply() // fails with "Unsupported concurrent change during composition" ``` (I haven't figured why sometimes and not always) Considering this, *we shouldn't change any state we read in `draw` inside `invalidate`* (because *we shouldn't change any state we read in `observeReads(block=)`) inside observeReads(onValueChangedForScope=)`*) We added this state not to trigger `RenderNodeLayer.drawLayer` after `RenderNodeLayer.invalidate`, but to trigger `SkiaGraphicsLayer.invalidate` after we called `RenderNodeLayer.drawLayer` inside `SkiaGraphicsLayer.draw`. Because: - we still need some way to notify `SkiaGraphicsLayer.invalidate` - only `RenderNodeLayer.invalidate` knows that `RenderNodeLayer.drawLayer` is changed We: - TODO <details> <summary>A synthetic reproducer of the issue for history (skip it if details are already understood)</summary> ``` import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.neverEqualPolicy import androidx.compose.runtime.snapshots.Snapshot import androidx.compose.runtime.snapshots.SnapshotStateObserver fun main() { val drawState = mutableStateOf(Unit, neverEqualPolicy()) val observer = SnapshotStateObserver { it.invoke() } observer.start() fun draw() { observer.observeReads(Unit, { println("invalidate draw (drawState.value = Unit)") drawState.value = Unit }) { println("\ndraw") drawState.value } } fun externalChange() { println("\nexternalChange") drawState.value = Unit Snapshot.sendApplyNotifications() } fun composition() { println("\ncomposition BEFORE") Snapshot.takeMutableSnapshot().apply { enter { println("composition BEGIN") println("sendApplyNotifications") Snapshot.sendApplyNotifications() println("composition END") } println("applying composition state to global state") if (!apply().succeeded) { throw RuntimeException("Composition apply failed") } } } // frame 0 draw() externalChange() // frame 1 composition() } ``` Output: ``` draw externalChange invalidate draw (drawState.value = Unit) composition BEFORE invalidate draw (drawState.value = Unit) composition BEGIN sendApplyNotifications invalidate draw (drawState.value = Unit) composition END applying composition state to global state Exception in thread "main" java.lang.RuntimeException: Composition apply failed at MainKt.main$composition(main.kt:39) at MainKt.main(main.kt:50) at MainKt.main(main.kt) ``` </details> ## Release Notes ### Fixes - Multiplatform - _(prerelease fix)_ Fix "Unsupported concurrent change during composition"
igordmn
added a commit
that referenced
this pull request
Sep 25, 2024
Fixes https://youtrack.jetbrains.com/issue/CMP-6729/iOS-Runtime-crash.-Unsupported-concurrent-change-during-composition Regression after #1574 The draw call is similar to: ``` // NodeCoordinator.drawBlock observer.observeReads(Unit, { RenderNodeLayer.invalidate() drawState.value = Unit }) { RenderNodeLayer.drawLayer() drawState.value } ``` So, triggering the observer in response to the state change also changes this state, which triggers observer again. It is sometimes manifested via this call: ``` performRecompose() val snapshot = Snapshot.takeMutableSnapshot() Snapshot.sendApplyNotifications() // triggers drawState.value = Unit snapshot.enter { ... Snapshot.sendApplyNotifications() // can be called anywhere, triggers the observer of `drawState` because we changed `drawState` during the previous `sendApplyNotifications` } snapshot.apply() // fails with "Unsupported concurrent change during composition" ``` (I haven't figured why sometimes and not always) Considering this, *we shouldn't change any state we read in `draw` inside `invalidate`* (because *we shouldn't change any state we read in `observeReads(block=)`) inside observeReads(onValueChangedForScope=)`*) We added this state not to trigger `RenderNodeLayer.drawLayer` after `RenderNodeLayer.invalidate`, but to trigger `SkiaGraphicsLayer.invalidate` after we called `RenderNodeLayer.drawLayer` inside `SkiaGraphicsLayer.draw`. Because: - we still need some way to notify `SkiaGraphicsLayer.invalidate` - only `RenderNodeLayer.invalidate` knows that `RenderNodeLayer.drawLayer` is changed We: - TODO <details> <summary>A synthetic reproducer of the issue for history (skip it if details are already understood)</summary> ``` import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.neverEqualPolicy import androidx.compose.runtime.snapshots.Snapshot import androidx.compose.runtime.snapshots.SnapshotStateObserver fun main() { val drawState = mutableStateOf(Unit, neverEqualPolicy()) val observer = SnapshotStateObserver { it.invoke() } observer.start() fun draw() { observer.observeReads(Unit, { println("invalidate draw (drawState.value = Unit)") drawState.value = Unit }) { println("\ndraw") drawState.value } } fun externalChange() { println("\nexternalChange") drawState.value = Unit Snapshot.sendApplyNotifications() } fun composition() { println("\ncomposition BEFORE") Snapshot.takeMutableSnapshot().apply { enter { println("composition BEGIN") println("sendApplyNotifications") Snapshot.sendApplyNotifications() println("composition END") } println("applying composition state to global state") if (!apply().succeeded) { throw RuntimeException("Composition apply failed") } } } // frame 0 draw() externalChange() // frame 1 composition() } ``` Output: ``` draw externalChange invalidate draw (drawState.value = Unit) composition BEFORE invalidate draw (drawState.value = Unit) composition BEGIN sendApplyNotifications invalidate draw (drawState.value = Unit) composition END applying composition state to global state Exception in thread "main" java.lang.RuntimeException: Composition apply failed at MainKt.main$composition(main.kt:39) at MainKt.main(main.kt:50) at MainKt.main(main.kt) ``` </details> ## Release Notes ### Fixes - Multiplatform - _(prerelease fix)_ Fix "Unsupported concurrent change during composition"
igordmn
added a commit
that referenced
this pull request
Sep 25, 2024
Fixes https://youtrack.jetbrains.com/issue/CMP-6729/iOS-Runtime-crash.-Unsupported-concurrent-change-during-composition Regression after #1574 The draw call is similar to: ``` // NodeCoordinator.drawBlock observer.observeReads(Unit, { RenderNodeLayer.invalidate() drawState.value = Unit }) { RenderNodeLayer.drawLayer() drawState.value } ``` So, triggering the observer in response to the state change also changes this state, which triggers observer again. It is sometimes manifested via this call: ``` performRecompose() val snapshot = Snapshot.takeMutableSnapshot() Snapshot.sendApplyNotifications() // triggers drawState.value = Unit snapshot.enter { ... Snapshot.sendApplyNotifications() // can be called anywhere, triggers the observer of `drawState` because we changed `drawState` during the previous `sendApplyNotifications` } snapshot.apply() // fails with "Unsupported concurrent change during composition" ``` (I haven't figured why sometimes and not always) Considering this, *we shouldn't change any state we read in `draw` inside `invalidate`* (because *we shouldn't change any state we read in `observeReads(block=)`) inside observeReads(onValueChangedForScope=)`*) We added this state not to trigger `RenderNodeLayer.drawLayer` after `RenderNodeLayer.invalidate`, but to trigger `SkiaGraphicsLayer.invalidate` after we called `RenderNodeLayer.drawLayer` inside `SkiaGraphicsLayer.draw`. Because: - we still need some way to notify `SkiaGraphicsLayer.invalidate` - only `RenderNodeLayer.invalidate` knows that `RenderNodeLayer.drawLayer` is changed We notify via `isDirty` state and a separate check outside `invalidate` <details> <summary>A synthetic reproducer of the issue for history (skip it if details are already understood)</summary> ``` import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.neverEqualPolicy import androidx.compose.runtime.snapshots.Snapshot import androidx.compose.runtime.snapshots.SnapshotStateObserver fun main() { val drawState = mutableStateOf(Unit, neverEqualPolicy()) val observer = SnapshotStateObserver { it.invoke() } observer.start() fun draw() { observer.observeReads(Unit, { println("invalidate draw (drawState.value = Unit)") drawState.value = Unit }) { println("\ndraw") drawState.value } } fun externalChange() { println("\nexternalChange") drawState.value = Unit Snapshot.sendApplyNotifications() } fun composition() { println("\ncomposition BEFORE") Snapshot.takeMutableSnapshot().apply { enter { println("composition BEGIN") println("sendApplyNotifications") Snapshot.sendApplyNotifications() println("composition END") } println("applying composition state to global state") if (!apply().succeeded) { throw RuntimeException("Composition apply failed") } } } // frame 0 draw() externalChange() // frame 1 composition() } ``` Output: ``` draw externalChange invalidate draw (drawState.value = Unit) composition BEFORE invalidate draw (drawState.value = Unit) composition BEGIN sendApplyNotifications invalidate draw (drawState.value = Unit) composition END applying composition state to global state Exception in thread "main" java.lang.RuntimeException: Composition apply failed at MainKt.main$composition(main.kt:39) at MainKt.main(main.kt:50) at MainKt.main(main.kt) ``` </details> ## Release Notes ### Fixes - Multiplatform - _(prerelease fix)_ Fix "Unsupported concurrent change during composition"
igordmn
added a commit
that referenced
this pull request
Sep 25, 2024
Fixes https://youtrack.jetbrains.com/issue/CMP-6729/iOS-Runtime-crash.-Unsupported-concurrent-change-during-composition The issue was because of this concurrent calls: ``` performRecompose() val snapshot = Snapshot.takeMutableSnapshot() Snapshot.sendApplyNotifications() // update the Global snapshot, called because we need to pass it fresh to the local Recompose snapshot drawState.value = Unit // called because some other state captured in `draw` was changed snapshot.enter { ... Snapshot.sendApplyNotifications() // it is valid to call it anywhere drawState.value = Unit // called because some other state state captured in `draw` was changed again } snapshot.apply() // fails with "Unsupported concurrent change during composition" because it can't merge to state changes ``` Regression after #1574 A synthetic reproducer of the issue for history (skip it if details are already understood): ``` import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.neverEqualPolicy import androidx.compose.runtime.snapshots.Snapshot import androidx.compose.runtime.snapshots.SnapshotStateObserver fun main() { val drawState = mutableStateOf(Unit, neverEqualPolicy()) val observer = SnapshotStateObserver { it.invoke() } observer.start() fun draw() { observer.observeReads(Unit, { println("invalidate draw (drawState.value = Unit)") drawState.value = Unit }) { println("\ndraw") drawState.value } } fun externalChange() { println("\nexternalChange") drawState.value = Unit Snapshot.sendApplyNotifications() } fun composition() { println("\ncomposition BEFORE") Snapshot.takeMutableSnapshot().apply { enter { println("composition BEGIN") println("sendApplyNotifications") Snapshot.sendApplyNotifications() println("composition END") } println("applying composition state to global state") if (!apply().succeeded) { throw RuntimeException("Composition apply failed") } } } // frame 0 draw() externalChange() // frame 1 composition() } ``` _Doesn't represent real case, as we capture and change `drawState` of the same layer. In a real case layer captures only children `drawState`. Still it shows similar chain of calls_ Output: ``` draw externalChange invalidate draw (drawState.value = Unit) composition BEFORE invalidate draw (drawState.value = Unit) composition BEGIN sendApplyNotifications invalidate draw (drawState.value = Unit) composition END applying composition state to global state Exception in thread "main" java.lang.RuntimeException: Composition apply failed at MainKt.main$composition(main.kt:39) at MainKt.main(main.kt:50) at MainKt.main(main.kt) ``` --------- Co-authored-by: Ivan Matkov <ivan.matkov@jetbrains.com>
igordmn
added a commit
that referenced
this pull request
Sep 25, 2024
Fixes https://youtrack.jetbrains.com/issue/CMP-6729/iOS-Runtime-crash.-Unsupported-concurrent-change-during-composition The issue was because of this concurrent calls: ``` performRecompose() val snapshot = Snapshot.takeMutableSnapshot() Snapshot.sendApplyNotifications() // update the Global snapshot, called because we need to pass it fresh to the local Recompose snapshot drawState.value = Unit // called because some other state captured in `draw` was changed snapshot.enter { ... Snapshot.sendApplyNotifications() // it is valid to call it anywhere drawState.value = Unit // called because some other state state captured in `draw` was changed again } snapshot.apply() // fails with "Unsupported concurrent change during composition" because it can't merge to state changes ``` Regression after #1574 A synthetic reproducer of the issue for history (skip it if details are already understood): ``` import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.neverEqualPolicy import androidx.compose.runtime.snapshots.Snapshot import androidx.compose.runtime.snapshots.SnapshotStateObserver fun main() { val drawState = mutableStateOf(Unit, neverEqualPolicy()) val observer = SnapshotStateObserver { it.invoke() } observer.start() fun draw() { observer.observeReads(Unit, { println("invalidate draw (drawState.value = Unit)") drawState.value = Unit }) { println("\ndraw") drawState.value } } fun externalChange() { println("\nexternalChange") drawState.value = Unit Snapshot.sendApplyNotifications() } fun composition() { println("\ncomposition BEFORE") Snapshot.takeMutableSnapshot().apply { enter { println("composition BEGIN") println("sendApplyNotifications") Snapshot.sendApplyNotifications() println("composition END") } println("applying composition state to global state") if (!apply().succeeded) { throw RuntimeException("Composition apply failed") } } } // frame 0 draw() externalChange() // frame 1 composition() } ``` _Doesn't represent real case, as we capture and change `drawState` of the same layer. In a real case layer captures only children `drawState`. Still it shows similar chain of calls_ Output: ``` draw externalChange invalidate draw (drawState.value = Unit) composition BEFORE invalidate draw (drawState.value = Unit) composition BEGIN sendApplyNotifications invalidate draw (drawState.value = Unit) composition END applying composition state to global state Exception in thread "main" java.lang.RuntimeException: Composition apply failed at MainKt.main$composition(main.kt:39) at MainKt.main(main.kt:50) at MainKt.main(main.kt) ``` --------- Co-authored-by: Ivan Matkov <ivan.matkov@jetbrains.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes https://youtrack.jetbrains.com/issue/CMP-6695
Testing
Test case from the issue.
This should be tested by QA
Release Notes
N/A (regression after #1555) wasn't released yet