-
Notifications
You must be signed in to change notification settings - Fork 43
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
feat(perf): add nim-libp2p v1.1 #262
base: master
Are you sure you want to change the base?
Conversation
Triggered a CI run: https://github.com/libp2p/test-plans/actions/runs/5881897412/job/15951280528 |
@lchenut missing step (2) and (3).
https://github.com/libp2p/test-plans/tree/master/perf#adding-a-new-implementation-or-a-new-version Excited to see the results. |
Perf run: https://github.com/libp2p/test-plans/actions/runs/5888783646/job/15970676824 On success you can see the results on: https://observablehq.com/@libp2p-workspace/performance-dashboard?branch=perf-nim |
perf/impl/nim-libp2p/v1.0/Makefile
Outdated
perf: perf.nim nim-libp2p | ||
docker run --rm --user "$(shell id -u):$(shell id -g)" -v "$(shell pwd)":/usr/src/myapp -w /usr/src/myapp nimlang/nim:1.6.10 sh -c "cd nim-libp2p && nimble install_pinned && cd - && nim c --NimblePath:nim-libp2p/nimbledeps/pkgs -p:nim-libp2p -d:chronicles_log_level=WARN --threads:off -d:release perf.nim" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI has failed:
bash: line 1: ./impl/nim-libp2p/v1.0/perf: No such file or directory
Is this actually producing a binary under the name perf
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I was afraid of. It's a Docker problem, it creates an ELF executable, no matter what, where it should create a host-compatible executable.
But it seems that there's no nim image on docker hub with this option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a bit of research, the problem doesn't come from the executable itself, but come from the linking of the .so...
$ file perf
perf: ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, BuildID[sha1]=895de8bd9fdf766ac9448fa05056cb4370bb6f54, for GNU/Linux 3.2.0, not stripped
/lib64/ld-linux-x86-64.so.2 isn't found
And I have no idea what I should use instead.
Friendly ping @lchenut. Any progress on the above linking issue? |
Hum... I was so sure about this one. After a bit of digging and not finding a clear way to fix this (non-existant). It seems that I forgot to include |
Cool. Mind resolving the merge conflicts @lchenut? I can then trigger another run. |
@mxinden done |
@mxinden friendly ping to trigger the CI :) |
Sorry for the delay. I was out sick with COVID for a week. Resolved conflicts with #299 and triggered CI: https://github.com/libp2p/test-plans/actions/runs/6188558497/job/16800823070 |
@lchenut @Menduist CI run failed:
|
@mxinden sorry for the delay... it should be good now. |
Triggered a new run: https://github.com/libp2p/test-plans/actions/runs/6473803273/job/17577296660 |
Hi @mxinden, we often find ourselves needing CI runs for our PRs but lack the necessary permissions. Is there a way to streamline this process to minimize manual coordination? Thanks. |
In case you want to run the perf tests off of your laptop against your own AWS account: https://github.com/libp2p/test-plans/blob/master/perf/README.md#running-manually In case you want to run the perf tests off of the nim-libp2p CI, you would need to run against your own AWS account. For that, the Either way, I can't give you access to the Protocol Labs AWS account. |
In case you have not seen the failures:
|
@mxinden It's my bad, I should have pinged you when I pushed last week. Like the last three times, it should work now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sorry that this is dragging along.
Unfortunately the output format has changed in the meantime. I don't expect this requires a large change on your end, though you will need to continuously emit the current throughput every 1s.
You can find the larger rational in #261. I expect this to be especially interesting for you, given that you looked into congestion controller slow start for your upcoming Gossipsub optimizations.
dur = dur + (await PerfClient.perf(conn, f.uploadBytes, f.downloadBytes)) | ||
let ns = dur.nanos | ||
let s = Second.nanos | ||
echo "{\"latency\": ", fmt"{ns div s}.{ns mod s:09}", "}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the output format has changed with #276.
The pull request updated the documentation. You can find the new output format here:
Now that the perf protocol is merged.
I'm a bit rusty with docker though, so I'm not 100% sure if what I did is correct.