Skip to content
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

Draft: Save timings to Bencher #2

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Draft: Save timings to Bencher #2

wants to merge 2 commits into from

Conversation

epompeii
Copy link

This is a first pass at saving rebar timings to Bencher.

The CLI has to already be installed in order for things to work. In a follow up, this requirement will be removed, and we'll just send the POST request ourselves. Having Bencher publish its types would make this easier, so I'll do that first.

My main goal with this draft PR is to just get general feedback and input both on code and the matching of the rebar model up with the Bencher model.

@epompeii epompeii requested a review from BurntSushi March 30, 2023 16:13
@BurntSushi
Copy link
Owner

BurntSushi commented Mar 30, 2023

Thanks! I think the concept here looks good. The actual nuts & bolts is probably not quite right though. I think the biggest issue is that bencher submission shouldn't be tied to measurement. Measurement is S..L..O..W. Instead, I think there should be a new sub-command called bencher that uses the results produced by rebar measure. There are several commands like that already, for example, rebar diff and rebar cmp and rebar report.

Otherwise, a smallish nit is that we shouldn't bring in dependencies for trivial things like generating slugs. Now, the deunicoding part of that isn't trivial. But! Engine names are already pretty restricted and limited to ASCII. Which makes the slug routine nearly trivial. Moreover, I'd imagine we could just use the engine name as-is without needing to slugify it. Or are there requirements on bencher's end? Maybe the / is a problem? Maybe we could just substitute / with -.

and we'll just send the POST request ourselves

I think I'd actually rather just have bencher as a CLI installed. If rebar is going to send the request, then that means I need to bring in an HTTP client dependency. And I don't think there is any HTTP client I want to bring in here in the Rust ecosystem. It'd be one thing if HTTP was the main point of rebar, but we shouldn't balloon compile times for something like this.

Sorry I am cranky about dependencies. It's just way too easy to add them and end up in a situation where you need to build hundreds of crates.

@epompeii
Copy link
Author

Measurement is S..L..O..W. Instead, I think there should be a new sub-command called bencher that uses the results produced by rebar measure.

Sounds good!

Engine names are already pretty restricted and limited to ASCII.

Ah! I did not see that. I will just substitute / with -, as you suggested. And yeah, the value needs to either be the slug or the UUID for the branch (or in this case engine). It may be worth noting that if an engine ever gets renamed for some reason, we'll need to switch things over to a map of UUIDs. For now though, I think the simple substitution should be fine.

I think I'd actually rather just have bencher as a CLI installed.

That works for me.

@BurntSushi
Copy link
Owner

Ah interesting. Yeah, I do hope engines won't get renamed, but it is very possibly that I have my taxonomy or classification incorrect in some way. But yeah, let's not worry about that right now.

@epompeii
Copy link
Author

epompeii commented Mar 30, 2023

Since we're keeping the CLI, we could use --if-branch instead of --branch as the former expects the branch/engine name.

There are some trade offs though. --if-branch queries the backend every time to see if that branch name exists. This could be convenient for you in the long term though as there is also a way to have bencher create the branch/engine if it does not already exist on the backend.

This would keep you from having to manually adding every new engine to Bencher. Though, the update process in the case of a name change would still be manual.

See: https://bencher.dev/docs/explanation/branch-selection

@epompeii
Copy link
Author

epompeii commented Apr 3, 2023

This updated switches from Bencher being a flag under measure to its own bencher subcommand.
In doing so, it became a lot easier to just go for our end goal of comparing the geometric means for the speed ratios.
To facilitate this, I have broken out shared code between bencher and report into its own module, flattened.

I've create a new metric kind named Speed Ratio for the geometric mean.
The semantic meaning of branches is no longer overloaded.
The benchmark names are now just the engine names with a suffix, /search or /compile to indicate search or compile summaries respectively.
This completely removes the previous worry about existing branches, etc. I have also added a master branch for future use.

An example can be seen here: https://bencher.dev/console/projects/rebar/perf?key=true&tab=benchmarks&metric_kind=speed-ratio&branches=30c09da5-3f38-4bd7-aa10-0dde97e77049&testbeds=320bfacb-119b-4519-b45d-f2dae844ce96&benchmarks=1509ceb5-1199-4238-ac9f-91394f6dc28f%2C06092431-00d2-4cdf-b6a8-d8cbfcc38d9f

@BurntSushi
Copy link
Owner

This is looking much better, thank you.

I'm not sure when I'll get around to experimenting with this. Hopefully soon.

If it's okay with you, I might not actually bring this specific PR in, but rather, use your bencher sub-command as a template of sorts to work off of. The reason is that I'm not quite sure this refactoring here is the direction I want to go in, but I'm also not sure what the right direction is yet. My suspicion is that I want to make the geometric mean more of a first class concept in the tooling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants