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

Add SPEEDTEST #10920

Merged
merged 3 commits into from
Jul 2, 2023
Merged

Add SPEEDTEST #10920

merged 3 commits into from
Jul 2, 2023

Conversation

blyxyas
Copy link
Member

@blyxyas blyxyas commented Jun 10, 2023

In the master branch, we currently don't have any way to test the performance of a single lint in changes.
This PR adds SPEEDTEST, the environment variable which lets you do a speed test on a lint / category of tests with various configuration options.

Maybe we should merge this with lintcheck 🤔
See the book page for more information.

changelog:none

@rustbot
Copy link
Collaborator

rustbot commented Jun 10, 2023

r? @llogiq

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 10, 2023
@blyxyas blyxyas force-pushed the speedtest branch 2 times, most recently from 9b0099f to 7a49175 Compare June 10, 2023 09:33
Copy link
Member

@y21 y21 left a comment

Choose a reason for hiding this comment

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

I usually put a Instant::now().elapsed() check in a lint myself when making a PR to see roughly what impact it has, so this would be really nice to have if it was directly supported! I have one question tho:

Comment on lines +381 to +246
let mut sum = 0;
for _ in 0..iterations {
let start = std::time::Instant::now();
f();
sum += start.elapsed().as_millis();
}
Copy link
Member

Choose a reason for hiding this comment

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

since the timing itself also takes some time to run, would it make sense to move the timing stuff out of the loop and divide the time taken by the number of iterations after the loop? the Instant::now().elapsed() overhead might also be negligible compared to how long a lint pass takes, so maybe this doesn't matter that much (from what I remember, it was around 40ns on the playground...)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the usual way to micro benchmark is to time variable numbers of iterations, then do a linear regression, but this is clearly a macro benchmark, so don't worry about either loop or time measurement overhead.

@bors
Copy link
Contributor

bors commented Jun 26, 2023

☔ The latest upstream changes (presumably #10426) made this pull request unmergeable. Please resolve the merge conflicts.

@xFrednet
Copy link
Member

xFrednet commented Jun 26, 2023

Since this PR popped up in my notifications and to continue something @blyxyas started:


Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

=^.^=

@llogiq
Copy link
Contributor

llogiq commented Jul 2, 2023

Thank you! @bors r+

@bors
Copy link
Contributor

bors commented Jul 2, 2023

📌 Commit 57923c3 has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 2, 2023

⌛ Testing commit 57923c3 with merge ea4ca22...

@bors
Copy link
Contributor

bors commented Jul 2, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing ea4ca22 to master...

1 similar comment
@bors
Copy link
Contributor

bors commented Jul 2, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing ea4ca22 to master...

@bors bors merged commit ea4ca22 into rust-lang:master Jul 2, 2023
@blyxyas blyxyas deleted the speedtest branch October 5, 2023 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants