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 note about remove_duplicate_unreachable_blocks in source comments #110658

Closed
wants to merge 1 commit into from

Conversation

miguelraz
Copy link
Contributor

@miguelraz miguelraz commented Apr 21, 2023

gotta write down negative results, for science!

@Nilstrieb

#110524 (comment)

Please signal for rollup.

@rustbot
Copy link
Collaborator

rustbot commented Apr 21, 2023

r? @wesleywiser

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 21, 2023
@rustbot
Copy link
Collaborator

rustbot commented Apr 21, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@compiler-errors
Copy link
Member

I don't think we usually comment about failed perf attempts, since I would assume very hot code (e.g. trait selection) would be riddled with these comments -- though perhaps someone like @nnethercote knows about best practice here.

Comment on lines +293 to +294
// Note: Trying to Turn `duplicates` into a `SmallVec` for performance did not bear fruit, see
// https://github.com/rust-lang/rust/pull/110524
Copy link
Member

@compiler-errors compiler-errors Apr 21, 2023

Choose a reason for hiding this comment

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

Even if we do want to leave a note about perf sensitivity here, it should probably left as a comment on the field instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree.

@miguelraz
Copy link
Contributor Author

That sounds reasonable - I'm open to whatever maintainers feel is best.

@nnethercote
Copy link
Contributor

Comments about perf are good when the code has unexpected things in it. E.g. often code that is optimized is written in a way that is different to the obvious way, and a comment about that is helpful: "this code is hot, hence...".

This PR's comment is effectively saying "this code is written in the obvious way, not an optimized way". And @compiler-errors is right that we don't normally write comments like that. If there were many of them I think they'd get annoying. If there was a reason to think that this code would be especially hot, then maybe, but it doesn't strike me that way.

@Noratrieb
Copy link
Member

I just thought that it'd be quite likely that someone in the future might see this and see an opportunity to optimize. Only to waste their time. But if you think it's not worth it, fair.

@apiraino
Copy link
Contributor

Switching to waiting on author, IIUC indications were left for some changes? thanks

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 10, 2023
@compiler-errors
Copy link
Member

I'm gonna close this since it doesn't seem like adding this note is useful. Thanks for the PR!

@miguelraz miguelraz deleted the smallvec-simplify-note branch May 15, 2023 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants