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

perf: Delay formatting of lint messages until they are know to be used #67755

Closed
wants to merge 1 commit into from

Conversation

Marwes
Copy link
Contributor

@Marwes Marwes commented Dec 31, 2019

Found that (touch+) check builds in one of my crates spent 5% of the time just
formatting types for the sake of the BoxPointers lint, only for the
strings to get thrown away immediately since it is an allow lint.

This does the bare minimum to instead pass a fmt::Display instance to
the lint so that msg may only be created if it is needed.

Unsure if this is the desired way to solve this problem so I didn't go
through the lint system throughly to see if there were more places that
should take/pass format_args! instances instead of using format!.

Found `check` builts in one of my crates spent 5% of the time just
formatting types for the sake of the `BoxPointers` lint, only for the
strings to get thrown away immediately since it is an `allow` lint.

This does the bare minimum to instead pass a `fmt::Display` instance to
the lint so that `msg` may only be created if it is needed.

Unsure if this is the desired way to solve this problem so I didn't go
through the lint system throughly to see if there were more places that
should take/pass `format_args!` instances instead of using `format!`.
@rust-highfive
Copy link
Collaborator

r? @eddyb

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 31, 2019
@jonas-schievink
Copy link
Contributor

@bors try

@rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Dec 31, 2019

⌛ Trying commit 055ce62 with merge 228ed11...

bors added a commit that referenced this pull request Dec 31, 2019
perf: Delay formatting of lint messages until they are know to be used

Found that (touch+) `check` builds in one of my crates spent 5% of the time just
formatting types for the sake of the `BoxPointers` lint, only for the
strings to get thrown away immediately since it is an `allow` lint.

This does the bare minimum to instead pass a `fmt::Display` instance to
the lint so that `msg` may only be created if it is needed.

Unsure if this is the desired way to solve this problem so I didn't go
through the lint system throughly to see if there were more places that
should take/pass `format_args!` instances instead of using `format!`.
level: Level,
src: LintSource,
span: Option<MultiSpan>,
msg: &dyn std::fmt::Display,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... this does nothing though to prevent similar mistakes in the future.

Perhaps we should:

  • Set the initial message to "" (so there should be no allocation).

  • Invert control: Instead of returning a DiagnosticBuilder<'_>, we take in a function impl Fn(&mut DiagnosticBuilder<'_>) which allows the caller to make modifications, but ultimately this is never called on Level::Allow, thereby avoiding allocations that follow in e.g.

    let binding = match binding_annot {
    hir::BindingAnnotation::Unannotated => None,
    hir::BindingAnnotation::Mutable => Some("mut"),
    hir::BindingAnnotation::Ref => Some("ref"),
    hir::BindingAnnotation::RefMut => Some("ref mut"),
    };
    let ident = if let Some(binding) = binding {
    format!("{} {}", binding, ident)
    } else {
    ident.to_string()
    };

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 idea of changing the api surface to always defer evaluation of the message would be a relatively painless big win.

Copy link
Contributor

Choose a reason for hiding this comment

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

Filed #67927.

@bors
Copy link
Contributor

bors commented Dec 31, 2019

☀️ Try build successful - checks-azure
Build commit: 228ed11 (228ed1118a30b6bfb1f725f66259681e8d2c2cf8)

@rust-timer
Copy link
Collaborator

Queued 228ed11 with parent 71bb0ff, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 228ed11, comparison URL.

@Centril
Copy link
Contributor

Centril commented Dec 31, 2019

@bors rollup=never

cc @oli-obk

@oli-obk
Copy link
Contributor

oli-obk commented Jan 2, 2020

cc @rust-lang/wg-diagnostics

r? @oli-obk

@Dylan-DPC-zz
Copy link

@Marwes can you rebase this? Thanks

@Dylan-DPC-zz Dylan-DPC-zz 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 Jan 17, 2020
@Marwes
Copy link
Contributor Author

Marwes commented Jan 17, 2020

It looks like there is in progress work to further improve this ( #67927 ) so maybe this should just be closed?

@Dylan-DPC-zz
Copy link

Closing it then. Thanks :)

@estebank
Copy link
Contributor

I feel like merging this PR is a reasonable stop gap measure, if you wish to put in the work to clean it up @Marwes.

@Marwes
Copy link
Contributor Author

Marwes commented Jan 17, 2020

Looks like things have changed quite a bit (files deleted/moved) so I will leave this alone. Got other performance PRs that needs investigation anyways.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants