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

=lib Make use of CompletionStage#handle instead of whenComplete #10181

Merged
merged 1 commit into from Nov 25, 2022

Conversation

He-Pin
Copy link
Contributor

@He-Pin He-Pin commented Oct 8, 2022

Which has better performance.

@lrytz Would you like to give some input, thanks.

Another one was :
line/armeria#1440
line/armeria#1444
akka/akka#31283

I think this will help especially when the CompletionStage was failed already.

@scala-jenkins scala-jenkins added this to the 2.13.11 milestone Oct 8, 2022
@He-Pin He-Pin closed this Oct 8, 2022
@He-Pin He-Pin reopened this Oct 8, 2022
@He-Pin He-Pin force-pushed the 2.13.x branch 2 times, most recently from 827edfb to fbbc998 Compare October 8, 2022 09:21
@SethTisue
Copy link
Member

@viktorklang ?

@He-Pin
Copy link
Contributor Author

He-Pin commented Oct 10, 2022

image
image

jmh:run -i 5 -wi 5 -f1 -t1 .CompletionStageBenchmark.

package bench

import org.openjdk.jmh.annotations._

import java.util.concurrent.CompletableFuture
import java.util.concurrent.CountDownLatch
import java.util.concurrent.TimeUnit

@State(Scope.Benchmark)
@OutputTimeUnit(TimeUnit.SECONDS)
@BenchmarkMode(Array(Mode.Throughput))
class CompletionStageBenchmark {
  val ex = new RuntimeException("ex")

  @Benchmark
  def benchWhenCompleteFailedSync(): Unit = {
    val future = new CompletableFuture[Int]()
    future.completeExceptionally(ex)
    val latch = new CountDownLatch(1)
    future.whenComplete((_, _) => latch.countDown())
    latch.await()
  }

  @Benchmark
  def benchWhenCompleteSuccessSync(): Unit = {
    val future = new CompletableFuture[Int]()
    future.complete(1)
    val latch = new CountDownLatch(1)
    future.whenComplete((_, _) => latch.countDown())
    latch.await()
  }

  @Benchmark
  def benchHandleFailedSync(): Unit = {
    val future = new CompletableFuture[Int]()
    future.completeExceptionally(ex)
    val latch = new CountDownLatch(1)
    future.handle((_, _) => latch.countDown())
    latch.await()
  }

  @Benchmark
  def benchHandleSuccessSync(): Unit = {
    val future = new CompletableFuture[Int]()
    future.complete(1)
    val latch = new CountDownLatch(1)
    future.handle((_, _) => latch.countDown())
    latch.await()
  }

}

@He-Pin
Copy link
Contributor Author

He-Pin commented Oct 11, 2022

I submitted the same to kt too: Kotlin/kotlinx.coroutines#3475

@He-Pin
Copy link
Contributor Author

He-Pin commented Oct 13, 2022

image

@He-Pin
Copy link
Contributor Author

He-Pin commented Oct 15, 2022

refs: e3d202e too

src/library/scala/jdk/javaapi/FutureConverters.scala Outdated Show resolved Hide resolved
if (future.isDone && !future.isCompletedExceptionally) {
p.tryComplete(Success(future.join()))
} else {
cs.handle(p)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why handle is faster than whenComplete? Is the performance change significant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expecially on the failed case, I think the main outcome is from avoiding the encodeThrowable

@He-Pin He-Pin requested a review from lrytz November 8, 2022 07:27
@SethTisue SethTisue added library:concurrent Changes to the concurrency support in stdlib java interop and removed library:concurrent Changes to the concurrency support in stdlib labels Nov 10, 2022
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @He-Pin

@lrytz lrytz merged commit 4cbc73f into scala:2.13.x Nov 25, 2022
@@ -70,9 +70,13 @@ object FutureConverters {
case cf: CF[T] => cf.wrapped
// in theory not safe (could be `class C extends Future[A] with CompletionStage[B]`):
case f: Future[T @unchecked] => f
case cf: CompletableFuture[T @unchecked] if cf.isDone && !cf.isCompletedExceptionally =>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI @He-Pin
JDK 11 has MinimalStage which is a CompletableFuture (also CompletionStage), with UnsupportedOperationException in lot of method bodies (like isDone).
With this change we throw from here at line 73 if encounered a MinimalStage.

I am not claiming this is a bug, or MinimalStage is flawed; neither know whether it is a valid case to have MinimalStage flying around, just letting you know.
Don't shoot the messenger please ;)
Others also implement safety measures: Kotlin/kotlinx.coroutines#2456

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks @nmarton-da. It reproduces indeed:

scala> import scala.jdk.FutureConverters._
import scala.jdk.FutureConverters._

scala> import java.util.concurrent.CompletableFuture
import java.util.concurrent.CompletableFuture

scala> CompletableFuture.completedStage(42).asScala
java.lang.UnsupportedOperationException
  at java.base/java.util.concurrent.CompletableFuture$MinimalStage.isDone(CompletableFuture.java:2976)
  at scala.jdk.javaapi.FutureConverters$.asScala(FutureConverters.scala:73)
  at scala.jdk.FutureConverters$CompletionStageOps$.asScala$extension(FutureConverters.scala:41)
  ... 30 elided

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems play framework had to work around it: playframework/playframework#11993

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, I was busy at shopping festival, will submit fix to other repository this weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
java interop library:concurrent Changes to the concurrency support in stdlib
Projects
None yet
5 participants