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.

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

Kotlin optimized lock asserts #5604

Merged
merged 4 commits into from Nov 17, 2019
Merged

Kotlin optimized lock asserts #5604

merged 4 commits into from Nov 17, 2019

Conversation

@yschimke
Copy link
Collaborator

yschimke commented Nov 17, 2019

Use a clear intentional method instead of kotlin assert() function vs JVM assert keyword. Guarded behind a cached assertion status check.

Addressing issue #5586

yschimke added 3 commits Nov 17, 2019
@yschimke

This comment has been minimized.

Copy link
Collaborator Author

yschimke commented Nov 17, 2019

FWIW we could optimize the byte code by avoiding a pretty exception.

@Suppress("NOTHING_TO_INLINE")
internal inline fun Any.assertThreadHoldsLock() {
  if (AssertionsEnabled) {
    assert(!Thread.holdsLock(this))
  }
}

@Suppress("NOTHING_TO_INLINE")
internal inline fun Any.assertThreadDoesntHoldLock() {
  if (AssertionsEnabled) {
    assert(Thread.holdsLock(this))
  }
}
      int isRunning = false;
      boolean var4;
      boolean var5;
      boolean var6;
      if (Util.AssertionsEnabled) {
         boolean var3 = Thread.holdsLock(this);
         var4 = false;
         var5 = false;
         if (_Assertions.ENABLED && !var3) {
            var6 = false;
            String var14 = "Assertion failed";
            throw (Throwable)(new AssertionError(var14));
         }
      }
Copy link
Member

swankjesse left a comment

So Kotlin’s assert function makes no sense? It’s API works like this:

assert(expensiveComputation())

But it should have worked like this:

assert { expensiveComputation() }

https://kotlinlang.org/api/latest/jvm/stdlib/kotlin/assert.html

Ie. the benefit of the -ea flag isn’t to turn off failing, it’s to defer expensive computation.

@@ -527,3 +528,20 @@ fun <T> readFieldOrNull(instance: Any, fieldType: Class<T>, fieldName: String):
internal fun <E> MutableList<E>.addIfAbsent(element: E) {
if (!contains(element)) add(element)
}

@JvmField
internal val AssertionsEnabled = OkHttpClient::class.java.desiredAssertionStatus()

This comment has been minimized.

Copy link
@swankjesse

swankjesse Nov 17, 2019

Member

Is the casing of this wrong? Should be assertionsEnabled ?

@yschimke

This comment has been minimized.

Copy link
Collaborator Author

yschimke commented Nov 17, 2019

So Kotlin’s assert function makes no sense?

Yep - see here https://youtrack.jetbrains.com/issue/KT-22292

Worse callers now rely on this eager assert, so it needs a migration after it gets deprecated and fixed.

@yschimke yschimke merged commit 7a64153 into square:master Nov 17, 2019
4 checks passed
4 checks passed
ci/circleci: checkjdk11 Your tests passed on CircleCI!
Details
ci/circleci: testjdk11 Your tests passed on CircleCI!
Details
ci/circleci: testjdk13 Your tests passed on CircleCI!
Details
ci/circleci: testjdk8 Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.