-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Invert control in struct_lint_level. #68725
Invert control in struct_lint_level. #68725
Conversation
Can someone run the benchmark to check that I've done this correctly, and we get at least the same performance boost as #67755? Cheers. |
@bors try @rust-timer queue |
Awaiting bors try build completion |
⌛ Trying commit b0f6d3d368a6c43548b445fdac1c61fe5235967d with merge a4d02393db8c5ea445507bef973e8f2ffb5ac6f7... |
src/librustc/lint.rs
Outdated
level: Level, | ||
src: LintSource, | ||
span: Option<MultiSpan>, | ||
decorate: Box<dyn for<'b> FnOnce(LintDiagnosticBuilder<'b>) + 'd>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not &dyn
here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
decorate
is FnOnce, so I can't call it through &dyn
which matters in cases like these, where a captured variable (in this case, overlap) is moved out of its environment. Using FnOnce
also avoids callers having rewrite their decorate
function to be Fn
when we'll only call it once anyway, giving them the most flexibility in terms of what kind of captures the closure can do.
I'll push changes that remove the Box anyway, to see if we get any sizeable performance gain. I was hoping the time for allocation of the box would be small enough in comparison to the previous time we were spending formatting, but given the results we're seeing, it couldn't hurt to give this a try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right; we might want to try the other changes first though before doing the unboxing to see what the full impact of unboxing would be.
☀️ Try build successful - checks-azure |
Queued a4d02393db8c5ea445507bef973e8f2ffb5ac6f7 with parent cdd41ea, future comparison URL. |
Finished benchmarking try commit a4d02393db8c5ea445507bef973e8f2ffb5ac6f7, comparison URL. |
Curiously, this PR brought some sizeable perf wins in some tests, but interestingly the set seems to be almost entirely disjoint from the wins in #67755. It would be good to make some of the tweaks I suggested, in particular avoiding the |
This haven't updated the BOX_POINTERS lint to make the message lazy https://github.com/rust-lang/rust/pull/67755/files#diff-1651ba5b47f9ffa648d59265d3f94becR115 . Might be other places that are missed as well? |
Oh, yeah, for sure -- here, for example, I've left the caller taking Hmm, actually, I can probably still make this function (and other calls) lazy by doing the function-specific things first, before handing off to the user. I'll give this a try. |
Changing the |
2c2aa5e
to
bc5bf23
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors try @rust-timer queue |
Awaiting bors try build completion |
…=<try> Invert control in struct_lint_level. Closes #67927 Changes the `struct_lint*` methods to take a `decorate` function instead of a message string. This decorate function is also responsible for eventually stashing, emitting or cancelling the diagnostic. If the lint was allowed after all, the decorate function is not run at all, saving us from spending time formatting messages (and potentially other expensive work) for lints don't end up being emitted. r? @Centril
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is progressing nicely. I found some more places with potential unnecessary allocations.
☀️ Try build successful - checks-azure |
Queued 2144ce4 with parent c58e09f, future comparison URL. |
Finished benchmarking try commit 2144ce4, comparison URL. |
Looks like we got the wins from #67755 as well now + some more. It's looking quite good. Let's adjust the other ones I pointed out as well for a final perf run and then let's merge this (we can perf test the boxing stuff separately). |
bc5bf23
to
a3600ab
Compare
- Make report_unsafe take decorate function - Remove span_lint, replacing calls with struct_span_lint, as caller is now responsible for emitting. - Remove lookup_and_emit, replacing with just lookup which takes a decorate function. - Remove span_lint_note, span_lint_help. These methods aren't easily made lazy as standalone methods, private, and unused. If this functionality is needed, to be lazy, they can easily be made into Fn(&mut DiagnosticBuilder) that are meant to be called _within_ the decorate function. - Rename lookup_and_emit_with_diagnostics to lookup_with_diagnostics to better reflect the fact that it doesn't emit for you.
- AnonymousParameters::check_trait_item - TypeAliasBounds::check_item - NonSnakeCase::check_snake_case
span_lint was removed. Callers should use the `lint` method now, and call `set_span` within the closure passed to this method.
bf54787
to
b959da2
Compare
@bors r=Centril |
📌 Commit b959da2 has been approved by |
…=Centril Invert control in struct_lint_level. Closes #67927 Changes the `struct_lint*` methods to take a `decorate` function instead of a message string. This decorate function is also responsible for eventually stashing, emitting or cancelling the diagnostic. If the lint was allowed after all, the decorate function is not run at all, saving us from spending time formatting messages (and potentially other expensive work) for lints that don't end up being emitted. r? @Centril
/// Sets the message passed in via `message`, then adds the span labels for you, before applying | ||
/// further modifications in `emit`. It's up to you to call emit(), stash(..), etc. within the | ||
/// `emit` method. If you don't need to do any additional processing, just use | ||
/// struct_generic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is struct_generic
, so I am confused by the last sentence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Opened #69077.
☀️ Test successful - checks-azure |
Rustup to rust-lang/rust#68725 Preparation for rust-lang/rust#68725 changelog: none
Pkgsrc changes: * Bump rust bootstrap version to 1.42.0, except for Darwin/i686 where the bootstrap is not (yet?) available. Upstream changes: Version 1.43.0 (2020-04-23) ========================== Language -------- - [Fixed using binary operations with `&{number}` (e.g. `&1.0`) not having the type inferred correctly.][68129] - [Attributes such as `#[cfg()]` can now be used on `if` expressions.][69201] **Syntax only changes** - [Allow `type Foo: Ord` syntactically.][69361] - [Fuse associated and extern items up to defaultness.][69194] - [Syntactically allow `self` in all `fn` contexts.][68764] - [Merge `fn` syntax + cleanup item parsing.][68728] - [`item` macro fragments can be interpolated into `trait`s, `impl`s, and `extern` blocks.][69366] For example, you may now write: ```rust macro_rules! mac_trait { ($i:item) => { trait T { $i } } } mac_trait! { fn foo() {} } ``` These are still rejected *semantically*, so you will likely receive an error but these changes can be seen and parsed by macros and conditional compilation. Compiler -------- - [You can now pass multiple lint flags to rustc to override the previous flags.][67885] For example; `rustc -D unused -A unused-variables` denies everything in the `unused` lint group except `unused-variables` which is explicitly allowed. However, passing `rustc -A unused-variables -D unused` denies everything in the `unused` lint group **including** `unused-variables` since the allow flag is specified before the deny flag (and therefore overridden). - [rustc will now prefer your system MinGW libraries over its bundled libraries if they are available on `windows-gnu`.][67429] - [rustc now buffers errors/warnings printed in JSON.][69227] Libraries --------- - [`Arc<[T; N]>`, `Box<[T; N]>`, and `Rc<[T; N]>`, now implement `TryFrom<Arc<[T]>>`,`TryFrom<Box<[T]>>`, and `TryFrom<Rc<[T]>>` respectively.][69538] **Note** These conversions are only available when `N` is `0..=32`. - [You can now use associated constants on floats and integers directly, rather than having to import the module.][68952] e.g. You can now write `u32::MAX` or `f32::NAN` with no imports. - [`u8::is_ascii` is now `const`.][68984] - [`String` now implements `AsMut<str>`.][68742] - [Added the `primitive` module to `std` and `core`.][67637] This module reexports Rust's primitive types. This is mainly useful in macros where you want avoid these types being shadowed. - [Relaxed some of the trait bounds on `HashMap` and `HashSet`.][67642] - [`string::FromUtf8Error` now implements `Clone + Eq`.][68738] Stabilized APIs --------------- - [`Once::is_completed`] - [`f32::LOG10_2`] - [`f32::LOG2_10`] - [`f64::LOG10_2`] - [`f64::LOG2_10`] - [`iter::once_with`] Cargo ----- - [You can now set config `[profile]`s in your `.cargo/config`, or through your environment.][cargo/7823] - [Cargo will now set `CARGO_BIN_EXE_<name>` pointing to a binary's executable path when running integration tests or benchmarks.][cargo/7697] `<name>` is the name of your binary as-is e.g. If you wanted the executable path for a binary named `my-program`you would use `env!("CARGO_BIN_EXE_my-program")`. Misc ---- - [Certain checks in the `const_err` lint were deemed unrelated to const evaluation][69185], and have been moved to the `unconditional_panic` and `arithmetic_overflow` lints. Compatibility Notes ------------------- - [Having trailing syntax in the `assert!` macro is now a hard error.][69548] This has been a warning since 1.36.0. - [Fixed `Self` not having the correctly inferred type.][69340] This incorrectly led to some instances being accepted, and now correctly emits a hard error. [69340]: rust-lang/rust#69340 Internal Only ------------- These changes provide no direct user facing benefits, but represent significant improvements to the internals and overall performance of `rustc` and related tools. - [All components are now built with `opt-level=3` instead of `2`.][67878] - [Improved how rustc generates drop code.][67332] - [Improved performance from `#[inline]`-ing certain hot functions.][69256] - [traits: preallocate 2 Vecs of known initial size][69022] - [Avoid exponential behaviour when relating types][68772] - [Skip `Drop` terminators for enum variants without drop glue][68943] - [Improve performance of coherence checks][68966] - [Deduplicate types in the generator witness][68672] - [Invert control in struct_lint_level.][68725] [67332]: rust-lang/rust#67332 [67429]: rust-lang/rust#67429 [67637]: rust-lang/rust#67637 [67642]: rust-lang/rust#67642 [67878]: rust-lang/rust#67878 [67885]: rust-lang/rust#67885 [68129]: rust-lang/rust#68129 [68672]: rust-lang/rust#68672 [68725]: rust-lang/rust#68725 [68728]: rust-lang/rust#68728 [68738]: rust-lang/rust#68738 [68742]: rust-lang/rust#68742 [68764]: rust-lang/rust#68764 [68772]: rust-lang/rust#68772 [68943]: rust-lang/rust#68943 [68952]: rust-lang/rust#68952 [68966]: rust-lang/rust#68966 [68984]: rust-lang/rust#68984 [69022]: rust-lang/rust#69022 [69185]: rust-lang/rust#69185 [69194]: rust-lang/rust#69194 [69201]: rust-lang/rust#69201 [69227]: rust-lang/rust#69227 [69548]: rust-lang/rust#69548 [69256]: rust-lang/rust#69256 [69361]: rust-lang/rust#69361 [69366]: rust-lang/rust#69366 [69538]: rust-lang/rust#69538 [cargo/7823]: rust-lang/cargo#7823 [cargo/7697]: rust-lang/cargo#7697 [`Once::is_completed`]: https://doc.rust-lang.org/std/sync/struct.Once.html#method.is_completed [`f32::LOG10_2`]: https://doc.rust-lang.org/std/f32/consts/constant.LOG10_2.html [`f32::LOG2_10`]: https://doc.rust-lang.org/std/f32/consts/constant.LOG2_10.html [`f64::LOG10_2`]: https://doc.rust-lang.org/std/f64/consts/constant.LOG10_2.html [`f64::LOG2_10`]: https://doc.rust-lang.org/std/f64/consts/constant.LOG2_10.html [`iter::once_with`]: https://doc.rust-lang.org/std/iter/fn.once_with.html
Closes #67927
Changes the
struct_lint*
methods to take adecorate
function instead of a message string. This decorate function is also responsible for eventually stashing, emitting or cancelling the diagnostic. If the lint was allowed after all, the decorate function is not run at all, saving us from spending time formatting messages (and potentially other expensive work) for lints that don't end up being emitted.r? @Centril