Add benchmarking application. No test clients yet. #629

Yury-Fridlyand · 2023-11-22T19:57:07Z

Issue #, if available:
This PR contains the benchmarking application required by #511.
To reduce PR size, there are no testable clients included.
Part 1 of 4.

Description of changes:
Follow up for #494 and #505.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Signed-off-by: Yury-Fridlyand <[email protected]>

java/benchmarks/src/main/java/javababushka/benchmarks/utils/ChosenAction.java

barshaul · 2023-11-23T10:21:30Z

java/benchmarks/src/main/java/javababushka/benchmarks/BenchmarkingApp.java

-            ClientName.JEDIS, ClientName.JEDIS_ASYNC, ClientName.LETTUCE, ClientName.LETTUCE_ASYNC
+            // ClientName.LETTUCE,
+            // ClientName.LETTUCE_ASYNC,
+            ClientName.BABUSHKA_ASYNC, ClientName.BABUSHKA,


If it's the default configuration - lets leave it only with ClientName.BABUSHKA_ASYNC. remove all others

the default we have in other languages is to run all clients if no clients are specified.

Fixed in 842a222 - changed to all clients

barshaul · 2023-11-23T10:30:52Z

java/benchmarks/src/main/java/javababushka/benchmarks/utils/Benchmarking.java

+                              ChosenAction.GET_NON_EXISTING, new ArrayList<>(),
+                              ChosenAction.SET, new ArrayList<>());
+                      int iterationIncrement = iterationCounter.getAndIncrement();
+                      int clientIndex = iterationIncrement % clients.size();


why not to set it only once, inside the while loop?

It is set outside the loop due to debug logging (off by default) outside the loop - see block on lines 185-189

We may not want to execute the loop. For example, if increment size < tasks.
Or, more realistically, some tasks don't get executed until later in the run, and may not receive an increment index until there's no more work to be done. We see this when running 1000 tasks.

@acarbonetto - so, if we don't get into the loop - why do we need to set client_index at all? and regarding tasks not being used at all - how many tasks do you see that actually do the work?
@Yury-Fridlyand debugging the client index doesn't have any meaning as the client index is suppose to change and isn't fixed per task

I got the client_index code from here: https://github.com/aws/babushka/blob/main/benchmarks/node/node_benchmark.ts#L66 so that we uniformly distribute clients to tasks.

regarding tasks not being used at all - how many tasks do you see that actually do the work?

It seems to cap the number of tasks used at just over 100. I think this number is based on the number of processors or threads available to the event loop. So when we run this with 100 tasks, they are all used fairly uniformly. When we run with 1000 tasks, only about 100 of the tasks get any work (distributed somewhat uniformly over those 100 tasks).

barshaul · 2023-11-23T10:37:20Z

java/benchmarks/src/main/java/javababushka/benchmarks/utils/Benchmarking.java

+            ? () ->
+                ((AsyncClient) client)
+                    .asyncGet(generateKeySet())
+                    .get(ASYNC_OPERATION_TIMEOUT_SEC, TimeUnit.SECONDS)


Wrapping async tasks with timeouts usually has significant impact on the performance. We should configure timeout inside the client and skip this timeout wrapping in the benchmark. We compare async clients to sync clients, and the sync isn't being wrapped with a timeout, so the benchmark should minimize these differences.

Good. I remove timeout here and will add client timeouts in scope of #632

Fixed in 842a222.

shachlanAmazon · 2023-11-23T13:45:26Z

java/benchmarks/src/main/java/javababushka/benchmarks/BenchmarkingApp.java

