Skip to content

Cli tool to compare commits locally #1734

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Closed
Nadrieril opened this issue Oct 12, 2023 · 3 comments
Closed

Cli tool to compare commits locally #1734

Nadrieril opened this issue Oct 12, 2023 · 3 comments
Labels
help wanted We are looking for volunteers to take on this issue

Comments

@Nadrieril
Copy link
Member

Hi! I tend to work on performance-sensitive parts of the compiler so I check performance locally a lot. In the current setup this requires me to run a docker script and check a website page. It would make my life much easier if I could run say rustc-perf compare <commita> <commitb> instructions:u and get out the little table of relevant benchmark diffs. Would that be easy to implement?
@Kobzol
Copy link
Contributor

Kobzol commented Oct 12, 2023

Hi, you don't really have to run Docker, you can either just build the website locally (using npm run watch && cargo build -p site) or you can download a precompiled binary with the website (https://github.com/rust-lang/rustc-perf/releases).

That being said, a basic CLI output wouldn't be a bad idea, we already have it for runtime benchmarks.

It should be relatively easy to implement, either by modifying the behavior of bench_local, and adding output of the measured results, or by introducing a separate new command for this. The collector code was heavily refactored and running benchmarks from it in a new command should hopefully be relatively easy.

@Kobzol Kobzol mentioned this issue May 25, 2024
Open
@Kobzol Kobzol added the help wanted We are looking for volunteers to take on this issue label Jun 10, 2024
@Kobzol Kobzol mentioned this issue Jun 12, 2024
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 22, 2024
@matthiaskrgr
Rollup merge of rust-lang#126318 - Kobzol:bootstrap-perf, r=onur-ozkan
162120b 
Add a `x perf` command for integrating bootstrap with `rustc-perf`

This PR adds a new `x perf` command to bootstrap. The idea is to let rustc developers profile (`profile_local`) and benchmark (`bench_local`) a stage1/stage2 compiler directly from within `rust`.

Before, if you wanted to use `rustc-perf`, you had to clone it, set it up, copy the `rustc` sysroot after every change to `rust` etc. This is an attempt to automate that.

I opened this PR mostly for discussion. My idea is to offer an interface that looks something like this (a random sample of commands):
```bash
x perf --stage 2 profile eprintln
x perf --stage1 profile cachegrind
x perf benchmark --id baseline
x perf benchmark --id after-edit
x perf cmp baseline after-edit
```

In this PR, I'd like to only implement the simplest case (`profile_local (eprintln)`), because that only requires a single sysroot (you don't compare anything), and it's relatively easy to set up. Also, I'd like to avoid forcing developers to deal with the rustc-perf UI, so more complex use-cases (like benchmarking two sysroots and comparing the results) should probably wait for rust-lang/rustc-perf#1734 (which is hopefully coming along soon-ish).

I'm not sure if it's better to do this in bootstrap directly, or if I should create some shim tool that will receive a `rustc` sysroot, and offer a simplified CLI on top of `rustc-perf`.

## Why is a separate CLI needed?
We definitely need to add some support to bootstrap to automate preparing `rustc-perf` and the `rustc` sysroot, but in theory after that we could just let people invoke `rustc-perf` manually. While that is definitely possible, you'd need to manually figure out where is your sysroot located, which seems annoying to me. The `rustc-perf` CLI is also relatively complex, and for this use-case it makes sense to only use a subset of it. So I thought that it would be better to offer a simplified interface on top of it that would make life easier for contributors. But maybe it's not worth it.

CC `@onur-ozkan`
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jun 22, 2024
@rust-timer
Unrolled build for rust-lang#126318
63c59f7 
Rollup merge of rust-lang#126318 - Kobzol:bootstrap-perf, r=onur-ozkan

Add a `x perf` command for integrating bootstrap with `rustc-perf`

This PR adds a new `x perf` command to bootstrap. The idea is to let rustc developers profile (`profile_local`) and benchmark (`bench_local`) a stage1/stage2 compiler directly from within `rust`.

Before, if you wanted to use `rustc-perf`, you had to clone it, set it up, copy the `rustc` sysroot after every change to `rust` etc. This is an attempt to automate that.

I opened this PR mostly for discussion. My idea is to offer an interface that looks something like this (a random sample of commands):
```bash
x perf --stage 2 profile eprintln
x perf --stage1 profile cachegrind
x perf benchmark --id baseline
x perf benchmark --id after-edit
x perf cmp baseline after-edit
```

In this PR, I'd like to only implement the simplest case (`profile_local (eprintln)`), because that only requires a single sysroot (you don't compare anything), and it's relatively easy to set up. Also, I'd like to avoid forcing developers to deal with the rustc-perf UI, so more complex use-cases (like benchmarking two sysroots and comparing the results) should probably wait for rust-lang/rustc-perf#1734 (which is hopefully coming along soon-ish).

I'm not sure if it's better to do this in bootstrap directly, or if I should create some shim tool that will receive a `rustc` sysroot, and offer a simplified CLI on top of `rustc-perf`.

## Why is a separate CLI needed?
We definitely need to add some support to bootstrap to automate preparing `rustc-perf` and the `rustc` sysroot, but in theory after that we could just let people invoke `rustc-perf` manually. While that is definitely possible, you'd need to manually figure out where is your sysroot located, which seems annoying to me. The `rustc-perf` CLI is also relatively complex, and for this use-case it makes sense to only use a subset of it. So I thought that it would be better to offer a simplified interface on top of it that would make life easier for contributors. But maybe it's not worth it.

CC `@onur-ozkan`
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Jun 23, 2024
@matthiaskrgr
Rollup merge of #126318 - Kobzol:bootstrap-perf, r=onur-ozkan
a2c6ad3 
Add a `x perf` command for integrating bootstrap with `rustc-perf`

This PR adds a new `x perf` command to bootstrap. The idea is to let rustc developers profile (`profile_local`) and benchmark (`bench_local`) a stage1/stage2 compiler directly from within `rust`.

Before, if you wanted to use `rustc-perf`, you had to clone it, set it up, copy the `rustc` sysroot after every change to `rust` etc. This is an attempt to automate that.

I opened this PR mostly for discussion. My idea is to offer an interface that looks something like this (a random sample of commands):
```bash
x perf --stage 2 profile eprintln
x perf --stage1 profile cachegrind
x perf benchmark --id baseline
x perf benchmark --id after-edit
x perf cmp baseline after-edit
```

In this PR, I'd like to only implement the simplest case (`profile_local (eprintln)`), because that only requires a single sysroot (you don't compare anything), and it's relatively easy to set up. Also, I'd like to avoid forcing developers to deal with the rustc-perf UI, so more complex use-cases (like benchmarking two sysroots and comparing the results) should probably wait for rust-lang/rustc-perf#1734 (which is hopefully coming along soon-ish).

I'm not sure if it's better to do this in bootstrap directly, or if I should create some shim tool that will receive a `rustc` sysroot, and offer a simplified CLI on top of `rustc-perf`.

## Why is a separate CLI needed?
We definitely need to add some support to bootstrap to automate preparing `rustc-perf` and the `rustc` sysroot, but in theory after that we could just let people invoke `rustc-perf` manually. While that is definitely possible, you'd need to manually figure out where is your sysroot located, which seems annoying to me. The `rustc-perf` CLI is also relatively complex, and for this use-case it makes sense to only use a subset of it. So I thought that it would be better to offer a simplified interface on top of it that would make life easier for contributors. But maybe it's not worth it.

CC `@onur-ozkan`
This was referenced Jul 29, 2024
Merged
Merged
@s7tya
Copy link
Contributor

s7tya commented Sep 16, 2024

I think we can close this because the bench_cmp command is implemented and it's ported into the bootstrap. Of course we should improve the functionality though.

@Kobzol
Copy link
Contributor

Kobzol commented Sep 16, 2024

Agreed 👍

@Kobzol Kobzol closed this as completed Sep 16, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Assignees
No one assigned
Labels
help wanted We are looking for volunteers to take on this issue
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@Kobzol @Nadrieril @s7tya