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.

Sign up for GitHub

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

Jump to bottom

Investigate limiting System.identityHashcode() to positive numbers #3062

Closed
DanHeidinga opened this issue Sep 28, 2018 · 10 comments
Closed

Investigate limiting System.identityHashcode() to positive numbers #3062

DanHeidinga opened this issue Sep 28, 2018 · 10 comments
Assignees
@gacholio
Labels
comp:vm doc:externals
Milestone
Release 0.11.0 (J...

Comments

@DanHeidinga
Copy link
Member

OpenJ9 allows both positive and negative identity hashcodes. This is problematic for programs that incorrectly assume hashcodes can only be positive.

Investigate adding an option to limit hashcode to be positive and determine the performance overhead of this.

Note, any such option would limit the range of hashcodes and may lead to more collisions.
@DanHeidinga DanHeidinga added this to the Release 0.11.0 milestone Sep 28, 2018
@charliegracie
Copy link
Contributor

charliegracie commented Sep 28, 2018 •
edited
Loading

Are there open/closed issues where consumers of OpenJ9 were having issues related to incorrectly assuming hashcodes were positive?

@DanHeidinga
Copy link
Member Author

Yes, there are libraries (Hessian?) that assume that the hashcode is positive and use it to index into arrays. Some adopters have run into blocking issues as their stack depends on libraries that have this assumption.

Providing this as an option eases the migration path.

@DanHeidinga
Copy link
Member Author

The likely place to hack for this is https://github.com/eclipse/openj9/blob/2f295ea91cdd97ff37eb5d164fc63014ea718bc7/runtime/oti/ObjectHash.hpp#L177

Changing it here will ensure the option + mask is only rechecked until the object is moved. All the fast paths that read the hashcode from moved objects shouldn't require any modification.

Will attempt to add an -XX option and investigate the performance implications of this change.

@vijaysun-omr
Copy link
Contributor

vijaysun-omr commented Sep 29, 2018 •
edited
Loading

Maybe if the result of the -XX option is stored in a private static final boolean variable and if that variable is checked in the identityHashCode at the spot @DanHeidinga pointed out, then the JIT would be able to fold away the extra test at compile time via it's usual static final optimizations.

@vijaysun-omr
Copy link
Contributor

I assume you were thinking of an -XX option to work around this particular migration issue in terms of different JCL behavior. One question is whether you prefer different -XX options for different JCL issues such as this one (assuming there are more of them we will find down the line) or have one option that changes all the JCL behaviours needed to make migration easier.

@DanHeidinga
Copy link
Member Author

@vijaysun-omr I've looked for how to push the option as low in the code as possible - down to the native hash operation - to ensure there is only 1 place that needs to be modified and that there's never a chance of an inconsistent hashcode being returned.

One question is whether you prefer different -XX options for different JCL issues such as this one (assuming there are more of them we will find down the line) or have one option that changes all the JCL behaviours needed to make migration easier.

That's an interesting idea. I think we should start with a single option. If we end up with multiple migration related options, they can be grouped later under a single -XX:EasyMigration (or similar) option.

@vijaysun-omr
Copy link
Contributor

Thanks @DanHeidinga I am in agreement with your approach to wait for the "second" migration related option before considering grouping under one option.

Is there a PR for the change to the native hash ?

@SueChaplain
Copy link
Contributor

When you are ready, please open an issue at the OpenJ9-docs repo using this template: https://github.com/eclipse/openj9-docs/issues/new?template=new-documentation-change.md

@gacholio gacholio self-assigned this Oct 9, 2018
gacholio added a commit to gacholio/openj9 that referenced this issue Oct 9, 2018
@gacholio
Add option to limit identity hash code to positive values
44faec9 
Add command line option "-XX:[+|-]PositiveIdentityHash" to
enable/disable forcing identity hash code to be a positive value.

Fixes: eclipse-openj9#3062

Signed-off-by: Graham Chapman <[email protected]>
@gacholio gacholio mentioned this issue Oct 9, 2018
Merged
@DanHeidinga
Copy link
Member Author

Thanks @gacholio for coding this up. For those following along, the next step is to check a microbench for any regressions and then we'll proceed with the merge provided the results are ok

@gacholio gacholio mentioned this issue Oct 10, 2018
Closed
@gacholio
Copy link
Contributor

Doc issue: eclipse-openj9/openj9-docs#109

@keithc-ca keithc-ca mentioned this issue Feb 15, 2019
Closed
igordmn added a commit to JetBrains/compose-multiplatform-core that referenced this issue Nov 16, 2021
@igordmn
Fix overflow in Identity collections (on Eclipse OpenJ9)
d72aee2 
Fixes JetBrains/compose-multiplatform#991

Eclipse OpenJ9 can return negative hashcodes (at least 15.0.2):
-1366493741
192407236

when we compare them, we can catch an overflow:
`-1366493741 - 1924072363` will be more than 0

see eclipse-openj9/openj9#3062

That leads to wrong behaviour of IdentityArraySet, and so of Recomposer:
```
alreadyComposed.add(composition)
println(alreadyComposed.contains(composition)) // will print `false`
```

The crash:
```
Caused by: java.lang.IllegalStateException: pending composition has not been applied
	at androidx.compose.runtime.CompositionImpl.drainPendingModificationsForCompositionLocked(Composition.kt:448)
	at androidx.compose.runtime.CompositionImpl.recompose(Composition.kt:635)
	at androidx.compose.runtime.Recomposer.performRecompose(Recomposer.kt:793)
	at androidx.compose.runtime.Recomposer.access$performRecompose(Recomposer.kt:106)
	at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2$2.invoke(Recomposer.kt:460)
	at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2$2.invoke(Recomposer.kt:426)
	at androidx.compose.desktop.examples.example1.TestClock.withFrameNanos(Main.jvm.kt:85)
	at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2.invokeSuspend(Recomposer.kt:426)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630)
	at java.base/java.lang.Thread.run(Thread.java:853)
```

The reproducer:
```
import androidx.compose.runtime.AbstractApplier
import androidx.compose.runtime.Composable
import androidx.compose.runtime.Composition
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.MonotonicFrameClock
import androidx.compose.runtime.currentRecomposeScope
import androidx.compose.runtime.rememberCompositionContext
import androidx.compose.runtime.rememberUpdatedState
import androidx.compose.runtime.withRunningRecomposer
import kotlinx.coroutines.asCoroutineDispatcher
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.yield
import java.util.concurrent.Executors.newSingleThreadExecutor
import kotlin.random.Random

fun main() {
    runBlocking(newSingleThreadExecutor().asCoroutineDispatcher() + TestClock) {
        repeat(300) { num ->
            println("test $num")

            withRunningRecomposer {
                val composition = Composition(TestApplier, it)
                composition.setContent {
                    val scope = currentRecomposeScope
                    Test(Random.nextInt())
                    Test(Random.nextInt())
                    LaunchedEffect(Unit) {
                        scope.invalidate()
                    }
                }
                yield()
                yield()
            }
        }
    }
}

@composable
fun Test(test: Int) {
    val latestContent = rememberUpdatedState(test)
    latestContent.value

    val parent = rememberCompositionContext()
    LaunchedEffect(Unit) {
        val composition1 = Composition(TestApplier, parent)
        composition1.setContent {
            latestContent.value
        }
        val composition2 = Composition(TestApplier, parent)
        composition2.setContent {
            latestContent.value
        }
    }
}

internal object TestApplier: AbstractApplier<Unit>(Unit) {
    override fun insertTopDown(index: Int, instance: Unit) = Unit
    override fun insertBottomUp(index: Int, instance: Unit) = Unit
    override fun remove(index: Int, count: Int) = Unit
    override fun move(from: Int, to: Int, count: Int) = Unit
    override fun onClear() = Unit
}

object TestClock : MonotonicFrameClock {
    override suspend fun <R> withFrameNanos(onFrame: (frameTimeNanos: Long) -> R): R {
        return onFrame(System.nanoTime())
    }
}
```

java --version:
```
openjdk 15.0.2 2021-01-19
OpenJDK Runtime Environment AdoptOpenJDK (build 15.0.2+7)
Eclipse OpenJ9 VM AdoptOpenJDK (build openj9-0.24.0, JRE 15 Linux amd64-64-Bit Compressed References 20210121_172 (JIT enabled, AOT enabled)
OpenJ9   - 345e1b09e
OMR      - 741e94ea8
JCL      - 863b523566 based on jdk-15.0.2+7)
```
OS: Ubuntu 20.0.4

Test: manual (see the reproducer)
Test: ./gradlew :compose:runtime:runtime:test (without the fix we have failing Identity*Test on this JDK)
@igordmn igordmn mentioned this issue Nov 16, 2021
Merged
igordmn added a commit to JetBrains/compose-multiplatform-core that referenced this issue Jan 14, 2022
@igordmn
Fix overflow in Identity collections (on Eclipse OpenJ9)
d9d9efa 
Fixes JetBrains/compose-multiplatform#991

Eclipse OpenJ9 can return negative hashcodes (at least 15.0.2):
-1366493741
192407236

when we compare them, we can catch an overflow:
`-1366493741 - 1924072363` will be more than 0

see eclipse-openj9/openj9#3062

That leads to wrong behaviour of IdentityArraySet, and so of Recomposer:
```
alreadyComposed.add(composition)
println(alreadyComposed.contains(composition)) // will print `false`
```

The crash:
```
Caused by: java.lang.IllegalStateException: pending composition has not been applied
	at androidx.compose.runtime.CompositionImpl.drainPendingModificationsForCompositionLocked(Composition.kt:448)
	at androidx.compose.runtime.CompositionImpl.recompose(Composition.kt:635)
	at androidx.compose.runtime.Recomposer.performRecompose(Recomposer.kt:793)
	at androidx.compose.runtime.Recomposer.access$performRecompose(Recomposer.kt:106)
	at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2$2.invoke(Recomposer.kt:460)
	at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2$2.invoke(Recomposer.kt:426)
	at androidx.compose.desktop.examples.example1.TestClock.withFrameNanos(Main.jvm.kt:85)
	at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2.invokeSuspend(Recomposer.kt:426)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630)
	at java.base/java.lang.Thread.run(Thread.java:853)
```

The reproducer:
```
import androidx.compose.runtime.AbstractApplier
import androidx.compose.runtime.Composable
import androidx.compose.runtime.Composition
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.MonotonicFrameClock
import androidx.compose.runtime.currentRecomposeScope
import androidx.compose.runtime.rememberCompositionContext
import androidx.compose.runtime.rememberUpdatedState
import androidx.compose.runtime.withRunningRecomposer
import kotlinx.coroutines.asCoroutineDispatcher
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.yield
import java.util.concurrent.Executors.newSingleThreadExecutor
import kotlin.random.Random

fun main() {
    runBlocking(newSingleThreadExecutor().asCoroutineDispatcher() + TestClock) {
        repeat(300) { num ->
            println("test $num")

            withRunningRecomposer {
                val composition = Composition(TestApplier, it)
                composition.setContent {
                    val scope = currentRecomposeScope
                    Test(Random.nextInt())
                    Test(Random.nextInt())
                    LaunchedEffect(Unit) {
                        scope.invalidate()
                    }
                }
                yield()
                yield()
            }
        }
    }
}

@composable
fun Test(test: Int) {
    val latestContent = rememberUpdatedState(test)
    latestContent.value

    val parent = rememberCompositionContext()
    LaunchedEffect(Unit) {
        val composition1 = Composition(TestApplier, parent)
        composition1.setContent {
            latestContent.value
        }
        val composition2 = Composition(TestApplier, parent)
        composition2.setContent {
            latestContent.value
        }
    }
}

internal object TestApplier: AbstractApplier<Unit>(Unit) {
    override fun insertTopDown(index: Int, instance: Unit) = Unit
    override fun insertBottomUp(index: Int, instance: Unit) = Unit
    override fun remove(index: Int, count: Int) = Unit
    override fun move(from: Int, to: Int, count: Int) = Unit
    override fun onClear() = Unit
}

object TestClock : MonotonicFrameClock {
    override suspend fun <R> withFrameNanos(onFrame: (frameTimeNanos: Long) -> R): R {
        return onFrame(System.nanoTime())
    }
}
```

java --version:
```
openjdk 15.0.2 2021-01-19
OpenJDK Runtime Environment AdoptOpenJDK (build 15.0.2+7)
Eclipse OpenJ9 VM AdoptOpenJDK (build openj9-0.24.0, JRE 15 Linux amd64-64-Bit Compressed References 20210121_172 (JIT enabled, AOT enabled)
OpenJ9   - 345e1b09e
OMR      - 741e94ea8
JCL      - 863b523566 based on jdk-15.0.2+7)
```
OS: Ubuntu 20.0.4

Test: manual (see the reproducer)
Test: ./gradlew :compose:runtime:runtime:test (without the fix we have failing Identity*Test on this JDK)
igordmn added a commit to JetBrains/compose-multiplatform-core that referenced this issue Jan 18, 2022
@igordmn
Fix overflow in Identity collections (on Eclipse OpenJ9)
ed0a979 
Fixes JetBrains/compose-multiplatform#991

Eclipse OpenJ9 can return negative hashcodes (at least 15.0.2):
-1366493741
192407236

when we compare them, we can catch an overflow:
`-1366493741 - 1924072363` will be more than 0

see eclipse-openj9/openj9#3062

That leads to wrong behaviour of IdentityArraySet, and so of Recomposer:
```
alreadyComposed.add(composition)
println(alreadyComposed.contains(composition)) // will print `false`
```

The crash:
```
Caused by: java.lang.IllegalStateException: pending composition has not been applied
	at androidx.compose.runtime.CompositionImpl.drainPendingModificationsForCompositionLocked(Composition.kt:448)
	at androidx.compose.runtime.CompositionImpl.recompose(Composition.kt:635)
	at androidx.compose.runtime.Recomposer.performRecompose(Recomposer.kt:793)
	at androidx.compose.runtime.Recomposer.access$performRecompose(Recomposer.kt:106)
	at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2$2.invoke(Recomposer.kt:460)
	at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2$2.invoke(Recomposer.kt:426)
	at androidx.compose.desktop.examples.example1.TestClock.withFrameNanos(Main.jvm.kt:85)
	at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2.invokeSuspend(Recomposer.kt:426)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630)
	at java.base/java.lang.Thread.run(Thread.java:853)
```

The reproducer:
```
import androidx.compose.runtime.AbstractApplier
import androidx.compose.runtime.Composable
import androidx.compose.runtime.Composition
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.MonotonicFrameClock
import androidx.compose.runtime.currentRecomposeScope
import androidx.compose.runtime.rememberCompositionContext
import androidx.compose.runtime.rememberUpdatedState
import androidx.compose.runtime.withRunningRecomposer
import kotlinx.coroutines.asCoroutineDispatcher
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.yield
import java.util.concurrent.Executors.newSingleThreadExecutor
import kotlin.random.Random

fun main() {
    runBlocking(newSingleThreadExecutor().asCoroutineDispatcher() + TestClock) {
        repeat(300) { num ->
            println("test $num")

            withRunningRecomposer {
                val composition = Composition(TestApplier, it)
                composition.setContent {
                    val scope = currentRecomposeScope
                    Test(Random.nextInt())
                    Test(Random.nextInt())
                    LaunchedEffect(Unit) {
                        scope.invalidate()
                    }
                }
                yield()
                yield()
            }
        }
    }
}

@composable
fun Test(test: Int) {
    val latestContent = rememberUpdatedState(test)
    latestContent.value

    val parent = rememberCompositionContext()
    LaunchedEffect(Unit) {
        val composition1 = Composition(TestApplier, parent)
        composition1.setContent {
            latestContent.value
        }
        val composition2 = Composition(TestApplier, parent)
        composition2.setContent {
            latestContent.value
        }
    }
}

internal object TestApplier: AbstractApplier<Unit>(Unit) {
    override fun insertTopDown(index: Int, instance: Unit) = Unit
    override fun insertBottomUp(index: Int, instance: Unit) = Unit
    override fun remove(index: Int, count: Int) = Unit
    override fun move(from: Int, to: Int, count: Int) = Unit
    override fun onClear() = Unit
}

object TestClock : MonotonicFrameClock {
    override suspend fun <R> withFrameNanos(onFrame: (frameTimeNanos: Long) -> R): R {
        return onFrame(System.nanoTime())
    }
}
```

java --version:
```
openjdk 15.0.2 2021-01-19
OpenJDK Runtime Environment AdoptOpenJDK (build 15.0.2+7)
Eclipse OpenJ9 VM AdoptOpenJDK (build openj9-0.24.0, JRE 15 Linux amd64-64-Bit Compressed References 20210121_172 (JIT enabled, AOT enabled)
OpenJ9   - 345e1b09e
OMR      - 741e94ea8
JCL      - 863b523566 based on jdk-15.0.2+7)
```
OS: Ubuntu 20.0.4

Test: manual (see the reproducer)
Test: ./gradlew :compose:runtime:runtime:test (without the fix we have failing Identity*Test on this JDK)
igordmn added a commit to JetBrains/compose-multiplatform-core that referenced this issue Jan 20, 2022
@igordmn
Fix overflow in Identity collections (on Eclipse OpenJ9)
460f49c 
Fixes JetBrains/compose-multiplatform#991

Eclipse OpenJ9 can return negative hashcodes (at least 15.0.2):
-1366493741
192407236

when we compare them, we can catch an overflow:
`-1366493741 - 1924072363` will be more than 0

see eclipse-openj9/openj9#3062

That leads to wrong behaviour of IdentityArraySet, and so of Recomposer:
```
alreadyComposed.add(composition)
println(alreadyComposed.contains(composition)) // will print `false`
```

The crash:
```
Caused by: java.lang.IllegalStateException: pending composition has not been applied
	at androidx.compose.runtime.CompositionImpl.drainPendingModificationsForCompositionLocked(Composition.kt:448)
	at androidx.compose.runtime.CompositionImpl.recompose(Composition.kt:635)
	at androidx.compose.runtime.Recomposer.performRecompose(Recomposer.kt:793)
	at androidx.compose.runtime.Recomposer.access$performRecompose(Recomposer.kt:106)
	at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2$2.invoke(Recomposer.kt:460)
	at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2$2.invoke(Recomposer.kt:426)
	at androidx.compose.desktop.examples.example1.TestClock.withFrameNanos(Main.jvm.kt:85)
	at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2.invokeSuspend(Recomposer.kt:426)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630)
	at java.base/java.lang.Thread.run(Thread.java:853)
```

The reproducer:
```
import androidx.compose.runtime.AbstractApplier
import androidx.compose.runtime.Composable
import androidx.compose.runtime.Composition
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.MonotonicFrameClock
import androidx.compose.runtime.currentRecomposeScope
import androidx.compose.runtime.rememberCompositionContext
import androidx.compose.runtime.rememberUpdatedState
import androidx.compose.runtime.withRunningRecomposer
import kotlinx.coroutines.asCoroutineDispatcher
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.yield
import java.util.concurrent.Executors.newSingleThreadExecutor
import kotlin.random.Random

fun main() {
    runBlocking(newSingleThreadExecutor().asCoroutineDispatcher() + TestClock) {
        repeat(300) { num ->
            println("test $num")

            withRunningRecomposer {
                val composition = Composition(TestApplier, it)
                composition.setContent {
                    val scope = currentRecomposeScope
                    Test(Random.nextInt())
                    Test(Random.nextInt())
                    LaunchedEffect(Unit) {
                        scope.invalidate()
                    }
                }
                yield()
                yield()
            }
        }
    }
}

@composable
fun Test(test: Int) {
    val latestContent = rememberUpdatedState(test)
    latestContent.value

    val parent = rememberCompositionContext()
    LaunchedEffect(Unit) {
        val composition1 = Composition(TestApplier, parent)
        composition1.setContent {
            latestContent.value
        }
        val composition2 = Composition(TestApplier, parent)
        composition2.setContent {
            latestContent.value
        }
    }
}

internal object TestApplier: AbstractApplier<Unit>(Unit) {
    override fun insertTopDown(index: Int, instance: Unit) = Unit
    override fun insertBottomUp(index: Int, instance: Unit) = Unit
    override fun remove(index: Int, count: Int) = Unit
    override fun move(from: Int, to: Int, count: Int) = Unit
    override fun onClear() = Unit
}

object TestClock : MonotonicFrameClock {
    override suspend fun <R> withFrameNanos(onFrame: (frameTimeNanos: Long) -> R): R {
        return onFrame(System.nanoTime())
    }
}
```

java --version:
```
openjdk 15.0.2 2021-01-19
OpenJDK Runtime Environment AdoptOpenJDK (build 15.0.2+7)
Eclipse OpenJ9 VM AdoptOpenJDK (build openj9-0.24.0, JRE 15 Linux amd64-64-Bit Compressed References 20210121_172 (JIT enabled, AOT enabled)
OpenJ9   - 345e1b09e
OMR      - 741e94ea8
JCL      - 863b523566 based on jdk-15.0.2+7)
```
OS: Ubuntu 20.0.4

Test: manual (see the reproducer)
Test: ./gradlew :compose:runtime:runtime:test (without the fix we have failing Identity*Test on this JDK)
igordmn added a commit to JetBrains/compose-multiplatform-core that referenced this issue Jan 20, 2022
@igordmn
Fix overflow in Identity collections (on Eclipse OpenJ9)
f72e62a 
Fixes JetBrains/compose-multiplatform#991

Eclipse OpenJ9 can return negative hashcodes (at least 15.0.2):
-1366493741
192407236

when we compare them, we can catch an overflow:
`-1366493741 - 1924072363` will be more than 0

see eclipse-openj9/openj9#3062

That leads to wrong behaviour of IdentityArraySet, and so of Recomposer:
```
alreadyComposed.add(composition)
println(alreadyComposed.contains(composition)) // will print `false`
```

The crash:
```
Caused by: java.lang.IllegalStateException: pending composition has not been applied
	at androidx.compose.runtime.CompositionImpl.drainPendingModificationsForCompositionLocked(Composition.kt:448)
	at androidx.compose.runtime.CompositionImpl.recompose(Composition.kt:635)
	at androidx.compose.runtime.Recomposer.performRecompose(Recomposer.kt:793)
	at androidx.compose.runtime.Recomposer.access$performRecompose(Recomposer.kt:106)
	at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2$2.invoke(Recomposer.kt:460)
	at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2$2.invoke(Recomposer.kt:426)
	at androidx.compose.desktop.examples.example1.TestClock.withFrameNanos(Main.jvm.kt:85)
	at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2.invokeSuspend(Recomposer.kt:426)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630)
	at java.base/java.lang.Thread.run(Thread.java:853)
```

The reproducer:
```
import androidx.compose.runtime.AbstractApplier
import androidx.compose.runtime.Composable
import androidx.compose.runtime.Composition
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.MonotonicFrameClock
import androidx.compose.runtime.currentRecomposeScope
import androidx.compose.runtime.rememberCompositionContext
import androidx.compose.runtime.rememberUpdatedState
import androidx.compose.runtime.withRunningRecomposer
import kotlinx.coroutines.asCoroutineDispatcher
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.yield
import java.util.concurrent.Executors.newSingleThreadExecutor
import kotlin.random.Random

fun main() {
    runBlocking(newSingleThreadExecutor().asCoroutineDispatcher() + TestClock) {
        repeat(300) { num ->
            println("test $num")

            withRunningRecomposer {
                val composition = Composition(TestApplier, it)
                composition.setContent {
                    val scope = currentRecomposeScope
                    Test(Random.nextInt())
                    Test(Random.nextInt())
                    LaunchedEffect(Unit) {
                        scope.invalidate()
                    }
                }
                yield()
                yield()
            }
        }
    }
}

@composable
fun Test(test: Int) {
    val latestContent = rememberUpdatedState(test)
    latestContent.value

    val parent = rememberCompositionContext()
    LaunchedEffect(Unit) {
        val composition1 = Composition(TestApplier, parent)
        composition1.setContent {
            latestContent.value
        }
        val composition2 = Composition(TestApplier, parent)
        composition2.setContent {
            latestContent.value
        }
    }
}

internal object TestApplier: AbstractApplier<Unit>(Unit) {
    override fun insertTopDown(index: Int, instance: Unit) = Unit
    override fun insertBottomUp(index: Int, instance: Unit) = Unit
    override fun remove(index: Int, count: Int) = Unit
    override fun move(from: Int, to: Int, count: Int) = Unit
    override fun onClear() = Unit
}

object TestClock : MonotonicFrameClock {
    override suspend fun <R> withFrameNanos(onFrame: (frameTimeNanos: Long) -> R): R {
        return onFrame(System.nanoTime())
    }
}
```

java --version:
```
openjdk 15.0.2 2021-01-19
OpenJDK Runtime Environment AdoptOpenJDK (build 15.0.2+7)
Eclipse OpenJ9 VM AdoptOpenJDK (build openj9-0.24.0, JRE 15 Linux amd64-64-Bit Compressed References 20210121_172 (JIT enabled, AOT enabled)
OpenJ9   - 345e1b09e
OMR      - 741e94ea8
JCL      - 863b523566 based on jdk-15.0.2+7)
```
OS: Ubuntu 20.0.4

Test: manual (see the reproducer)
Test: ./gradlew :compose:runtime:runtime:test (without the fix we have failing Identity*Test on this JDK)
@igordmn igordmn mentioned this issue Feb 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gacholio gacholio

Labels
comp:vm doc:externals
Projects
None yet
Milestone
Release 0.11.0 (Java 8, 11 Oct refresh)
Development

No branches or pull requests
6 participants
@DanHeidinga @charliegracie @gacholio @pshipton @SueChaplain @vijaysun-omr