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

rustc: Use tcx.used_crates(()) more #124976

Merged
merged 1 commit into from May 23, 2024
Merged

Conversation

petrochenkov
Copy link
Contributor

And explain when it should be used.

Addresses comments from #121167.

@rustbot
Copy link
Collaborator

rustbot commented May 10, 2024

r? @compiler-errors

rustbot has assigned @compiler-errors.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels May 10, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 10, 2024

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

// FIXME: This is currently only used for collecting lang items, but should be used instead of
// `crates` in most other cases too.
// Should be used to maintain observable language behavior, for example when collecting lang
// items or impls from all crates, or collecting libraries to link.
query used_crates(_: ()) -> &'tcx [CrateNum] {
Copy link
Member

Choose a reason for hiding this comment

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

"used" kind of sounds like if I add a crate as a dependency but don't use it, it doesn't show up.

Maybe required_crates?

Also all_crates could maybe need a more scary name, like crates_including_speculative or so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

crates_including_speculative looks fine to me.
Not sure about required_crates.

Ideally I'd turn the used bool in crate loader into a 3-variant enum:

enum WhatCrateIsLoadedFor {
  ForRealThings,
  // Need to be encoded into metadata in any case
  ForDocLinksAndDiagnosticsOnly,
  // It may be possible to drop these ones from metadata, if we clean their traces from source map somehow
  ForDiagnosticsOnly, 
}

Maybe even merge it with existing enum CrateDepKind which is sort of similar.

Let's keep its pre-existing naming for now.

@compiler-errors
Copy link
Member

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2024
And explain when it should be used.
@oli-obk
Copy link
Contributor

oli-obk commented May 22, 2024

@bors try @rust-timer queue

@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 May 22, 2024
@bors
Copy link
Contributor

bors commented May 22, 2024

⌛ Trying commit 0f35ba8 with merge 0cba80b...

bors added a commit to rust-lang-ci/rust that referenced this pull request May 22, 2024
rustc: Use `tcx.used_crates(())` more

And explain when it should be used.

Addresses comments from rust-lang#121167.
@petrochenkov
Copy link
Contributor Author

I was just going to push an updated version.
@bors r-
Not sure if it's possible to cancel bors try.

@compiler-errors
Copy link
Member

If you push, then it'll cancel the try.

@petrochenkov petrochenkov reopened this May 22, 2024
@petrochenkov
Copy link
Contributor Author

Also, in practice speculative crates are not common, so I don't expect much difference in perf.

@petrochenkov
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@petrochenkov petrochenkov removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 22, 2024
@bors
Copy link
Contributor

bors commented May 22, 2024

⌛ Trying commit 711338b with merge 20a9c4a...

bors added a commit to rust-lang-ci/rust that referenced this pull request May 22, 2024
rustc: Use `tcx.used_crates(())` more

And explain when it should be used.

Addresses comments from rust-lang#121167.
@bors
Copy link
Contributor

bors commented May 22, 2024

☀️ Try build successful - checks-actions
Build commit: 20a9c4a (20a9c4a92b1d3b63271afea003f1ad71c96ca23a)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (20a9c4a): comparison URL.

Overall result: no relevant changes - 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-perf -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (primary 1.3%, secondary -1.5%)

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

Cycles

Results (secondary 2.2%)

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.2%, 2.2%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 672.314s -> 672.229s (-0.01%)
Artifact size: 315.50 MiB -> 316.18 MiB (0.22%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 22, 2024
@petrochenkov
Copy link
Contributor Author

petrochenkov commented May 22, 2024

@bors rollup=maybe
@rustbot ready

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 22, 2024
@petrochenkov
Copy link
Contributor Author

@bors rollup=maybe

@oli-obk
Copy link
Contributor

oli-obk commented May 23, 2024

r? @oli-obk

@bors r+

@bors
Copy link
Contributor

bors commented May 23, 2024

📌 Commit 711338b has been approved by oli-obk

It is now in the queue for this repository.

@rustbot rustbot assigned oli-obk and unassigned compiler-errors May 23, 2024
@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 May 23, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request May 23, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#124297 (Allow coercing functions whose signature differs in opaque types in their defining scope into a shared function pointer type)
 - rust-lang#124516 (Allow monomorphization time const eval failures if the cause is a type layout issue)
 - rust-lang#124976 (rustc: Use `tcx.used_crates(())` more)
 - rust-lang#125210 (Cleanup: Fix up some diagnostics)
 - rust-lang#125409 (Rename `FrameworkOnlyWindows` to `RawDylibOnlyWindows`)
 - rust-lang#125416 (Use correct param-env in `MissingCopyImplementations`)
 - rust-lang#125421 (Rewrite `core-no-oom-handling`, `issue-24445` and `issue-38237` `run-make` tests to new `rmake.rs` format)
 - rust-lang#125438 (Remove unneeded string conversion)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit eda4a35 into rust-lang:master May 23, 2024
7 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 23, 2024
Rollup merge of rust-lang#124976 - petrochenkov:usedcrates, r=oli-obk

rustc: Use `tcx.used_crates(())` more

And explain when it should be used.

Addresses comments from rust-lang#121167.
error: requires `sized` lang_item

error: aborting due to 4 previous errors
error: aborting due to 2 previous errors
Copy link
Member

@lqd lqd May 24, 2024

Choose a reason for hiding this comment

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

Was this diagnostics change expected? (I'm analyzing the regressions caused by this PR)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, during my review I assumed this is the weak lang item check that now only looks at crates that are actually used, so we aren't speculatively looking at weak lang items of unused crates anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I see libcore is loaded speculatively in this test (missing_core in fn resolve_crate) and this error happens due to (weak) lang items in core.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

None yet

8 participants