-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Make use of CompletionStage#handle
instead of whenComplete
.
#3475
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked the implementation of uniWhenCompleteStage
and it seems like the issue described in Akka/armeria is only applicable for *Async
methods, not to regular ones.
I've also benchmarked the simplest asCompletableFuture
and future{}
builders and found no difference.
Taking into account that handle
is semantically heavier than when*
as it returns new value and potentially can change the result of the computation when used carelessly, it would be nice to see a benchmark that shows that this change yields a performance improvement prior to merge.
Code-wise changes looks good
@qwwdfsad thanks for the reviewing, you can take a look at this link line/armeria#1444, I will submit an jmh bench later。 |
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()
}
}
|
Thanks! For the history, here is the coroutines-related benchmark and the results that are much less noisy:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Please rebase your PR on |
Have rebased on to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Which leads a better performance.
line/armeria#1440
akka/akka#31283