test: perf: add end-to-end benchmark for alternator

[nuivall]

The code is based on similar idea as perf_simple_query. The main differences are:

• it starts full scylla process
• communicates with alternator via http (localhost)
• uses richer table schema with all dynamoDB types instead of only strings

Testing code runs in the same process as scylla so we can easily get various perf counters (tps, instr, allocation, etc).

Results on my machine (with 1 vCPU):

./build/release/scylla perf-alternator-workloads --workdir ~/tmp --smp 1 --developer-mode 1 --alternator-port 8000 --alternator-write-isolation forbid --workload read --duration 10 2> /dev/null
...
median 23402.59616090321
median absolute deviation: 598.77
maximum: 24014.41
minimum: 19990.34

./build/release/scylla perf-alternator-workloads --workdir ~/tmp --smp 1 --developer-mode 1 --alternator-port 8000 --alternator-write-isolation forbid --workload write --duration 10 2> /dev/null
...
median 16089.34211320635
median absolute deviation: 552.65
maximum: 16915.95
minimum: 14781.97

The above seem more realistic than results from perf_simple_query which are 96k and 49k tps (per core).

Related: #12518

[scylladb-promoter]

CI state SUCCESS - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/4845/

[nyh]

Testing code runs in the same process as scylla so we can easily get various perf counters (tps, instr, allocation, etc).

Ideally an external process could have also gotten the same counters using metrics?

What worries me a bit about having the client in the same process as the server is that it mixes the performance of the client and server, and also might make it harder to include some things (like client authentication). But I guess it's a good start.

[nyh]

Looks good (I just left a few comments/questions), I am guessing that you'll probably want to improve this code together with doing Alternator optimizations, and understanding better what you really want to benchmark / profile.

[nyh]

space after if

[nuivall]

fixed

[nyh]

A thought (feel free to discard): this after_init_func made the alternator_workloads function very weird - it needs to return a lambda, set another lambda, etc. Wouldn't it be easier for main to offer a promise to wait for initialization to complete (maybe we already have such a thing somehow?) the the workload function will be just like the rest, just start by running main and await in the beginning for the initialization to complete?)

[nuivall]

Maybe it's doable to split it to 2 parts, but I wanted changes to be minimal in scylla_main as it's already very complex (or at least long).

It has something like supervisor::notify which could be extended, although this is "global complexity" vs "local complexity" to me. perf::alternator_workloads is more complex but it doesn't affect anything not related to it while extending this supervisor::notify for instance (or something similar) would affect every usage.

the difference versus other perf:: functions from above stem from the fact that it's the only one which needs scylla_main to be called, other replace it.

[nyh]

what does this move achieve?

[avikivity]

It's probably to avoid the lambda coroutine fiasco. Please see if coroutine::lambda() fixes it instead.

[nyh]

I thought about this, but wasn't this fiasco about captures, not parameters? (but maybe I'm misremembering).

[nuivall]

yes, it was added to keep it alive. Indeed coroutine fiasco document mentions only captures. I am not sure why this is happening. It looks like in_ is freed after the first suspension point in lambda.

Adding coroutine::lambda or the second solution with std::ref doesn't help here.

Perhaps there is something wrong with caller code in seastar?

            return do_with(std::move(rep), [&con, handle = std::move(handle)] (auto& rep) mutable {
                return handle(rep, con.in(rep));
            });

handle is my lambda from above. I've tried also put it in do_with to keep it alive, without success:

            return do_with(std::move(rep), std::move(handle), [&con] (auto& rep, auto& handle) mutable {
                return handle(rep, con.in(rep));
            });

so this trick with
auto in = std::move(in_); is the only one which works for me (I saw it also here: https://github.com/scylladb/scylladb/blob/master/alternator/executor.cc#L91 and in one of Pavel's wip patches)

[avikivity]

The entire coroutine frame is lost. It applies equally to captures, parameters, and locals that are promoted to live in the coroutine frame.

[avikivity]

So it's better to use coroutine::lambda(), it solves the problem rather than working around it and failing if someone adds a local variable.

[nuivall]

like just wrap the lambda?

co_await cli.make_request(std::move(req), coroutine::lambda([] (const http::reply& rep, input_stream<char>&& in_) -> future<> {
...
})

tested this before and it doesn't solve it

[nuivall]

I think it can be solved but in seastar instead:

diff --git a/src/http/client.cc b/src/http/client.cc
index 7a66823c..46df99df 100644
--- a/src/http/client.cc
+++ b/src/http/client.cc
@@ -236,9 +236,9 @@ future<> client::make_request(request req, reply_handler handle, reply::status_t
                 return make_exception_future<>(std::runtime_error(format("request finished with {}", rep._status)));
             }
 
-            return do_with(std::move(rep), [&con, handle = std::move(handle)] (auto& rep) mutable {
-                return handle(rep, con.in(rep));
-            });
+            return do_with(std::move(rep), coroutine::lambda([&con, handle = std::move(handle)] (auto& rep) mutable -> future<> {
+                co_await handle(rep, con.in(rep));
+            }));
         });
     });
 }

while this is documented
/// lambda coroutine must complete (co_await) in the same statement.
I am not sure why it requires immediate co_await to work

[nyh]

I don't understand what I'm seeing here. You have "concurrency", and yet just one "cli" object per shard. How does it work?

[nuivall]

I think it was working because underneath connection::make_request doesn't yield, at least in this case.

Although I think it's more realistic for alternator to work on a higher number of connections, so I will add a pool, making one number of connections equal to concurrency.

[nuivall]

v2:

• space added
• connection pool instead of one connection per shard
• added option to run against remote host

[scylladb-promoter]

CI state SUCCESS - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/193/

[scylladb-promoter]

CI state FAILURE - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/1487/

[nuivall]

v3:

• added scan workload

[scylladb-promoter]

CI state FAILURE - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/1576/

[scylladb-promoter]

CI state FAILURE - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/1591/

[scylladb-promoter]

CI state SUCCESS - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/1605/

[scylladb-promoter]

CI state SUCCESS - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/2578/

[nuivall]

v4:

• added option to continue after failed requests
• added gsi write workload

[scylladb-promoter]

🔴 CI State: FAILURE

✅ - Build
✅ - Unit Tests
❌ - Sanity Tests

Failed Tests (1/21011):

• test_replace_node_using_the_same_ip_then_shut_down[rbo_disabled] 🔍

Build Details:

• Build URL: https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/4388/
• Duration: 2 hr 35 min
• Builder: spider5.cloudius-systems.com

[mykaul]

Test failure could be #15334 - looks unrelated to this PR?

[scylladb-promoter]

🔴 CI State: FAILURE

✅ - Build
✅ - Container Test
✅ - dtest
✅ - dtest with topology changes
❌ - Unit Tests

Failed Tests (2/30566):

• test_auth_v2_migration 🔍
• auth_cluster.test_auth_v2_migration.debug.3 🔍

Build Details:

• Duration: 2 hr 40 min
• Builder: spider3.cloudius-systems.com

[nuivall]

@nyh I've synchronized this PR with enterprise one. I think you can merge both.

[avikivity]

Testing code runs in the same process as scylla so we can easily get various perf counters (tps, instr, allocation, etc).

How do you isolate client code from server code?

[nuivall]

Testing code runs in the same process as scylla so we can easily get various perf counters (tps, instr, allocation, etc).

How do you isolate client code from server code?

I don't. Anyway interesting is delta (i.e. before and after some patch). Absolute number has very little meaning.

[nyh]

@nyh I've synchronized this PR with enterprise one. I think you can merge both.

Thanks! I'm waiting for the CI to finish.

[scylladb-promoter]

🔴 CI State: FAILURE

✅ - Build
✅ - Container Test
❌ - dtest with topology changes
✅ - dtest
✅ - Unit Tests

Build Details:

• Duration: 5 hr 49 min
• Builder: spider7.cloudius-systems.com

[nuivall]

Seemingly unrelated test failed, filled https://github.com/scylladb/scylla-dtest/issues/4252, restarting

[nuivall]

@yarongilor clang-tidy is failing with

Error: /home/runner/work/scylladb/scylladb/serializer_impl.hh:20:10: error: 'absl/container/btree_set.h' file not found [clang-diagnostic-error]
   20 | #include <absl/container/btree_set.h>
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
(...)
ninja: build stopped: subcommand failed.
Error: Process completed with exit code 1.

don't see ehow this is related to the PR. Does it block the merge now?

[scylladb-promoter]

🟢 CI State: SUCCESS

✅ - Build
✅ - Container Test
✅ - dtest with topology changes
✅ - dtest
✅ - Unit Tests

Build Details:

• Duration: 2 hr 38 min
• Builder: spider5.cloudius-systems.com

[nyh]

Thanks @nuivall. I see CI passed, so I merged now.