@@ -209,6 +204,7 @@ public enum ClientName {
    LETTUCE("Lettuce"),
    LETTUCE_ASYNC("Lettuce async"),
    BABUSHKA_ASYNC("Babushka async"),
+    BABUSHKA("Babushka"),


what is this client? let's remove clients which we won't benchmark or don't exist (sync lettuce, sync babushka, async jedis). I'm fine with Babushka being called Babushka instead of babushka async.

Removed. We wanted to compare async Babushka vs async Lettuce and sync Babushka vs sync Lettuce.

Removed in 842a222

shachlanAmazon · 2023-11-23T13:46:03Z

java/benchmarks/src/main/java/javababushka/benchmarks/BenchmarkingApp.java

-            ClientName.JEDIS, ClientName.JEDIS_ASYNC, ClientName.LETTUCE, ClientName.LETTUCE_ASYNC
+            // ClientName.LETTUCE,
+            // ClientName.LETTUCE_ASYNC,
+            ClientName.BABUSHKA_ASYNC, ClientName.BABUSHKA,


the default we have in other languages is to run all clients if no clients are specified.

shachlanAmazon · 2023-11-23T13:46:13Z

java/benchmarks/src/main/java/javababushka/benchmarks/BenchmarkingApp.java

@@ -248,13 +245,15 @@ public RunConfiguration() {
      concurrentTasks = new int[] {100, 1000};
      clients =
          new ClientName[] {
-            // ClientName.BABUSHKA_ASYNC,
-            ClientName.JEDIS, ClientName.JEDIS_ASYNC, ClientName.LETTUCE, ClientName.LETTUCE_ASYNC
+            // ClientName.LETTUCE,


why leave the commented out code?

Removed in 842a222

shachlanAmazon · 2023-11-23T13:46:31Z

java/benchmarks/src/main/java/javababushka/benchmarks/BenchmarkingApp.java

          };
      host = "localhost";
      port = 6379;
-      clientCount = new int[] {1};
+      clientCount = new int[] {1, 2};


these aren't the defaults used in other languages benchmarks.

Fixed in 842a222

shachlanAmazon · 2023-11-23T13:46:46Z

java/benchmarks/src/main/java/javababushka/benchmarks/BenchmarkingApp.java

@@ -248,13 +245,15 @@ public RunConfiguration() {
      concurrentTasks = new int[] {100, 1000};


missing 1, 10

Added in 842a222

shachlanAmazon · 2023-11-23T14:14:44Z

java/benchmarks/src/main/java/javababushka/benchmarks/utils/Benchmarking.java

+  }
+
+  private static double stdDeviation(List<Long> latencies, Double avgLatency) {
+    double stdDeviation =


Fixed in 842a222

shachlanAmazon · 2023-11-23T14:16:00Z

java/benchmarks/src/main/java/javababushka/benchmarks/utils/Benchmarking.java

+      List<Long> latencies = entry.getValue();
+
+      if (latencies.size() == 0) {
+        results.put(action, new LatencyResults(0, 0, 0, 0, 0, 0));


this situation should be impossible. why silently ignore this error?

Fixed in 842a222

shachlanAmazon · 2023-11-23T14:18:30Z

java/benchmarks/src/main/java/javababushka/benchmarks/utils/Benchmarking.java

+    System.out.println("Total hits: " + totalHits);
+  }
+
+  public static void testClientSetGet(


why is this in a single, huge, very indented function? can it be split into subroutines?

Refactored in 28e5fe0

shachlanAmazon · 2023-11-23T14:20:01Z

java/benchmarks/src/main/java/javababushka/benchmarks/utils/Benchmarking.java

+
+          var clientName = clients.get(0).getName();
+
+          System.out.printf(


please log data size too

Fixed in 842a222

shachlanAmazon · 2023-11-23T14:26:06Z

java/benchmarks/src/main/java/javababushka/benchmarks/utils/Benchmarking.java

+                              iterationIncrement + 1, iterations, clientIndex + 1, clientCount);
+                        }
+
+                        var actions = getActionMap(clients.get(clientIndex), dataSize, async);


why do you create a new map on every iteration, instead of creating a single map once per client?

clientIndex is changing every loop iteration

then pass clientIndex or client as an argument the functions in the map, and reuse the map.

Fixed in f7dbd3d.

acarbonetto · 2023-11-22T23:14:09Z

java/benchmarks/build.gradle

@@ -18,6 +19,10 @@ dependencies {
    implementation 'io.lettuce:lettuce-core:6.2.6.RELEASE'
    implementation 'commons-cli:commons-cli:1.5.0'
    implementation group: 'org.apache.commons', name: 'commons-lang3', version: '3.13.0'
+    implementation group: 'com.google.code.gson', name: 'gson', version: '2.10.1'
+
+    compileOnly 'org.projectlombok:lombok:1.18.30'


not sure we need lombok just for ConnectionSettings. We can talk about it, or consider removing it later if we don't use lombok throughout.

Lombok removed in 842a222

acarbonetto · 2023-11-22T23:15:54Z

java/benchmarks/src/main/java/javababushka/benchmarks/clients/AsyncClient.java

+
+  long DEFAULT_TIMEOUT_MILLISECOND = 1000;
+
+  Future<T> asyncConnectToRedis(ConnectionSettings connectionSettings);


if asyncConnectToRedis is defined here, why are we extending Client? Seems like we should be reusing connectToClient instead.
note: this means we will have to update and separate our JavaClient into an async and sync client.

Removed in 842a222. We don't test (don't benchmark) connection - so all clients do a sync connect.

acarbonetto · 2023-11-22T23:24:49Z

java/benchmarks/src/main/java/javababushka/benchmarks/utils/Benchmarking.java

+    return (Math.floor(Math.random() * SIZE_SET_KEYSPACE) + 1) + "";
+  }
+
+  public interface Operation {


we can make this private

No - it is used in IT (#632)

We should investigate this and find a better way to test then.

Signed-off-by: Yury-Fridlyand <[email protected]>

shachlanAmazon · 2023-11-24T08:22:11Z

java/benchmarks/src/main/java/javababushka/benchmarks/BenchmarkingApp.java

-        case JEDIS_ASYNC:
-          // run testClientSetGet on JEDIS pseudo-async client
-          System.out.println("Run JEDIS pseudo-async client");
-          break;
        case LETTUCE:
          // run testClientSetGet on LETTUCE sync client
          System.out.println("Run LETTUCE sync client");


it's an async client

Oh I deleted the wrong reference. Restored.

Signed-off-by: Yury-Fridlyand <[email protected]>

acarbonetto · 2023-11-24T07:32:04Z

java/benchmarks/src/main/java/javababushka/benchmarks/utils/Benchmarking.java

+    try {
+      actions.get(action).go();
+    } catch (Exception e) {
+      // timed out - exception from Future::get


The Operator is equally unspecific, but we can can change that to only ignore some specific handled types. Shachar is right in that we need to report timeout errors from upstream.

acarbonetto · 2023-11-24T07:33:43Z

java/benchmarks/src/main/java/javababushka/benchmarks/utils/Benchmarking.java

+  }
+
+  public interface Operation {
+    void go() throws Exception;


we can be more explicit instead of throwing Exceptions

Fixed in 859c9e6

acarbonetto · 2023-11-24T07:35:48Z

java/benchmarks/src/main/java/javababushka/benchmarks/utils/Benchmarking.java

+    return (Math.floor(Math.random() * SIZE_SET_KEYSPACE) + 1) + "";
+  }
+
+  public interface Operation {


We should investigate this and find a better way to test then.

acarbonetto · 2023-11-24T07:38:31Z

java/benchmarks/src/main/java/javababushka/benchmarks/utils/Benchmarking.java

+                              ChosenAction.GET_NON_EXISTING, new ArrayList<>(),
+                              ChosenAction.SET, new ArrayList<>());
+                      int iterationIncrement = iterationCounter.getAndIncrement();
+                      int clientIndex = iterationIncrement % clients.size();


We may not want to execute the loop. For example, if increment size < tasks.
Or, more realistically, some tasks don't get executed until later in the run, and may not receive an increment index until there's no more work to be done. We see this when running 1000 tasks.

Signed-off-by: Yury-Fridlyand <[email protected]>

barshaul · 2023-11-28T08:40:35Z

java/benchmarks/src/main/java/javababushka/benchmarks/BenchmarkingApp.java

-                "one of:"
-                    + " all|jedis|jedis_async|lettuce|lettuce_async|babushka_async|all_async|all_sync"
-                    + " [all]")
+                "one of: all|jedis|jedis_async|lettuce|lettuce_async|"


Should remove all irrelevant clients

Fixed in d3f707f.

barshaul · 2023-11-28T08:41:13Z

java/benchmarks/src/main/java/javababushka/benchmarks/BenchmarkingApp.java

-    ALL("All"),
-    ALL_SYNC("All sync"),
-    ALL_ASYNC("All async");
+    JEDIS("Jedis"), // async


async -> sync?

Fixed in d3f707f.

barshaul · 2023-11-28T08:43:59Z

java/benchmarks/src/main/java/javababushka/benchmarks/clients/AsyncClient.java

+  Future<String> asyncGet(String key);
+
+  default <T> T waitForResult(Future<T> future) {
+    return waitForResult(future, DEFAULT_TIMEOUT_MILLISECOND);


We don't need to wrap the results with timeouts

It is a protection against infinite hangs. If something happen, a CI run won't finish and will block running tests for other clients.
Please confirm that I can remove it.

barshaul · 2023-11-28T08:45:20Z

java/benchmarks/src/main/java/javababushka/benchmarks/clients/AsyncClient.java

+
+  default <T> T waitForResult(Future<T> future, long timeout) {
+    try {
+      return future.get(timeout, TimeUnit.MILLISECONDS);


barshaul · 2023-11-28T08:46:43Z

java/benchmarks/src/main/java/javababushka/benchmarks/clients/AsyncClient.java

+    try {
+      return future.get(timeout, TimeUnit.MILLISECONDS);
+    } catch (TimeoutException ignored) {
+      return null;


we don't want to ignore errors

Fixed in d3f707f.

barshaul · 2023-11-28T09:03:53Z

java/benchmarks/src/main/java/javababushka/benchmarks/utils/Benchmarking.java

+      }
+    }
+
+    System.out.println();


Why is it needed?

To prettify the printed output - it adds blank lines between test reports to ease reading.

shachlanAmazon

no serious design issues.
@barshaul leaving to you for final verifications and merging once all remaining comments are fixed.

shachlanAmazon · 2023-11-28T09:05:57Z

java/benchmarks/src/main/java/javababushka/benchmarks/BenchmarkingApp.java

-          // run testClientSetGet on JEDIS sync client
-          System.out.println("Run JEDIS sync client");
+          // run testClientSetGet on JEDIS pseudo-sync client
+          System.out.println("Run JEDIS async client");


why do you log Jedis as async? there's no such thing, and we shouldn't create it.
Babushka & Lettuce are async, Jedis is sync. that's all we need.

Sorry, it was uncleaned reference to that client. It is already deleted.
Fixed in d3f707f.

shachlanAmazon · 2023-11-28T09:08:10Z

java/benchmarks/src/main/java/javababushka/benchmarks/utils/Benchmarking.java

+                  ChosenAction.GET_EXISTING, new ArrayList<>(),
+                  ChosenAction.GET_NON_EXISTING, new ArrayList<>(),
+                  ChosenAction.SET, new ArrayList<>());
+          int iterationIncrement = iterationCounter.getAndIncrement();


why do you increment this outside of the loop? wouldn't it make more sense to just increment it at the beginning of the while loop instead of before the loop AND inside the loop?

Fixed in d3f707f.

shachlanAmazon · 2023-11-28T09:10:33Z

java/benchmarks/src/main/java/javababushka/benchmarks/utils/Benchmarking.java

+          int clientIndex = iterationIncrement % clients.size();
+
+          if (debugLogging) {
+            System.out.printf(


notice that logging the client index here is misleading, since the client index changes in the loop.

Fixed in d3f707f.

shachlanAmazon · 2023-11-28T09:11:23Z

java/benchmarks/src/main/java/javababushka/benchmarks/utils/Benchmarking.java

+            var actions = getActionMap(clients.get(clientIndex), dataSize, async);
+            // operate and calculate tik-tok
+            Pair<ChosenAction, Long> result = measurePerformance(actions);
+            if (result != null) {


when would this be null?

Nice catch! It is never happen now.
Fixed in d3f707f.

shachlanAmazon · 2023-11-28T09:13:31Z

java/benchmarks/src/main/java/javababushka/benchmarks/utils/Benchmarking.java

@@ -51,15 +52,21 @@ public static Pair<ChosenAction, Long> measurePerformance(Map<ChosenAction, Oper
    long before = System.nanoTime();
    try {
      actions.get(action).go();
-    } catch (Exception e) {
-      // timed out - exception from Future::get
+    } catch (TimeoutException ignored) {


why ignore timeouts? if the timeouts are set to a generous value, such as 1 second, the benchmark shouldn't timeout. if it does, it's probably an error.

Fixed in 859c9e6.

shachlanAmazon · 2023-11-28T09:23:46Z

java/benchmarks/src/main/java/javababushka/benchmarks/utils/Benchmarking.java

-    actions.put(
+        client ->
+            () -> {
+              if (async) {


minor: why drop the ternary operator syntax?

Java does not accept this in void expressions. I was also disappointed.
We ignore results of get/set, Operation interface defines a void function.

shachlanAmazon · 2023-11-28T09:25:37Z

java/benchmarks/src/main/java/javababushka/benchmarks/BenchmarkingApp.java

-                "one of:"
-                    + " all|jedis|jedis_async|lettuce|lettuce_async|babushka_async|all_async|all_sync"
-                    + " [all]")
+                "one of: all|jedis|jedis_async|lettuce|lettuce_async|"


just all |jedis|lettuce|babushka
(if you also want all sync / all async that's fine, but I don't see the benefit there)

Nice catch, thank you.
Fixed in d3f707f.

shachlanAmazon · 2023-11-28T09:26:43Z

java/benchmarks/src/main/java/javababushka/benchmarks/BenchmarkingApp.java

-    ALL("All"),
-    ALL_SYNC("All sync"),
-    ALL_ASYNC("All async");
+    JEDIS("Jedis"), // async


Fixed in d3f707f.

shachlanAmazon · 2023-11-28T09:32:26Z

java/benchmarks/src/main/java/javababushka/benchmarks/utils/Benchmarking.java

+        });
+  }
+
+  public static Map<ChosenAction, Function<Client, Operation>> getActionMap(


minor: instead of a function returning a function, Operation can just take a Client as an argument, no?

D'oh!
Fixed in d3f707f.

Signed-off-by: Yury-Fridlyand <[email protected]>

barshaul · 2023-12-05T10:55:01Z

@Yury-Fridlyand Please pull latest changes & resolve conflicts, squash the commits and merge

Add benchmarking application. No test clients yet.

5b31ff9

Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand requested review from jonathanl-bq, barshaul, asafpamzn, shachlanAmazon, acarbonetto and Elen-Ghulam November 22, 2023 19:57

Yury-Fridlyand requested a review from a team as a code owner November 22, 2023 19:57

Yury-Fridlyand added the java issues and fixes related to the java client label Nov 22, 2023

This was referenced Nov 22, 2023

Auxiliary changes for java client benchmarking. #630

Merged

Add java JNI/Netty client - rust part. #631

Merged

Add test clients for benchmarking. #632

Merged

barshaul requested changes Nov 23, 2023

View reviewed changes

shachlanAmazon reviewed Nov 23, 2023

View reviewed changes

acarbonetto reviewed Nov 23, 2023

View reviewed changes

Address PR feedback.

842a222

Signed-off-by: Yury-Fridlyand <[email protected]>

shachlanAmazon reviewed Nov 24, 2023

View reviewed changes

Yury-Fridlyand added 2 commits November 24, 2023 08:18

Restore Jedis async.

58f3684

Signed-off-by: Yury-Fridlyand <[email protected]>

refactor

28e5fe0

Signed-off-by: Yury-Fridlyand <[email protected]>

acarbonetto reviewed Nov 24, 2023

View reviewed changes

Yury-Fridlyand added 3 commits November 27, 2023 08:19

Add exception handling.

3138a7a

Signed-off-by: Yury-Fridlyand <[email protected]>

Typo fix.

859c9e6

Signed-off-by: Yury-Fridlyand <[email protected]>

Rework getActionMap.

f7dbd3d

Signed-off-by: Yury-Fridlyand <[email protected]>

barshaul approved these changes Nov 28, 2023

View reviewed changes

shachlanAmazon approved these changes Nov 28, 2023

View reviewed changes

Yury-Fridlyand added 2 commits November 29, 2023 13:15

remaning

c83d4f3

Signed-off-by: Yury-Fridlyand <[email protected]>

Revert some renamings.

199ed5a

Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand mentioned this pull request Nov 30, 2023

[to remove] Java benchmarks: add Jedis and Lettuce clients (sync and async) #540

Closed

Address PR feedback.

d3f707f

Signed-off-by: Yury-Fridlyand <[email protected]>

Merge branch 'main' into integ-java-client-milestone-1-benchmarking

26e786d

Yury-Fridlyand merged commit b5fc593 into valkey-io:main Dec 6, 2023
2 checks passed

Yury-Fridlyand deleted the integ-java-client-milestone-1-benchmarking branch December 6, 2023 03:00

		@@ -248,13 +245,15 @@ public RunConfiguration() {
		concurrentTasks = new int[] {100, 1000};


		var clientName = clients.get(0).getName();

		System.out.printf(


		long DEFAULT_TIMEOUT_MILLISECOND = 1000;

		Future<T> asyncConnectToRedis(ConnectionSettings connectionSettings);

Add benchmarking application. No test clients yet. #629

Add benchmarking application. No test clients yet. #629

Conversation

Yury-Fridlyand commented Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shachlanAmazon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yury-Fridlyand commented Nov 22, 2023 •

edited

Loading

shachlanAmazon left a comment •

edited

Loading