[Experiment] Replace crate-hash with simple timestamp. #94852
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
the
T-compiler
This comment has been minimized.
This comment has been minimized.
michaelwoerister
force-pushed
the
no-crate-hash
branch
from
a58b166 to
59f269c
Compare
|
It looks like it's not that trivial to make this pass all tests. But let's see if we can get a build for perf.rlo. @bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit 59f269c with merge 9fc5ba51a036e587a8b4be991f470bd2d268a87b... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
This would make builds non-reproducible and if anything increase the risk of multiple crates having the same crate hash due to being either built very close to each other on the same machine, multiple machines having the same time or the time going backwards due to eg an empty cmos battery or ntp adjustments. |
|
☀️ Try build successful - checks-actions |
|
Queued 9fc5ba51a036e587a8b4be991f470bd2d268a87b with parent af8604f, future comparison URL. |
|
Sure, this is just an experiment to see the maximum impact something like this can have on incr comp performance. Reproducible builds are high priority. An actual implementation would have to take that into account. I'm starting to wonder though if we need the crate hash in the current form at all. |
The crate hash is what ensures that replacing a dependency always forces the dependent crate to be recompiled. (Something more finegrained may make it impossible to eg add extra query dependencies without causing breakage) It is also essential for incr comp to invalidate queries based on changed crate metadata. |
|
I am not sure this perf run will measure what we want it to measure. |
|
Finished benchmarking commit (9fc5ba51a036e587a8b4be991f470bd2d268a87b): comparison url. Summary: This benchmark run shows 44 relevant improvements 🎉 but 77 relevant regressions 😿 to instruction counts.
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 try perf run, please indicate this with @bors rollup=never |
rustbot
added
perf-regression
cjgillot
reviewed
Mar 12, 2022
| @@ -776,7 +776,6 @@ impl<T> MaybeOwner<T> { | |||
| #[derive(Debug)] | |||
| pub struct Crate<'hir> { | |||
| pub owners: IndexVec<LocalDefId, MaybeOwner<&'hir OwnerInfo<'hir>>>, | |||
| pub hir_hash: Fingerprint, | |||
There was a problem hiding this comment.
Why this change? This may force re-executions that are unrelated to crate_hash.
There was a problem hiding this comment.
Yes, that's true, making the hir_crate query no_hash is an unrelated change. Although from the performance results it looks like it does not make much of difference?
|
Let me try to elaborate where this might be going. Some background first: With respect to incremental compilation, the crate hash is used to determine if something from crate metadata can have changed at all. I.e. it is the root dependency node for all queries that read from the metadata of a given upstream crate. The requirement for it is thus that if anything in crate metadata changes, it too needs to reflect that change. Right now crate hash is computed from a set of things that supposedly makes it fulfill this requirement: the HIR, (some) commandline args, the file names of the current crate's source files after remapping, etc. There are some problems with this approach though:
So, what I'm trying to figure out is: Can we make this cheaper, simpler, and more robust at the same time. I see several options:
I think that option (2) is by far the most promising. Option (1) is a distant second because of the performance implications. Option (3) might be even worse than the current approach due to the macro problem. Consider yourselves invited to try and poke holes in my reasoning |
|
I agree with your reasoning.
Remark: we already partially re-fingerprint data we are loading from disk even when the
I just posted #94878 as a call for participation to investigate this. I am available to mentor it, you are welcome to join. "Con 1" may just be the necessary cost for our peace of mind here. We will have to measure the perf effect of course. One tricky thing will be to have the stable hasher and the metadata encoding agree:
In general, proc macros are not deterministic, they could even emit a time stamp. Only regular macros are. In consequence, we need to hash the post-expansion AST, or to add the output of each proc-macro expansion to the crate hash.
I agree with the ordering of solutions, and I haven't found holes yet :). |
Yes we do. In any case the crate metadata already contains the crate hash for all dependencies so hashing the crate metadata is enough. Note that hashing the crate metadata is not enough for proc macros. The crate metadata can be unchanged despite the actual implementation changing. If the implementation changes the crate hash needs to change to ensure that we don't forget to expand the proc macro invocations again. |
That's the main uncertainty I see with this approach, yes. It might be OK to hash the bytes verbatim because the tables needed for decoding the indices are part of the same value - but I'm not certain that that covers everything. The approach in #94878 should not have that issue though. |
|
As a first step, we could reuse what @Aaron1011 did in #87896 (= just sha256 the binary blob), and refine from there if the perf effect is too large. |
|
Hashing the binary blob is what I suggested above. However, it does potentially have the problem you mentioned. But if we also feed the crate hashes of all upstream crates into the hash, we will have transitively hashed all the data that can be used to do the defid->defpathhash mapping and thus should be able to detect any relevant change. I haven't had time to really think through this though - so take it with a grain of salt :) |
@bjorn3, can you elaborate your reasoning here? I don't disagree but I'd be interested in why you think it's necessary.
Ah yes, that's true.
I assume you are referring to "This might be as simple as including the crate hash of proc-macro crates" in Option (3), right? |
Upstream crates can affect things like the layout of used types. Layouts are not stored in the crate metadata and thus wouldn't change the crate hash unless dependencies are included in the crate hash. |
I was refering to option 2. Hashing just the crate metadata would not contain enough data to uniquely identify proc macros when their implementation changes, which may cause proc macros to not be re-expanded on implementation changes if macro expansion becomes incremental. |
|
I've been thinking a bit more about this. IMO, the only reason why hashing the crate metadata as a flat byte stream is not trivially correct is that the CrateLoader does some additional processing (like computing the cnum_map) which might introduce data dependencies on things that come from outside the byte stream. The way things are set up right now, it is very hard to tell what data goes into computing this additional data structures. If things were set up in a way that there was an eval_always query that only allowed access to the MetadataBlob then things would be simpler. I think what @cjgillot proposes in #94878 would be an improvement over what we have currently. I'd be very interested in seeing how it performs. I'm going to close this PR as it's clear that we won't merge it. |
Wouldn't you necessarily also introduce a dependency on the crate hash of the upstream crate in such cases? |
So this would be in the case where the upstream crate is a proc-macro? |
|
At the query graph level yes, all data loaded from dependencies results in a dependency on the crate_hash query. At the crate metadata level, the explicit list of the dependency in the crate metadata is the only thing ensuring that dependency versions aren't mismatched. This list needs to be part of the crate hash one way or another. Hashing just file sources would not be enough for this. Hashing full crate metadata would be enough. |
|
Right, proc-macro crates don't seem to encode their MIR at the moment -- so I can see how this is a problem. |
|
Hm, given that proc-macros are not required to be deterministic, they would need to be handled specially anyway, I guess. I.e. even a hash that completely captured the code of the proc macro would not be strictly correct since the proc-macro can still pull in data from arbitrary sources. So we would have to unconditionally assume that it has changed. |
|
For something like rust-analyzer unconditionally assuming that proc macro output has changed would result in 100% cpu usage if any proc macro is used. It should re-expand exactly when the proc macro needs to be/has been recompiled. In the future when rust-analyzer and rustc share more code it would be nice to be able to use the crate hash for that purpose. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This might be a nice simplification that make things more robust and would probably also simplify any work on end-to-end queries. Let's see what performance looks like.
r? @ghost