ci: add continuous benchmarking #1726

CarlWachter · 2025-05-20T16:31:24Z

This will execute the benchmarks in the hermit-rs repo according to the bench.json file. Results will be published to Github Pages.

CI will fail until hermit-os/hermit-rs#708 gets merged.

.github/workflows/ci.yml

jounathaen · 2025-05-21T09:13:14Z

Ci is unfortunately still failing, despite hermit-os/hermit-rs#708 being merged and also used.

.github/workflows/ci.yml

CarlWachter · 2025-05-21T18:39:47Z

@jounathaen it's passing now and results look good

jounathaen · 2025-05-22T09:30:44Z

The benchmarks take 27 minutes now, which is 10 mins longer than the tests. Can we change the benchmark setup, so that the CI duration is not dominated by the benchmarks anymore (parallelize it on two runners or reduce the benchmark size)?

mkroening · 2025-05-22T11:43:32Z

I think the benchmarks taking more time is fine, but I would suggest running the benchmark suite after merging and on PRs only on demand.

jounathaen · 2025-05-22T12:30:07Z

I think the benchmarks taking more time is fine, but I would suggest running the benchmark suite after merging and on PRs only on demand.

But in that case, we wouldn't know if a PR affects performance before merging it.

jounathaen · 2025-05-22T12:32:20Z

