From a5082e5d942627f0c72bc0df79088d94197a8ada Mon Sep 17 00:00:00 2001 From: Ilya Kirillov Date: Fri, 13 Jan 2023 16:22:57 +0100 Subject: [PATCH] [LL FIR] do not rethrow exceptions from caches Previously, the exceptions from ValueWithPostCompute was saved in cache and rethrown. This was needed to avoid multiple heavy calculation which will be still uncecessfull. Currently we do not have much of such exceptions and such thing may break perf test by throwing exceptions from old test invocation --- .../api/fir/caches/ValueWithPostCompute.kt | 32 ++------ .../fir/caches/ValueWithPostComputeTest.kt | 79 ------------------- 2 files changed, 5 insertions(+), 106 deletions(-) diff --git a/analysis/low-level-api-fir/src/org/jetbrains/kotlin/analysis/low/level/api/fir/caches/ValueWithPostCompute.kt b/analysis/low-level-api-fir/src/org/jetbrains/kotlin/analysis/low/level/api/fir/caches/ValueWithPostCompute.kt index 94708c69191..ccb36ac50cc 100644 --- a/analysis/low-level-api-fir/src/org/jetbrains/kotlin/analysis/low/level/api/fir/caches/ValueWithPostCompute.kt +++ b/analysis/low-level-api-fir/src/org/jetbrains/kotlin/analysis/low/level/api/fir/caches/ValueWithPostCompute.kt @@ -6,7 +6,6 @@ package org.jetbrains.kotlin.analysis.low.level.api.fir.fir.caches import org.jetbrains.kotlin.analysis.low.level.api.fir.util.lockWithPCECheck -import org.jetbrains.kotlin.util.shouldIjPlatformExceptionBeRethrown import java.util.concurrent.locks.ReentrantLock /** @@ -36,7 +35,6 @@ internal class ValueWithPostCompute( /** * can be in one of the following three states: * [ValueIsNotComputed] -- value is not initialized and thread are now executing [_postCompute] - * [ExceptionWasThrownDuringValueComputation] -- exception was thrown during value computation, it will be rethrown on every value access * [ValueIsPostComputingNow] -- thread with threadId has computed the value and only it can access it during post compute * some value of type [VALUE] -- value is computed and post compute was executed, values is visible for all threads * @@ -61,16 +59,13 @@ internal class ValueWithPostCompute( return stateSnapshot.value as VALUE } else { lock?.lockWithPCECheck(LOCKING_INTERVAL_MS) { // wait until other thread which holds the lock now computes the value - when (val newStateSnapshot = value) { + when (value) { ValueIsNotComputed -> { // if we have a PCE during value computation, then we will enter the critical section with `value == ValueIsNotComputed` // in this case, we should try to recalculate the value return computeValueWithoutLock() } - is ExceptionWasThrownDuringValueComputation -> { - // if some other thread tried to compute the value but failed with the exception - throw newStateSnapshot.error - } + else -> { // other thread computed the value for us return value as VALUE @@ -83,9 +78,6 @@ internal class ValueWithPostCompute( return computeValueWithoutLock() } ?: return value as VALUE - is ExceptionWasThrownDuringValueComputation -> { - throw stateSnapshot.error - } else -> { return stateSnapshot as VALUE } @@ -98,15 +90,11 @@ internal class ValueWithPostCompute( // if we entered synchronized section that's mean that the value is not yet calculated and was not started to be calculated // or the some other thread calculated the value while we were waiting to acquire the lock - when (val newStateSnapshot = value) { + when (value) { ValueIsNotComputed -> { // will be computed later, the read of `ValueIsNotComputed` guarantees that lock is not null require(lock!!.isHeldByCurrentThread) } - is ExceptionWasThrownDuringValueComputation -> { - // if some other thread tried to compute the value but failed with the exception - throw newStateSnapshot.error - } else -> { // other thread computed the value for us and set `lock` to null require(lock == null) @@ -122,11 +110,7 @@ internal class ValueWithPostCompute( _postCompute!!(key, calculated, data) calculated } catch (e: Throwable) { - if (e is Exception && exceptionShouldBeSavedInCache(e)) { - value = ExceptionWasThrownDuringValueComputation(e) - } else { - value = ValueIsNotComputed - } + value = ValueIsNotComputed throw e } // reading lock = null implies that the value is calculated and stored @@ -138,20 +122,14 @@ internal class ValueWithPostCompute( return calculatedValue } - private fun exceptionShouldBeSavedInCache(exception: Exception): Boolean = - !shouldIjPlatformExceptionBeRethrown(exception) - - @Suppress("UNCHECKED_CAST") - fun getValueIfComputed(): VALUE? = when (val snapshot = value) { + fun getValueIfComputed(): VALUE? = when (value) { ValueIsNotComputed -> null is ValueIsPostComputingNow -> null - is ExceptionWasThrownDuringValueComputation -> throw snapshot.error else -> value as VALUE } private class ValueIsPostComputingNow(val value: Any?, val threadId: Long) - private class ExceptionWasThrownDuringValueComputation(val error: Throwable) private object ValueIsNotComputed } diff --git a/analysis/low-level-api-fir/tests/org/jetbrains/kotlin/analysis/low/level/api/fir/caches/ValueWithPostComputeTest.kt b/analysis/low-level-api-fir/tests/org/jetbrains/kotlin/analysis/low/level/api/fir/caches/ValueWithPostComputeTest.kt index 9801549aed9..dc757d2ba9a 100644 --- a/analysis/low-level-api-fir/tests/org/jetbrains/kotlin/analysis/low/level/api/fir/caches/ValueWithPostComputeTest.kt +++ b/analysis/low-level-api-fir/tests/org/jetbrains/kotlin/analysis/low/level/api/fir/caches/ValueWithPostComputeTest.kt @@ -69,25 +69,6 @@ class ValueWithPostComputeTest { Assertions.assertNotEquals(pceOnFirstAccess, pceOnSecondAccess, "different PCE should be thrown on every access") } - @Test - fun testNonRecoverableExceptionISavedInCache() { - val valueWithPostCompute = ValueWithPostCompute( - key = 1, - calculate = { "value" to Unit }, - postCompute = { _, _, _ -> - throw IllegalStateException() - } - ) - - val exceptionOnFirstAccess = kotlin.runCatching { valueWithPostCompute.getValue() }.exceptionOrNull() - Assertions.assertInstanceOf(IllegalStateException::class.java, exceptionOnFirstAccess) - - val exceptionOnSecondAccess = kotlin.runCatching { valueWithPostCompute.getValue() }.exceptionOrNull() - Assertions.assertInstanceOf(IllegalStateException::class.java, exceptionOnSecondAccess) - - Assertions.assertEquals(exceptionOnFirstAccess, exceptionOnSecondAccess, "the same exception should be thrown on every access") - } - /** * Tests the following scenario: * - thread `t1` access the cache and executes `calculate()` and then `postCompute()` under a lock hold @@ -145,64 +126,4 @@ class ValueWithPostComputeTest { } } } - - /** - * Tests the following scenario: - * - thread `t1` access the cache and executes `calculate()` and then `postCompute()` under a lock hold - * - while the lock hold by `t1`, `t2` tries to also access the value and waits for the lock to be released by `t1` - * - t1: during the post compute, some non-recoverable exception happens inside the `postCompute()` and exception is saved in the cache - * - t1 releases the lock with the `value` set to `ExceptionWasThrownDuringValueComputation` - * - t2 acquires the lock and should correctly see `ExceptionWasThrownDuringValueComputation` and rethrow the exception - */ - @Test - fun testSavedExceptionInCacheFromPostCompute() { - class ExceptionWhichWillBeSavedInCache : Exception() - for (i in 1..100) { - val t1CalledCalculate = CountDownLatch(1) - val t2AccessedTheCache = CountDownLatch(1) - - val resultRef = AtomicReference(null) - - val valueWithPostCompute = ValueWithPostCompute( - key = 1, - calculate = { - if (Thread.currentThread().name == "t1") { - t1CalledCalculate.countDown() - } - Thread.currentThread().name to Unit - }, - postCompute = { _, _, _ -> - t2AccessedTheCache.await() - if (Thread.currentThread().name == "t1") { - throw ExceptionWhichWillBeSavedInCache() - } - } - ) - - val t1 = thread(name = "t1") { - try { - valueWithPostCompute.getValue() - } catch (_: Throwable) { - } - } - - val t2 = thread(name = "t2") { - t1CalledCalculate.await() - t2AccessedTheCache.countDown() - - try { - resultRef.set(valueWithPostCompute.getValue()) - } catch (e: Throwable) { - resultRef.set(e) - } - } - t2.join() - t1.join() - when (val result = resultRef.get()) { - is ExceptionWhichWillBeSavedInCache -> {} - is Throwable -> throw result - else -> Assertions.fail("Unexpected value $result") - } - } - } } \ No newline at end of file