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

Introduce UnordMap, UnordSet, and UnordBag (MCP 533) #102698

Merged
merged 1 commit into from
Oct 29, 2022
Merged

Introduce UnordMap, UnordSet, and UnordBag (MCP 533) #102698

merged 1 commit into from
Oct 29, 2022

Conversation

michaelwoerister
Copy link
Member

This is the start of implementing MCP 533.

I followed @eddyb's suggestion of naming the collection types Unord(Map/Set/Bag) which is a bit easier to type than Unordered(Map/Set/Bag)

r? @eddyb

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 5, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 5, 2022
@bors
Copy link
Contributor

bors commented Oct 19, 2022

☔ The latest upstream changes (presumably #103228) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@michaelwoerister
Introduce UnordMap, UnordSet, and UnordBag (see MCP 533)
9117ea9 
MCP 533: rust-lang/compiler-team#533

Also, as an example, substitute UnordMap for FxHashMap in
used_trait_imports query result.
@michaelwoerister
Copy link
Member Author

r? compiler

@michaelwoerister
Copy link
Member Author

r? compiler

Hm, that didn't seem to work...

@michaelwoerister
Copy link
Member Author

Re-assignment doesn't to work right now. @lcnr, do you want to take a look at this? You seemed to have some interest on Zulip a few months ago.

@michaelwoerister
Copy link
Member Author

@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 Oct 28, 2022
@bors
Copy link
Contributor

bors commented Oct 28, 2022

⌛ Trying commit 9117ea9 with merge 8f2030cfc7fe4c36f7e41a7593c5d23356c06f5c...

@lcnr
Copy link
Contributor

lcnr commented Oct 28, 2022

r? @lcnr

@rustbot rustbot assigned lcnr and unassigned eddyb Oct 28, 2022
@lcnr
Copy link
Contributor

lcnr commented Oct 28, 2022

r=me after perf. Is the end goal here to completely remove FxHashSet/Map from the compiler?

@michaelwoerister
Copy link
Member Author

The MCP only states the goal of removing the HashStable implementations, which means that they can't be used in query keys or results anymore. But I'd be fine with removing them altogether.

@michaelwoerister michaelwoerister mentioned this pull request Oct 28, 2022
Closed
@bors
Copy link
Contributor

bors commented Oct 28, 2022

☀️ Try build successful - checks-actions
Build commit: 8f2030cfc7fe4c36f7e41a7593c5d23356c06f5c (8f2030cfc7fe4c36f7e41a7593c5d23356c06f5c)

1 similar comment
@bors
Copy link
Contributor

bors commented Oct 28, 2022

☀️ Try build successful - checks-actions
Build commit: 8f2030cfc7fe4c36f7e41a7593c5d23356c06f5c (8f2030cfc7fe4c36f7e41a7593c5d23356c06f5c)

@rust-timer
Copy link
Collaborator

Queued 8f2030cfc7fe4c36f7e41a7593c5d23356c06f5c with parent cdd7afe, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8f2030cfc7fe4c36f7e41a7593c5d23356c06f5c): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

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 may lead to changes in compiler perf.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 0.4% [0.4%, 0.4%] 2
Improvements ✅
(primary)		 -0.7% [-0.7%, -0.7%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.7% [-0.7%, -0.7%] 1

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Footnotes

  1. the arithmetic mean of the percent change ↩

  2. number of relevant changes ↩

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 28, 2022
@michaelwoerister
Copy link
Member Author

Perf looks unchanged -- I don't think the two minor changes are real.

@bors r=lncr

@bors
Copy link
Contributor

bors commented Oct 28, 2022

📌 Commit 9117ea9 has been approved by lncr

It is now in the queue for this repository.

@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 Oct 28, 2022
@michaelwoerister
Copy link
Member Author

Thanks for the review, @lncr!

@bors
Copy link
Contributor

bors commented Oct 29, 2022

⌛ Testing commit 9117ea9 with merge 607878d...

@bors
Copy link
Contributor

bors commented Oct 29, 2022

☀️ Test successful - checks-actions
Approved by: lncr
Pushing 607878d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 29, 2022
@bors bors merged commit 607878d into rust-lang:master Oct 29, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 29, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (607878d): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 1.3% [1.2%, 1.4%] 2
Regressions ❌
(secondary)		 1.8% [0.4%, 4.1%] 14
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 1.3% [1.2%, 1.4%] 2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 3.8% [3.5%, 4.2%] 2
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 2.2% [2.0%, 2.3%] 6
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

@rustbot rustbot added the perf-regression Performance regression. label Oct 29, 2022
@lqd
Copy link
Member

lqd commented Oct 29, 2022

These benchmarks are currently noisy so I think we're good here, @rustbot label: +perf-regression-triaged

image

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Oct 29, 2022
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
@bors
Auto merge of rust-lang#102698 - michaelwoerister:unord-collections, …
fc6741c 
…r=lncr

Introduce UnordMap, UnordSet, and UnordBag (MCP 533)

This is the start of implementing [MCP 533](rust-lang/compiler-team#533).

I followed `@eddyb's` suggestion of naming the collection types `Unord(Map/Set/Bag)` which is a bit easier to type than `Unordered(Map/Set/Bag)`

r? `@eddyb`
This was referenced Apr 19, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcnr lcnr lcnr approved these changes

Assignees

@lcnr lcnr

Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.66.0
Development

Successfully merging this pull request may close these issues.
9 participants
@michaelwoerister @bors @rust-log-analyzer @rust-timer @lcnr @lqd @eddyb @rust-highfive @rustbot