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 #3221

Merged
merged 1 commit into from Oct 10, 2022

Conversation

He-Pin
Copy link
Contributor

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

Which has better performance.
line/armeria#1596

@He-Pin He-Pin requested a review from a team as a code owner October 8, 2022 08:30
@pivotal-cla
Copy link

@He-Pin Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

1 similar comment
@pivotal-cla
Copy link

@He-Pin Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@He-Pin Thank you for signing the Contributor License Agreement!

@chemicL
Copy link
Member

chemicL commented Oct 10, 2022

@He-Pin thank you for the proposal, looks like a good perf improvement.
@OlegDokuka assuming this gets merged to 3.4.x, will this need to be forward-merged to main or is the lazy implementation completely different and should rather be a separate PR?

@OlegDokuka
Copy link
Contributor

@chemicL this should be forwardmerged. Lazy one uses same method as well

@chemicL
Copy link
Member

chemicL commented Oct 10, 2022

I just noticed #3222 which addresses the issue on main. I suppose we'll need to forward-merge but discard the change and take the one from the other PR instead.

@He-Pin
Copy link
Contributor Author

He-Pin commented Oct 10, 2022

@chemicL Yes ,I created two PR, one for 3.4.x and one for main, I saw @OlegDokuka did more optimizations of memory usage on the current main branch:)
This is my first time contribution, thanks for any guidence.

@OlegDokuka
Copy link
Contributor

@He-Pin second is going to be closed since we use forward merge to keep both branches connected

@He-Pin
Copy link
Contributor Author

He-Pin commented Oct 10, 2022

But in the second one, you fold the BiConsumer to MonoCompletionStageSubscription. so I do it in a separated PR. or will you backport that memory optimization to 3.4.x?
@OlegDokuka so it's not the same pr.

@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()
  }

}

@simonbasle simonbasle added type/enhancement A general enhancement area/performance This belongs to the performance theme labels Oct 10, 2022
@simonbasle simonbasle added this to the 3.4.24 milestone Oct 10, 2022
@simonbasle simonbasle changed the title Make use of CompletionStage#handle instead of whenComplete. Make use of CompletionStage#handle instead of whenComplete Oct 10, 2022
@simonbasle simonbasle merged commit f160733 into reactor:3.4.x Oct 10, 2022
@reactorbot
Copy link

@simonbasle this PR seems to have been merged on a maintenance branch, please ensure the change is merge-forwarded to intermediate maintenance branches and up to main 🙇

simonbasle added a commit that referenced this pull request Oct 10, 2022
Since the codebase for MonoCompletionStage has diverged, there were
some conflicts. `main`-oriented variant of the PR was provided in #3222
which was used to resolve these conflicts.

Supersedes and closes #3222.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/performance This belongs to the performance theme type/enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants