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.

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

Use FxHashSet instead of Vec for well formed tys #88771

Merged
merged 2 commits into from
Sep 12, 2021
Merged

Use FxHashSet instead of Vec for well formed tys #88771

merged 2 commits into from
Sep 12, 2021

Conversation

jackh726
Copy link
Member

@jackh726 jackh726 commented Sep 9, 2021

Trying to recover perf from #88312

r? @ghost

@jackh726
Copy link
Member Author

jackh726 commented Sep 9, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 9, 2021
@bors
Copy link
Contributor

bors commented Sep 9, 2021

⌛ Trying commit ba67d85ae338bbb4ab1b840e9bf57883cad1efdd with merge 05ee7be89fecd02defa835ceae7c6c57e8582a12...

@bors
Copy link
Contributor

bors commented Sep 9, 2021

☀️ Try build successful - checks-actions
Build commit: 05ee7be89fecd02defa835ceae7c6c57e8582a12 (05ee7be89fecd02defa835ceae7c6c57e8582a12)

@rust-timer
Copy link
Collaborator

Queued 05ee7be89fecd02defa835ceae7c6c57e8582a12 with parent 626649f, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (05ee7be89fecd02defa835ceae7c6c57e8582a12): comparison url.

Summary: This change led to large relevant improvements 🎉 in compiler performance.

  • Large improvement in instruction counts (up to -2.6% on full builds of externs)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 9, 2021
@jackh726
Copy link
Member Author

jackh726 commented Sep 9, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 9, 2021
@bors
Copy link
Contributor

bors commented Sep 9, 2021

⌛ Trying commit 8e7613f with merge 4508123e015a7afc057ce1b1b417413dc7c45059...

@bors
Copy link
Contributor

bors commented Sep 9, 2021

☀️ Try build successful - checks-actions
Build commit: 4508123e015a7afc057ce1b1b417413dc7c45059 (4508123e015a7afc057ce1b1b417413dc7c45059)

@rust-timer
Copy link
Collaborator

Queued 4508123e015a7afc057ce1b1b417413dc7c45059 with parent 497ee32, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4508123e015a7afc057ce1b1b417413dc7c45059): comparison url.

Summary: This change led to large relevant improvements 🎉 in compiler performance.

  • Large improvement in instruction counts (up to -2.6% on full builds of externs)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 9, 2021
@jackh726 jackh726 changed the title [perf] Use FxHashSet instead of Vec for well formed tys Use FxHashSet instead of Vec for well formed tys Sep 9, 2021
@jackh726
Copy link
Member Author

jackh726 commented Sep 9, 2021

Doesn't win back all the perf lost from #88312, but is better. I'm not sure that there's a way to win more perf without changing the approach completely. So, I think this is probably worth merging and accepting the difference.

cc @rylev regarding perf
cc @nikomatsakis who might want to steal this review

r? rust-lang/compiler

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 10, 2021

📌 Commit 8e7613f has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Sep 10, 2021

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 10, 2021
@bors
Copy link
Contributor

bors commented Sep 12, 2021

⌛ Testing commit 8e7613f with merge 9ef27bf...

@bors
Copy link
Contributor

bors commented Sep 12, 2021

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 9ef27bf to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 12, 2021
@bors bors merged commit 9ef27bf into rust-lang:master Sep 12, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 12, 2021
@jackh726 jackh726 deleted the wf_tys_set branch September 12, 2021 14:55
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9ef27bf): comparison url.

Summary: This change led to large relevant improvements 🎉 in compiler performance.

  • Large improvement in instruction counts (up to -2.6% on full builds of externs)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@jackh726 jackh726 mentioned this pull request Sep 12, 2021
Merged
eddyb
eddyb reviewed Sep 18, 2021
View reviewed changes
compiler/rustc_trait_selection/src/infer.rs
Comment on lines -213 to +220
fn_sig_tys: &[Ty<'tcx>],
fn_sig_tys: FxHashSet<Ty<'tcx>>,
body_id: hir::HirId,
span: Span,
) {
debug!("add_implied_bounds()");

for &ty in fn_sig_tys {
for ty in fn_sig_tys {
Copy link
Member

Choose a reason for hiding this comment

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

This is iterating on a FxHashSet<Ty<'tcx>>, which will depend on ASLR (random memory allocation order) and the results could be observable (e.g. potentially in diagnostics order).

I wonder if we should ban iterating FxHash{Set,Map} and force the use of FxIndex{Set,Map} instead (since presumably the insertion order is deterministic).

Copy link
Member

Choose a reason for hiding this comment

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

See also #63713 (comment) - we should probably use internal lints to force this harder.

Copy link
Contributor

@lcnr lcnr Sep 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

we should ban iterating FxHash{Set,Map}

YES! I think recommending FxIndex{Set,Map} is probably the right choice, using an internal lint when using a FxIndexSet in a potentially nondet way. Now if the order doesn't matter, e.g. checking for existence without any sideeffects, this lint could be explicitly allowed.

Not sure what's the best way to write such a lint though 😅

@jackh726 jackh726 restored the wf_tys_set branch March 12, 2022 18:30
@jackh726 jackh726 deleted the wf_tys_set branch March 12, 2022 18:35
@jackh726 jackh726 restored the wf_tys_set branch March 12, 2022 18:42
@jackh726 jackh726 deleted the wf_tys_set branch March 12, 2022 18:53
@jieyouxu jieyouxu mentioned this pull request Aug 14, 2024
Open
81 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddyb eddyb eddyb left review comments

@lcnr lcnr lcnr left review comments

Assignees

@oli-obk oli-obk

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
Milestone
1.57.0
Development

Successfully merging this pull request may close these issues.
8 participants
@jackh726 @rust-timer @bors @nikomatsakis @eddyb @lcnr @oli-obk @rustbot