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

Make use of CompletionStage#handle instead of whenComplete. #3475

Merged
merged 1 commit into from
Oct 11, 2022

Conversation

He-Pin
Copy link
Contributor

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

Which leads a better performance.
line/armeria#1440
akka/akka#31283

@qwwdfsad qwwdfsad self-requested a review October 10, 2022 08:26
Copy link
Contributor

@qwwdfsad qwwdfsad left a 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

@He-Pin
Copy link
Contributor Author

He-Pin commented Oct 10, 2022

@qwwdfsad thanks for the reviewing, you can take a look at this link line/armeria#1444, handle helps especially when the CompletionStage is failed.

I will submit an jmh bench later。

@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 He-Pin requested a review from qwwdfsad October 10, 2022 12:13
@qwwdfsad
Copy link
Contributor

Thanks!

For the history, here is the coroutines-related benchmark and the results that are much less noisy:

@Warmup(iterations = 7, time = 1)
@Measurement(iterations = 5, time = 1)
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.MILLISECONDS)
@State(Scope.Benchmark)
@Fork(1)
open class FutureBenchmark {

    private val exception = Exception()


    /*
     OLD:

     FutureBenchmark.builderExceptionally    avgt    5  4.075 ± 0.348  us/op
     FutureBenchmark.builderNormally         avgt    5  0.833 ± 0.011  us/op
     FutureBenchmark.converterExceptionally  avgt    5  3.141 ± 0.264  us/op
     FutureBenchmark.converterNormally       avgt    5  0.269 ± 0.009  us/op


     NEW:
     FutureBenchmark.builderExceptionally    avgt    5  1.474 ± 0.053  us/op
     FutureBenchmark.builderNormally         avgt    5  0.924 ± 0.065  us/op
     FutureBenchmark.converterExceptionally  avgt    5  1.716 ± 0.086  us/op
     FutureBenchmark.converterNormally       avgt    5  0.274 ± 0.007  us/op

     */
    @Benchmark
    fun builderNormally(): CompletableFuture<*> = runBlocking {
        val f = future(Job()) {
            42
        }
        f
    }

    @Benchmark
    fun builderExceptionally(): CompletableFuture<*> = runBlocking {
        val f = future(Job()) {
            throw exception
        }
        f
    }

    @Benchmark
    fun converterNormally(): CompletableFuture<*> {
        val j = Job()
        val future = j.asCompletableFuture()
        future.complete(Unit)
        return future
    }

    @Benchmark
    fun converterExceptionally(): CompletableFuture<*> {
        val j = Job()
        val future = j.asCompletableFuture()
        future.completeExceptionally(exception)
        return future
    }
}

Copy link
Contributor

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

Thanks!

@qwwdfsad qwwdfsad changed the base branch from master to develop October 10, 2022 15:58
@qwwdfsad qwwdfsad changed the base branch from develop to master October 10, 2022 15:58
@qwwdfsad
Copy link
Contributor

Please rebase your PR on develop branch and change the target branch to develop as well

@He-Pin He-Pin changed the base branch from master to develop October 10, 2022 16:51
@He-Pin
Copy link
Contributor Author

He-Pin commented Oct 10, 2022

Have rebased on to the develop and retarget develop branch.

Copy link
Contributor

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants