Repository navigation
feat: End to end performance tests - #257
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run //:license-checkStatus: Click to expand output |
|
Documentation preview for this pull request is available at: |
|
Coverage report for this pull request: Status: success (workflow run) The archive contains the HTML report, the raw LCOV data and the JUnit XML test results. |
|
|
||
| The `.perf.data` recordings and `.perf.svg` flamegraphs are written beside `benchmarks.json`. | ||
|
|
||
| The test starts and stops both daemon pairs and the echo server itself. The client and server use |
There was a problem hiding this comment.
if we use linux namespaces and this shouldn't be possible since the 3 processes are separated from the other 3 processes by the linux os
There was a problem hiding this comment.
Linux Namespace is only used to separate tests from host environment and to have predefined environment (e.g. ports are available). All applications are running inside the same namespace.
I did not want to build too much on Linux Namespaces, because I still want to keep some possibility to launch the tests without bazel. I will consider if something can be done inside the Python Code.
There was a problem hiding this comment.
my personal opninion i dont like not having this isolation between the 2 network stacks . if it was separate namespaces then we could have passed 1 mwcom json with the same 2 ids to both client and ipc benchmark app . now we need 4 service ids 7200,7203 ,7201, 7202 and 2 diff mwcom json files
There was a problem hiding this comment.
I assumed this is the simpler solution. If we want to reuse the mwcom.json we need to put the benchmark processes and echo_server processes into separate Linux namespaces. I hope it works to connect both with a network bridge. This also gets closer to the docker-compose setup I am just removing.
IMHO the biggest benefit is that the current way is quite flexible. You can run on the same machine without containers or with containers. I assume it also works on real hardware or using QEMU. There is a reason I prefer to start with running everything on the same machine: I can then profile everything at once and see issues.
Global profiling is not implemented yet and I wonder if in this PR I should actually switch from per application profiling to global profiling.
| - name: Run Linux perf tests | ||
| run: | | ||
| bazel test --config=perf-tests //tests/benchmarks/... --build_tests_only | ||
| - name: Run Linux perf tests (flamegraphs) |
There was a problem hiding this comment.
why do we need to run these performance tests on the gh pipeline ? only to see that to have some graphs ? the results cannot be used since this is not running on a real hardware
There was a problem hiding this comment.
main purpose is that they do not bitrot and start to fail without us knowing. Github CI is I guess the least best place to get actual performance numbers from.
| << " -c/--configuration Specifies the configuration file\n" | ||
| << " -i/--ipc_channel Name of the IPC channel to gatewayd (default: " | ||
| << score::someip::kDefaultIpcChannelName << ")\n" | ||
| << "\n"; |
There was a problem hiding this comment.
both apps already have names (ipc_benchmarks and echo_server) so i propose meaningfull names instead of a and b
config/node_a_mw_com_config.json→config/bench_mw_com_config.jsonconfig/node_b_mw_com_config.json→config/echo_mw_com_config.jsonconfig/node_a_someip_config.json→config/bench_someip_config.jsonconfig/node_b_someip_config.json→config/echo_someip_config.jsonconfig/vsomeip_node_a.json→config/vsomeip_bench.jsonconfig/vsomeip_node_b.json→config/vsomeip_echo.json
| NODE_A = NodeSpec( | ||
| name="benchmark_node_a", | ||
| ipc_channel="benchmark_ipc_a", | ||
| vsomeip_network="benchmark-node-a", |
There was a problem hiding this comment.
Could the nodes be named after the app each one hosts instead of a/b? NODE_A -> BENCH_NODE (runs ipc_benchmarks), NODE_B -> ECHO_NODE (runs echo_server), with name="bench" / "echo" so a failed run leaves bench_gatewayd.log rather than benchmark_node_a_gatewayd.log. Same for NODE_A_IP / NODE_B_IP in setup_network.sh.
| NODE_A_IP="127.0.0.2" | ||
| NODE_B_IP="127.0.0.3" |
There was a problem hiding this comment.
NODE_A_IP / NODE_B_IP -> BENCH_IP / ECHO_IP if we go with the node rename.
| generate_someip_config_bin( | ||
| name = "node_a_config_file", | ||
| json = "config/node_a_someip_config.json", | ||
| output = "node_a_someip_config.bin", | ||
| ) | ||
|
|
||
| generate_someip_config_bin( | ||
| name = "node_b_config_file", | ||
| json = "config/node_b_someip_config.json", | ||
| output = "node_b_someip_config.bin", | ||
| ) |
There was a problem hiding this comment.
These should follow the rename too: node_a_config_file -> bench_config_file, config/node_a_someip_config.json -> config/bench_someip_config.json, same for the _b / mw_com ones.
| ) | ||
|
|
||
| validate_json_schema_test( | ||
| name = "benchmark_mw_com_config_schema_valid", |
There was a problem hiding this comment.
Nothing loads config/benchmark_mw_com_config.json any more — Drop it, the JSON, and the now-unused load on line 18?
There was a problem hiding this comment.
replaced with validation of mw_com config for bench and echo node
| `e2e_benchmarks` runs the existing Google Benchmark client and echo server over the complete | ||
| mw::com -> gatewayd -> someipd -> SOME/IP -> someipd -> gatewayd -> mw::com path. Both nodes run | ||
| in the isolated network namespace configured by `tests/benchmarks/setup_network.sh`, at | ||
| `127.0.0.2` and `127.0.0.3`. |
There was a problem hiding this comment.
The two throughput benchmarks looks to me they only write to a shm as fast as possible ?!
request_event.Send(std::move(pre_serialized_request));
, just send in a loop and never wait, so their messages/sec is how fast the client can write to shm
Can we say here which benchmark measures what? Otherwise that number ends up being quoted as SOME/IP traffic ?
There was a problem hiding this comment.
I will change the logic. I did not look too much into the C++ applications because I assumed they would already be fine. However the more I look into it, it looks like fire+forget without checking if anything has been received. I was working under the assumption that the ipc_benchmark.cpp code checks the echoed data.
There was a problem hiding this comment.
Clarification: The LatencyEcho test does that. The other are just fire and forget. I will change profiling to the LatencyEcho test.
| benchmark_cmd = _perf_record_argv(benchmark_perf_data, benchmark_cmd) | ||
|
|
||
| try: | ||
| with node_a, node_b: |
There was a problem hiding this comment.
i get a passed test even with side b missing completely
xxx@yyyy:score_inc_someip_gateway$ bazel test --config=perf-tests --nocache_test_results //tests/benchmarks:e2e_benchmarks --test_output=summary
WARNING: Option 'local_ram_resources' is deprecated: --local_ram_resources is deprecated, please use --local_resources=memory= instead.
WARNING: Option 'local_cpu_resources' is deprecated: --local_cpu_resources is deprecated, please use --local_resources=cpu= instead.
WARNING: Option 'local_ram_resources' is deprecated: --local_ram_resources is deprecated, please use --local_resources=memory= instead.
WARNING: Option 'local_cpu_resources' is deprecated: --local_cpu_resources is deprecated, please use --local_resources=cpu= instead.
INFO: Analyzed target //tests/benchmarks:e2e_benchmarks (0 packages loaded, 13 targets configured).
INFO: Found 1 test target...
Target //tests/benchmarks:e2e_benchmarks up-to-date:
bazel-bin/tests/benchmarks/e2e_benchmarks
INFO: Elapsed time: 44.232s, Critical Path: 42.72s
INFO: 2 processes: 11 action cache hit, 2 linux-sandbox.
INFO: Build completed successfully, 2 total actions
//tests/benchmarks:e2e_benchmarks PASSED in 42.5s
Executed 1 out of 1 test: 1 test passes.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are.
There was a problem hiding this comment.
benchmark.cpp's main function always returned 0 / EXIT_SUCCESS. I fixed that now.
|
LGTM to me as a first try .i am done and submitted my review |
The stress test will lose too much messages. I guess we need a better test, which does better rate limiting
| @@ -20,15 +20,16 @@ ARG USERNAME=vscode | |||
| # RENAME 'vscode' user to the specified username to satisfy the QNX license | |||
| # We use usermod to change the login name and home directory | |||
There was a problem hiding this comment.
LGTM all my comments were adressed
We need performance tests and profiling to figure out where we can improve the performance. This is only the first step and I would consider this far from being perfect.
I reworked the existing performance tests and made them work using bazel. We can also optionally run them using perf to create flamegraphs.
Having worked on the tests and seeing their CPU usage, I think there is room for improvement. I was expecting my CPU to be tortured, but it did not happen. I also see that dropped messages seem to be counted as delivered by the performance tests. I refrain from changing the C++ code too much to keep the PR at a reasonable size. Reworking the C++ code is a followup.
Closes #251