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

Replace CStore::stable_crate_ids with a fed query. #108390

Closed
wants to merge 1 commit into from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Feb 23, 2023

This avoids more state in the CStore, but is a bit odd from the feeding perspective, as there's no "creation" of the query key (the hash). But since it is kinda global information, I believe this is sound. After all, you can't create more crate ids after the resolver.

r? @petrochenkov

@rustbot
Copy link
Collaborator

rustbot commented Feb 23, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

Please see the contribution instructions for more information.

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 23, 2023

@bors try @rust-timer queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 23, 2023
@rust-timer

This comment has been minimized.

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

bors commented Feb 23, 2023

⌛ Trying commit 5c804cf with merge 74a0df9ce71855919a3852d28d49847ab3206ed1...

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 23, 2023

Not sure if a query is the best design for this. It is more similar to Definitions, as you'd create a CrateNum as an interned StableCrateId. This doesn't really work, as we need to be able to crate the CrateNum before having loaded the StableCrateId afaict. Well, at least for the local crate, which is special I guess.

Ok, enough rubberducking, I'll check if an interning scheme makes more sense

@cjgillot
Copy link
Contributor

An interner would make more sense.
The query system never sees the integer value of a CrateNum, only the StableCrateId. The query stable_crate_id_to_crate_num_raw is the identity function.
In that respect, CrateNum is just a shorthand for StableCrateId, like LocalDefId is a shorthand for a DefPathHash.

@bors
Copy link
Contributor

bors commented Feb 23, 2023

☀️ Try build successful - checks-actions
Build commit: 74a0df9ce71855919a3852d28d49847ab3206ed1 (74a0df9ce71855919a3852d28d49847ab3206ed1)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (74a0df9ce71855919a3852d28d49847ab3206ed1): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -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.

mean range count
Regressions ❌
(primary)
0.3% [0.2%, 0.9%] 74
Regressions ❌
(secondary)
0.5% [0.2%, 1.1%] 15
Improvements ✅
(primary)
-1.0% [-1.1%, -0.9%] 2
Improvements ✅
(secondary)
-2.2% [-2.5%, -2.0%] 6
All ❌✅ (primary) 0.3% [-1.1%, 0.9%] 76

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)
2.8% [2.8%, 2.8%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.3% [-2.3%, -2.3%] 1
All ❌✅ (primary) - - 0

Cycles

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

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Feb 23, 2023
@oli-obk oli-obk closed this Feb 24, 2023
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 21, 2023
Eagerly intern and check CrateNum/StableCrateId collisions

r? `@cjgillot`

It seems better to check things ahead of time than checking them afterwards.
The [previous version](rust-lang#108390) was a bit nonsensical, so this addresses the feedback
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 22, 2023
Eagerly intern and check CrateNum/StableCrateId collisions

r? ``@cjgillot``

It seems better to check things ahead of time than checking them afterwards.
The [previous version](rust-lang#108390) was a bit nonsensical, so this addresses the feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants