Skip to content

de-stabilize bench attribute #134273

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.

Sign up for GitHub

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

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

de-stabilize bench attribute #134273

wants to merge 2 commits into from
+16 −100

Conversation

RalfJung
Copy link
Member

This has been soft-unstable since forever (#64066), and shown in future-compat reports since Rust 1.77 (#116274).

The feature covering bench itself is tracked in #50297, which has been closed despite still having active feature gates referencing it.

Cc @rust-lang/libs-api

@rustbot
Copy link
Collaborator

rustbot commented Dec 13, 2024

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 13, 2024
@RalfJung
Copy link
Member Author

Hm actually I guess this is more of a lang thing, sorry... Cc @rust-lang/lang

@RalfJung RalfJung added T-lang Relevant to the language team, which will review and decide on the PR/issue. I-lang-nominated Nominated for discussion during a lang team meeting. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 13, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung
Copy link
Member Author

@ehuss what is the right thing to do with a lint that is not emitted any more by the compiler, but is kept around since it might be emitted again in the future? I don't think it makes sense to have an example in this lint, since we'll keep changing when the lint is emitted (as some soft-unstable items become real-unstable, and other stable items become soft-unstable).

@ehuss
Copy link
Contributor

ehuss commented Dec 14, 2024

What you've done looks good to me.

@RalfJung RalfJung added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Dec 14, 2024
@RalfJung RalfJung mentioned this pull request Dec 14, 2024
Open
@RalfJung
Copy link
Member Author

Maybe we should crater this as well, since it does have the potential of breaking stable code.
@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 20, 2024
@bors
Auto merge of rust-lang#134273 - RalfJung:de-stabilize-bench, r=<try>
2b41547 
de-stabilize bench attribute

This has been soft-unstable since forever (rust-lang#64066), and shown in future-compat reports since Rust 1.77 (rust-lang#116274).

The feature covering `bench` itself is tracked in rust-lang#50297, which has been closed despite still having active feature gates referencing it.

Cc `@rust-lang/libs-api`
@bors
Copy link
Collaborator

bors commented Dec 20, 2024

⌛ Trying commit f5c63b9 with merge 2b41547...

@bors
Copy link
Collaborator

bors commented Dec 20, 2024

☀️ Try build successful - checks-actions
Build commit: 2b41547 (2b41547e41f63ea0ca9cb25417ddd569b19e6a50)

@RalfJung
Copy link
Member Author

RalfJung commented Jan 4, 2025

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-134273 created and queued.
🤖 Automatically detected try build 2b41547
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jan 4, 2025
@craterbot
Copy link
Collaborator

🚧 Experiment pr-134273 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-134273 is completed!
📊 161 regressed and 28 fixed (563652 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jan 6, 2025
@RalfJung RalfJung mentioned this pull request Jan 6, 2025
Merged
@RalfJung
Copy link
Member Author

RalfJung commented Jan 6, 2025

Looking at the crates.io regressions, 12 out of 14 are spurious. The remaining 2 are:

@RalfJung RalfJung mentioned this pull request Jan 6, 2025
Open
@RalfJung RalfJung mentioned this pull request Jan 19, 2025
Open
@ibraheemdev ibraheemdev added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 31, 2025
@RalfJung
Copy link
Member Author

@rust-lang/lang 4 months have passed, any thoughts on this? :)

@traviscross
Copy link
Contributor

Thanks for the ping. Our design meetings for the next four weeks are all focused on bringing down the queue, so we should get to this.

Chausseaumoine pushed a commit to Chausseaumoine/lox-zkp-fork that referenced this pull request Apr 11, 2025
@onyiny-ang
Remove unstable bench test framework
628ff03 
This repo originally used the #[bench] test framework which is unstable
and has started throwing clippy warnings which will soon become a hard
error: rust-lang/rust#134273
This MR moves to Criterion for benchmarking, similar to dalek-ed25519
for the dleq benchmarking but removes benchmarking for the define_proof
macro. This should be revisited and is noted in lox-zkp#2
@traviscross
Copy link
Contributor

We discussed this in the lang call today. Let's propose to do this.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 23, 2025 •
edited by joshtriplett
Loading

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 23, 2025
@traviscross traviscross added I-lang-radar Items that are on lang's radar and will need eventual work or consideration. and removed I-lang-nominated Nominated for discussion during a lang team meeting. labels Apr 23, 2025
@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Apr 23, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 23, 2025

🔔 This is now entering its final comment period, as per the review above. 🔔

@joshtriplett
Copy link
Member

Relatedly, for anyone who is looking at this later on: We should consider having good a first-party benchmarking framework, for the same reason that we have a first-party test framework. Our first-party test framework has made testing extremely widespread in the Rust ecosystem, and benchmarking would have much the same effect. Right now, benchmarking requires third-party frameworks like divan or criterion.

Thoughts for folks working on libtest, and/or the folks working on benchmarking frameworks. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ibraheemdev ibraheemdev

Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. I-lang-radar Items that are on lang's radar and will need eventual work or consideration. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@RalfJung @rustbot @rust-log-analyzer @ehuss @bors @craterbot @traviscross @rfcbot @joshtriplett @ibraheemdev