CrateLocator refactorings

[bjorn3]

This makes the CrateLocator a lot cleaner IMHO and much more self-contained. The last commit removes extra_filename from the crate metadata. This is an insta-stable change as it allows a crate like libfoo-abc.rlib to be used as dependency and then be renamed as libfoo-bcd.rlib while still being found as indirect dependency. This may reduce performance when there are a lot of versions of the same crate available as the extra filename won't be used to do an early rejection of crates before trying to load metadata, but it makes the logic to find the right filename a lot cleaner.

[rust-highfive]

r? @wesleywiser

(rust-highfive has picked a reviewer for you, use r? to override)

[bjorn3]

I'm guessing that this failure was caused by the extra filename changes.

[petrochenkov]

I'm going to review because "continue refactoring crate locator" is an item from my work queue on which I was going to work myself (cc #56935 (comment)).

[bjorn3]

I forgot an ! in the only_needs_metadata calculation.

[bjorn3]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit 72af6d865b37f99524b00b6704622b446d74fefd with merge 9fb7be546f93c5a3ff56d4d58c89247eceba6c68...

[bjorn3]

I think I will revert "Don't pass Session to CrateError::report". Unlike with CrateLocator I don't think there are much benefits to CrateError::report not taking a Session after all.

[bors]

☀️ Try build successful - checks-actions
Build commit: 9fb7be546f93c5a3ff56d4d58c89247eceba6c68 (9fb7be546f93c5a3ff56d4d58c89247eceba6c68)

[rust-timer]

Queued 9fb7be546f93c5a3ff56d4d58c89247eceba6c68 with parent 76d18cf, future comparison URL.

[petrochenkov]

This may reduce performance when there are a lot of versions of the same crate available as the extra filename won't be used to do an early rejection of crates before trying to load metadata, but it makes the logic to find the right filename a lot cleaner.

As far as I understand, the crates will now be (usually) rejected via hash kept in metadata instead of the extra filename piece (which is also some kind of hash if cargo is used).

Besides many versions of the same crate the perf regression can hit many crates with the same prefix like librustc-abcd.rlib/librustc_foo-efgh.rlib/librustc_bar-ijkl.rlib/etc, and the search for the library rustc will now need to open all the librustc* crate files and read hashes from them, because we no longer rely on knowing -abcd.

[bjorn3]

As far as I understand, the crates will now be (usually) rejected via hash kept in metadata instead of the extra filename piece (which is also some kind of hash if cargo is used).

That and the file prefix depending in the crate name.

and the search for the library rustc will now need to open all the librustc* crate files and read hashes from them, because we no longer rely on knowing -abcd.

Didn't think about that. Would have been nice if the extra filename was required to start with - to clearly separate the crate name and extra filenane. It is now a breaking change to change this unfortunately even though cargo always does this.

[rust-timer]

Finished benchmarking try commit (9fb7be546f93c5a3ff56d4d58c89247eceba6c68): comparison url.

Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.

• Moderate improvement in instruction counts (up to -0.6% on incr-full builds of coercions)
• Very small regression in instruction counts (up to 0.3% on incr-patched: println builds of deep-vector)

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.

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 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-review -S-waiting-on-perf +perf-regression

[bjorn3]

I think the regression is neglectable.

and the search for the library rustc will now need to open all the librustc* crate files and read hashes from them, because we no longer rely on knowing -abcd.

I think a perf regression for this specific case can be prevented by checking with - as extra prefix first before checking without extra prefix. Assuming that the extra filename starts with a - most of the time this would prevent most of the unnecessary lookup. It would still do the unnecessary lookup if the extra filenames don't start with a -. Speaking of which I actually don't think this commit changes behavior. If the file is not found with the specified extra filename as extra prefix, it will already try again without extra prefix, which matches all crates with the right crate name independent of the extra filename.

[petrochenkov]

Speaking of which I actually don't think this commit changes behavior. If the file is not found with the specified extra filename as extra prefix, it will already try again without extra prefix, which matches all crates with the right crate name independent of the extra filename.

It can change behavior in theory if the file is found with the specified extra prefix.
With the old logic we stop searching, with the new logic we continue searching and can potentially report "multiple candidates" errors. This is not a very realistic case because for the error to be reported hashes must match, but the extra search can take some time.

[petrochenkov]

I'm still not sure about no longer keeping the -C extra_filename hints, they are cheap to keep and can potentially speed up crate search.
I need a second opinion, from cargo people maybe. cc @ehuss @alexcrichton

[bjorn3]

This is not a very realistic case because for the error to be reported hashes must match, but the extra search can take some time.

Doesn't the search stop as soon a crate with the right SVH was found if a required SVH was specified?

[petrochenkov]

@bjorn3

Doesn't the search stop as soon a crate with the right SVH was found if a required SVH was specified?

It does not? I've just looked through the code, but haven't noticed any such early return logic.

[bjorn3]

I will squash the reverts after review.

[alexcrichton]

I don't know the specifics of what this PR is about, and I can't find a necessarily clear question that's being asked, but I can try to offer some thoughts anyway. From the comment above it sounds like previously rustc is taking -Cextra-filename into account when searching for dependencies, first looking at rlibs with a matching -Cextra-filename and succeeding quickly if found. If that's the case and it's now being removed, I would caution against anything that would cause more candidate rlibs to be searched. Cargo dumps everything into deps for dependencies, and with rebuilds across features and Rust versions it's not uncommon for development directories to have hundreds of copies of libfoo-xxxxx.rlib. If all compilations have to search through all those rlibs to reject them that means that incremental compiles are going to be that much slower. This sort of test case is unlikely to really be exercised on perf.r-l.o since it's always working with pristine directories and it's relatively rare that multiple libfoo-xxxx.rlib files will appear in one crate graph.

[bjorn3]

I reverted the extra-filename commit. While I think it would be nice to drop extra-filename, the downsides don't weight up against it. At least without some kind of cache to map from svh to filename, but that introduces much more complexity elsewhere.

[petrochenkov]

r=me with commits appropriately squashed.

[petrochenkov]

@bors rollup=maybe

[bjorn3]

Squashed all reverts and the bugfix commit.

@bors r=petrochenkov

[bors]

📌 Commit ff98cb6 has been approved by petrochenkov

[bors]

⌛ Testing commit ff98cb6 with merge d295e36...

[bors]

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing d295e36 to master...