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

Cache non-exhaustive separately from attributes #74887

Merged
merged 1 commit into from
Jul 29, 2020

Conversation

Mark-Simulacrum
Copy link
Member

This prevents cross-crate attribute loading from metadata just for non_exhaustive checking; cross-crate attribute loading implies disk reading and is relatively slow.

@rust-highfive
Copy link
Collaborator

r? @lcnr

(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 Jul 28, 2020
@Mark-Simulacrum
Copy link
Member Author

r? @petrochenkov perhaps

@bors try @rust-timer queue

Locally this is a 2-3% performance win on the smaller crates.

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@rust-highfive rust-highfive assigned petrochenkov and unassigned lcnr Jul 28, 2020
@bors
Copy link
Contributor

bors commented Jul 28, 2020

⌛ Trying commit e0fb1d8781bffcf6e4b691f16a781e31b24a1318 with merge 554bc32d7285b816c134bbb13ccde3771cb3987a...

src/librustc_middle/ty/mod.rs Outdated Show resolved Hide resolved
src/librustc_typeck/collect.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

Implementation LGTM, waiting on perf.

@petrochenkov petrochenkov added S-waiting-on-perf Status: Waiting on a perf run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 28, 2020
@bors
Copy link
Contributor

bors commented Jul 28, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 554bc32d7285b816c134bbb13ccde3771cb3987a (554bc32d7285b816c134bbb13ccde3771cb3987a)

@rust-timer
Copy link
Collaborator

Queued 554bc32d7285b816c134bbb13ccde3771cb3987a with parent 7b3a781, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (554bc32d7285b816c134bbb13ccde3771cb3987a): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@Mark-Simulacrum
Copy link
Member Author

Seems like a clear, if small, win.

@bors r=petrochenkov rollup=never

I suspect it would be good to do this for all other attribute loading, but that can be done in follow-up PRs. I think ideally we want to convert all meaningful attributes into bit flags or so rather than iterating arrays to check for presence.

@bors
Copy link
Contributor

bors commented Jul 29, 2020

📌 Commit 13ad232 has been approved by petrochenkov

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 29, 2020
@petrochenkov petrochenkov removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 29, 2020
@bors
Copy link
Contributor

bors commented Jul 29, 2020

⌛ Testing commit 13ad232 with merge 10c3757...

@bors
Copy link
Contributor

bors commented Jul 29, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: petrochenkov
Pushing 10c3757 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 29, 2020
@bors bors merged commit 10c3757 into rust-lang:master Jul 29, 2020
@Mark-Simulacrum Mark-Simulacrum deleted the cache-non-exhaustive branch August 3, 2020 21:46
@Mark-Simulacrum
Copy link
Member Author

This was a small perf improvement, as expected.

@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants