-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add benchmarks for json string serialize/deserialize #12258
Conversation
There's an embarassing bug in the workflow alerter. It ignores time units. So "1 ms" compared to "900 us" looks like a performance regression of 900x: paradust7@28ed9bf#commitcomment-72699948 I'm going to disable alerts for now, and hope this is the only bug. |
Example page with results: https://sfan5.github.io/minetest/dev/bench/ |
That's pretty cool |
Sad. It's strange that it has no problem parsing the time units, but does nothing with them. We could fork github-action-benchmark to fix this, but it would be probably be easier to just have catch2 always emit the same unit for now. EDIT: I filed an issue for them: benchmark-action/github-action-benchmark#122 |
Co-authored-by: sfan5 <[email protected]>
Co-authored-by: sfan5 <[email protected]>
Can you give this action write permissions only on gh-pages? I didn't realize it was a third-party action. Maybe forking it is a good idea, or revision pinning. |
How would I do that? It worked out of the box on my personal repo (after creating the branch). |
Unfortunately, I don't see any way to limit it. The workflow gives it write permission for "deployments" and "contents" access groups. These are documented in terms of API access. "Contents" allows it to generate a pull request and merge it. But I don't see any way to limit it to a specific branch. There's a "pages" API group but it seems to be read-only. https://docs.github.com/en/rest/overview/permissions-required-for-github-apps If the action is pinned to a specific revision, at least that will guarantee that future changes to upstream will not introduce a sudden surprise. But won't help if there's already a backdoor or exploit in it. (very unlikely, it's only 4529 lines, but still...) Alternatively, could put the workflow in a completely separate repository, e.g. minetest/benchmarks, which has its own gh-pages, and keeps sync with a cron workflow. I'll try to do this and see if there are any snags. |
@sfan5 I got it working successfully from another repo. Can you create a "benchmarks" repo (or other appropriate name) under the minetest organization, so I can make a PR against it? Thanks. The "benchmarks" repo will also need a gh-pages branch to work. One will be created automatically if you setup a theme, as per these instructions: https://docs.github.com/en/pages/getting-started-with-github-pages/adding-a-theme-to-your-github-pages-site-with-the-theme-chooser The graphs will then be available at https://minetest.github.io/benchmarks/dev/bench |
I removed the github workflow from this PR. It will instead be in the benchmarks repo. |
Sounds good. How will the other workflow track commits in the main repo? Does Github Actions have something for that?
I can't do this myself but I'll get someone who can to do it. |
Here you are: https://github.com/minetest/benchmarks |
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.
Looks good.
Looks good apart from that indentation issue. Haven't tested, will trust sfan5 on that |
Thanks! Here's the PR on the benchmarks repo: |
--run-benchmarks
command-line flagBUILD_BENCHMARKS
flag which isFALSE
by default.src/benchmark
directorybenchmark_serialize.cpp
which contains benchmarks of serializeJsonStringIfNeeded and deSerializeJsonStringIfNeeded(*) I have no way of testing this, so it may be better to use the web interface to create it. It assumes the existence of a
gh-pages
branch specially created as described here: https://github.com/benchmark-action/github-action-benchmark