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

Dropping some CompletableFuture allocations #3233

Merged

Conversation

dfa1
Copy link
Contributor

@dfa1 dfa1 commented May 29, 2023

Hello @bbakerman @andimarek

just trying to improve the allocation rate (see the discussion in #3175).

There are 3 commits:

  • first one is to drop TracingInstrumentation from BenchMark (to better understand the numbers from the profiler);
  • second one is about to allocate less CompletableFuture in the Async class;
  • third one is about removing the unused index parameter from Async CF factory functions: this will drop the allocations of java.lang.Integer.

On my machine, I get the following numbers.

Baseline:

Result "benchmark.BenchMark.benchMarkSimpleQueriesThroughput":
  62.101 ±(99.9%) 2.006 ops/s [Average]
  (min, avg, max) = (59.572, 62.101, 65.441), stdev = 1.877
  CI (99.9%): [60.095, 64.108] (assumes normal distribution)

With this PR:

Result "benchmark.BenchMark.benchMarkSimpleQueriesThroughput":
  65.212 ±(99.9%) 2.302 ops/s [Average]
  (min, avg, max) = (61.499, 65.212, 68.287), stdev = 2.154
  CI (99.9%): [62.910, 67.515] (assumes normal distribution)

I see also that the allocation rate of CompletableFuture is dropping a bit.

Please have a look :)

dfa1 added 3 commits May 29, 2023 10:44
- dropping TracingInstrumentation as it is causing a lot of extra allocations
- removing some warnings by IntelliJ
- other minor cleanups
Avoiding some unneeded allocations of CompletableFuture in the
internal Async util class.
This is really a minor optimization to avoid
java.lang.Integer allocations.
@dfa1 dfa1 changed the title Drop completable future allocations Drop CompletableFuture allocations May 29, 2023
@dfa1 dfa1 changed the title Drop CompletableFuture allocations Droppinf some CompletableFuture allocations May 29, 2023
@bbakerman bbakerman added this to the 2023 July milestone May 30, 2023
@bbakerman bbakerman self-requested a review May 30, 2023 01:32
Copy link
Member

@bbakerman bbakerman left a comment

Choose a reason for hiding this comment

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

Thanks


@SuppressWarnings("unchecked")
private static <T> CompletableFuture<T> typedEmpty() {
return (CompletableFuture<T>) EMPTY;
}
Copy link
Member

Choose a reason for hiding this comment

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

This is a pretty good pattern for CFs that are really a static value - eg one that is completed and the same value

@bbakerman bbakerman added this pull request to the merge queue May 30, 2023
Merged via the queue into graphql-java:master with commit f866491 May 30, 2023
1 check passed
@dfa1 dfa1 changed the title Droppinf some CompletableFuture allocations Dropping some CompletableFuture allocations May 30, 2023
@dfa1 dfa1 deleted the drop-completable-future-allocations branch May 30, 2023 10:25
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