As @mkroening pointed out, we already have gh-pages installed for this repository (https://hermit-os.github.io/kernel/hermit/). We would need to find a way of publishing the results on a different URL. Martin suggested a separate repository for gh-pages, where this action pushes to. Maybe a combination of both gh-pages into subdirectories is also possible.

CarlWachter · 2025-05-26T08:57:04Z

We would need to find a way of publishing the results on a different URL. Martin suggested a separate repository for gh-pages, where this action pushes to. Maybe a combination of both gh-pages into subdirectories is also possible.

Is that absolutely necessary? We could use the same gh-pages branch of this Repo and have benchmarks hosted on https://hermit-os.github.io/kernel/performance/ (or something similar), all it takes is adjusting the folder github-action-benchmark puts the data into

Edit: Nevermind, i see the problem.... Another Repo sounds like a good idea. Seperate subdirectories would require the publish_docs to stop force pushing and that may lead to a lot of unecessary history

CarlWachter · 2025-06-02T10:12:12Z

Maybe a combination of both gh-pages into subdirectories is also possible.

By adding the keep_history flag to publish_docs.yml this works quite well:

      - name: Deploy documentation
        if: success()
        uses: crazy-max/ghaction-github-pages@v4
        with:
          target_branch: gh-pages
          build_dir: target/x86_64-unknown-none/doc
          keep_history: true

I've tested it on my fork https://github.com/CarlWachter/kernel.

(See also: example of outputs: https://carlwachter.github.io/kernel/benchmarks/,
example of PR with comparative comment: CarlWachter#5)

What do you think of this solution?

CarlWachter · 2025-06-02T11:01:57Z

Okay this is a little weird: To run the workflow with permissions to push and comment on PRs with results, the workflow has to use the pull_request_target trigger, which only works if the workflow already exists on the target Repo. As such this can't be executed in this PR, but would work for future PRs once this is merged. I therefore request you look at the changes and the previously mentioned test PRs in my fork (CarlWachter#5) to review this.

On that fork the only notable difference is the trigger method and runner (as it does not have a self-hosted runner) and it appears to function as intended.

jounathaen · 2025-06-03T10:06:38Z

.github/workflows/ci.yml

@@ -3,7 +3,7 @@ name: CI
 on:
  pull_request:
  merge_group:
-
+    


Tiny nitpick: Whitespace added

mkroening

Thanks for the PR! :)

I have a few requests. :D

.github/workflows/publish_docs.yml

.github/workflows/benchmark.yml

.github/benchmarks/netbench.json

.github/workflows/benchmark.yml

mkroening · 2025-06-03T12:07:55Z

.github/benchmarks/netbench.json

+    },
+    {
+        "name": "Netbench TCP BW - Client",
+        "command": "parallel ::: 'cargo run --manifest-path ./Cargo.toml --bin tcp-server-bw --release --target x86_64-unknown-linux-gnu -- --address 10.0.5.3 --bytes 1048576 --rounds 1000' 'sleep 10 && sudo qemu-system-x86_64 -display none -serial stdio -kernel hermit-loader-x86_64 -cpu qemu64,apic,fsgsbase,rdtscp,xsave,xsaveopt,fxsr,rdrand -enable-kvm -initrd target/x86_64-unknown-hermit/release/tcp-client-bw -smp 2 -m 1024M -netdev user,id=u1,hostfwd=tcp::9975-:9975,hostfwd=udp::9975-:9975,net=192.168.76.0/24,dhcpstart=192.168.76.9 -device virtio-net-pci,netdev=u1,disable-legacy=on,packed=on,mq=on -append \"-- --nonblocking --address 127.0.0.1 --bytes 1048576 --rounds 1000\"'",


Is there a reason for using user networking instead of taps? I get more than 15% more bandwidth on my machine.

No particular reason that i can think of, I'd be happy to switch to taps if you tell me the args you used to achieve that performance bump

It should work as described in the loader's README. :)

mkroening · 2025-06-03T12:12:15Z

.github/benchmarks/netbench.json

+    },
+    {
+        "name": "Netbench TCP Latency - Server",
+        "command": "parallel ::: 'sleep 10 && cargo run --manifest-path ./Cargo.toml --bin tcp-client-latency --release --target x86_64-unknown-linux-gnu -- --nonblocking --address 127.0.0.1 --bytes 1048576 --rounds 250' 'sudo qemu-system-x86_64 -display none -serial stdio -kernel hermit-loader-x86_64 -cpu qemu64,apic,fsgsbase,rdtscp,xsave,xsaveopt,fxsr,rdrand -enable-kvm -initrd target/x86_64-unknown-hermit/release/tcp-server-latency -smp 2 -m 1024M -netdev user,id=u1,hostfwd=tcp::7878-:7878,hostfwd=udp::9975-:9975,net=192.168.76.0/24,dhcpstart=192.168.76.9 -device virtio-net-pci,netdev=u1,disable-legacy=on,packed=on,mq=on -append \"-- --address 10.0.5.3 --bytes 1048576 --rounds 250\"'",


The forwarded ports for user networking are quite inconsistent right now. The application uses port 7878 by default and is configurable via CLI, but parts of the UDP and latency benches have 9975 hardcoded, which should be fixed.

The QEMU invocations should only forward the correct ports that are necessary.

I've changed the forwarded ports to be minimal now. As for the hardcoding that is in reference to the port that is being connected to, rather than the one that is binded to (which is determined by the CLI arg). Since this is explicitly a benchmark, not an example, i don't see any issue with that being hardcoded.

Ah, I see. So you mean that the remote port we connect to is inferred from CLI but the local port that we send the request from is hardcoded, right? In that case, the port should better be chosen by the operating system by specifying 0, or does Hermit not support that?

Yes i've tried that and had no success getting it to bind to port 0, always failing when i attempted that. It works when i run the code like that on a non-hermit system so i assume hermit does not support that.

Oh, I see. I have opened an issue for supporting ephemeral ports (#1753). Could you add a TODO note that the hardcoded value is not relevant and should be changed to zero once possible? I am also wondering if changing 7878 to 9975 as well would be more or less confusing. 🤔

Shouldn't that issue be on hermit-rs, since that is where the benchmarks with the hardcoded ports are?

Well, the issue is that the kernel does not support that. Sure, user space should be changed once the root issue is resolved.

n0toose · 2025-06-08T13:40:06Z

But in that case, we wouldn't know if a PR affects performance before merging it.

Hi, just chiming in: The (relatively newer) workflow_dispatch event could help with triggering workflows on demand, should a reviewer think that a PR could affect performance: https://docs.github.com/en/actions/managing-workflow-runs-and-deployments/managing-workflow-runs/manually-running-a-workflow

CarlWachter · 2025-06-08T14:58:29Z

I've addressed most of the issues brought up now, but once again ran into issues with permissions. Namely pushing to the hermit-bench Repo is not permissible with the default GITHUB_TOKEN.

@jounathaen @mkroening How would you feel about a Organisation owned PAT? It would allow the action to push to hermit-bench and be able to function on the standard pull_request trigger.

CarlWachter · 2025-06-20T06:32:49Z

I've cut netbench out for now as i have not been able to test with tap devices and there is a kernel side incompatibility of the current netbench implementation with the kernel anyhow (#1780).

Otherwise this PR is ready and I would recommend we merge this today without netbench.

I once again tested everything on my fork to ensure: CarlWachter#9

jounathaen reviewed May 21, 2025

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

CarlWachter force-pushed the feat/cb branch 4 times, most recently from 592e8c2 to dba2fde Compare May 21, 2025 11:54

CarlWachter mentioned this pull request May 21, 2025

bench(alloc): Reduce size of allocations for 8GB runner compatability hermit-os/hermit-rs#709

Merged

jounathaen closed this in hermit-os/hermit-rs#709 May 21, 2025

jounathaen reopened this May 21, 2025

jounathaen reviewed May 21, 2025

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

CarlWachter force-pushed the feat/cb branch from 47d7e2f to 475da90 Compare June 2, 2025 09:52

CarlWachter force-pushed the feat/cb branch from a8d5ea4 to df7c9a6 Compare June 2, 2025 11:00

CarlWachter requested a review from jounathaen June 2, 2025 11:02

jounathaen reviewed Jun 3, 2025

View reviewed changes

.github/workflows/ci.yml Outdated

@@ -3,7 +3,7 @@ name: CI

on:

pull_request:

merge_group:

Copy link

Member

jounathaen Jun 3, 2025

Choose a reason for hiding this comment

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

Tiny nitpick: Whitespace added

jounathaen approved these changes Jun 3, 2025

View reviewed changes

mkroening self-requested a review June 3, 2025 10:10

mkroening requested changes Jun 3, 2025

View reviewed changes

CarlWachter added 3 commits June 19, 2025 20:14

ci: add continuous benchmarking

292231e

fix(cb): install parallel for networking benchmarks

195b9b1

feat(bench): Parallel execution of benchmarks

17609dd

CarlWachter added 13 commits June 19, 2025 20:14

feat(bench): Label benchmark sets

158f85d

feat(ci): move benchmark to seperate workflow

a6d2877

fix(ci): add missing permissions for benchmark job

4916cd4

fix(ci): give PR writing permissions for forks

38d98ce

fix(ci): prevent publish_docs from clearing benchmark data

e5c2040

fix(bench): better qemu flags and benchmark arguments

0994886

cleanup: reformat benchmark workflow

19df47f

feat(bench): publish data to hermit-bench

8929fd5

feat(bench): prebuild netbench reference and tighten timing

611fd09

bench: use enviorment specific token

28c9a27

fix(bench): update plot group labels

226168f

bench: temporarily remove netbench

a76c663

bench: no default env

84dbe03

CarlWachter force-pushed the feat/cb branch from c6cfc74 to 84dbe03 Compare June 19, 2025 18:14

CarlWachter requested review from mkroening and jounathaen June 20, 2025 06:32

bench: build all benchmarks seperately

f5a019f

ci: add continuous benchmarking #1726

Are you sure you want to change the base?

ci: add continuous benchmarking #1726

Uh oh!

Conversation

CarlWachter commented May 20, 2025

Uh oh!

Uh oh!

jounathaen commented May 21, 2025

Uh oh!

Uh oh!

CarlWachter commented May 21, 2025

Uh oh!

jounathaen commented May 22, 2025

Uh oh!

mkroening commented May 22, 2025

Uh oh!

jounathaen commented May 22, 2025

Uh oh!

jounathaen commented May 22, 2025

Uh oh!

CarlWachter commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CarlWachter commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CarlWachter commented Jun 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkroening left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n0toose commented Jun 8, 2025

Uh oh!

CarlWachter commented Jun 8, 2025

Uh oh!

CarlWachter commented Jun 20, 2025

Uh oh!

Uh oh!

CarlWachter commented May 26, 2025 •

edited

Loading

CarlWachter commented Jun 2, 2025 •

edited

Loading