Don't sort a Vec before computing its DepTrackingHash
#85702
Conversation
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors rollup=never - could be perf sensitive, in theory, though somewhat unlikely |
This comment has been minimized.
This comment has been minimized.
Previously, we sorted the vec prior to hashing, making the hash independent of the original (command-line argument) order. However, the original vec was still always kept in the original order, so we were relying on the rest of the compiler always working with it in an 'order-independent' way. This assumption was not being upheld by the `native_libraries` query - the order of the entires in its result depends on the order of entries in `Options.libs`. This lead to an 'unstable fingerprint' ICE when the `-l` arguments were re-ordered. This PR removes the sorting logic entirely. Re-ordering command-line arguments (without adding/removing/changing any arguments) seems like a really niche use case, and correctly optimizing for it would require additional work. By always hashing arguments in their original order, we can entirely avoid a cause of 'unstable fingerprint' errors.
|
cc #84234 |
|
Thanks for the PR, @Aaron1011! This looks good to me -- but I don't quite understand how the regression test works? Can you add a comment explaining it? Otherwise feel free to r=me. |
|
@michaelwoerister the test gets run twice with different flag order. On the second execution, whichever it might be, it would ICE+panic and the test would fail. |
|
Right, but the first invocation of the test has to pass. Otherwise we'll start with an empty cache for the second run and won't be able to observe the crash. |
|
@michaelwoerister I don't think link errors affect the cache, but I could be wrong. |
|
That's possible. I'm not sure. I would not want to rely on it in a regression test though. |
|
The cache does get populated when there's a link error - I reproduced the ICE with this test on |
|
I'm a bit worried that this might change at some point in the future and then the test would silently become ineffective. Is there a way to make the test fail if the second run starts with an empty cache? Like adding one of the existing incr. comp. test attributes? Not really because we expect the cache to be cleared already during dep-graph loading, right? Maybe that would be worth adding another test attribute or -Z option though. I'll leave it up to you, @Aaron1011! If you decide to merge the test as it is I want to at least open an issue. |
|
@michaelwoerister Note that in this case, we will end up with an empty cache, since the commandline arg hash will be changed by the re-ordering of the link arguments. However, we will still attempt to load the old incremental data, and discover the mismatch when we do. It would be good to have a flag to assert this - I'll open an issue. |
|
@michaelwoerister: I've opened #85864 to track adding a new debugging flag to assert the correct state of the incr cache. @bors r=michaelwoerister |
|
📌 Commit 605513a has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
👍 |
Previously, we sorted the vec prior to hashing, making the hash
independent of the original (command-line argument) order. However, the
original vec was still always kept in the original order, so we were
relying on the rest of the compiler always working with it in an
'order-independent' way.
This assumption was not being upheld by the
native_librariesquery -the order of the entires in its result depends on the order of entries
in
Options.libs. This lead to an 'unstable fingerprint' ICE when the-larguments were re-ordered.This PR removes the sorting logic entirely. Re-ordering command-line
arguments (without adding/removing/changing any arguments) seems like a
really niche use case, and correctly optimizing for it would require
additional work. By always hashing arguments in their original order, we
can entirely avoid a cause of 'unstable fingerprint' errors.