-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Make weak item traversal deterministic #81393
Make weak item traversal deterministic #81393
Conversation
(No test added. The relevant test *is* ui/panic-handler/weak-lang-item.rs, and this change should make it less flaky.)
r? @estebank (rust-highfive has picked a reviewer for you, use r? to override) |
I do want a perf run on this b/c is adding a clone |
@@ -59,7 +59,7 @@ fn verify<'tcx>(tcx: TyCtxt<'tcx>, items: &lang_items::LanguageItems) { | |||
} | |||
} | |||
|
|||
for (name, &item) in WEAK_ITEMS_REFS.iter() { |
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.
WEAK_ITEMS_REFS
is already a StableMap
now, do you need to sort it? The only reason I am commenting on it is that I am slightly saddened by the new .clone()
(but I'll get over it).
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 PR is the one turning it into a StableMap
, and the sort is necessary to normalize the order, right?
(That is: If we don't normalize the order, then we don't get the benefit of this change, right?)
To be clear: I too am saddened by the .clone()
, and you might talk me into just using a different structure entirely. This seemed like the simplest fix, and if we're going to balk at doing it in this scenario, then maybe we should remove StableMap
entirely and figure out a different way to discourage people from using FxHashMap
?
@bors try @rust-timer queue |
Awaiting bors try build completion. |
⌛ Trying commit 4c5ede7 with merge 74e3708d92e199707b5b924507f40e55ca493b8f... |
☀️ Try build successful - checks-actions |
Queued 74e3708d92e199707b5b924507f40e55ca493b8f with parent f4eb5d9, future comparison URL. @rustbot label: +S-waiting-on-perf |
Finished benchmarking try commit (74e3708d92e199707b5b924507f40e55ca493b8f): 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 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 |
Perf looks fairly neutral to me. |
@bors r+ |
📌 Commit 4c5ede7 has been approved by |
🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened. |
☀️ Test successful - checks-actions |
Fix #81296.
(No test added. The relevant test is ui/panic-handler/weak-lang-item.rs, and this change should make it less flaky.)