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

Make re-export collection deterministic #65043

Merged
merged 2 commits into from Oct 6, 2019

Conversation

@Aaron1011
Copy link
Contributor

commented Oct 3, 2019

Fixes #65036

Previously, we were using an FxHashMap to collect module re-exports.
However, re-exports end up getting serialized into crate metadata, which
means that metadata generation was non-deterministic. This resulted in
spurious error messages changes (e.g. PR #64906) due to pretty-printing
implicitly depending on the order of re-exports when computing the
proper path to show to the user.

See #65042 for a long-term strategy to detect this kind of issue

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Oct 3, 2019

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2019

@rust-highfive rust-highfive assigned Centril and unassigned petrochenkov Oct 3, 2019
@Centril

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

Looks good but r? @petrochenkov should really review this since resolve is their wheelhouse. :)

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

I think we usually deal with this via collection into a Vec and sorting just before metadata serialization; that might be a better approach here?

@jonas-schievink

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

Fixes #65043

Did you mean "Fixes #65036"?

@Centril

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

I personally feel that IndexMap is a more reliable by-construction approach that takes care of every iteration site so you don't have to care about all of them. Maybe when we get the wrapper for FxHashMap we should use that instead.

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2019

@jonas-schievink: Thanks, fixed

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

We do have a StableMap in the data structures crate as of a recent PR, so maybe using that here would be better. It does not expose iteration order at all (lacking those methods entirely).

But again, I'm fine landing this in the mean time. An indexmap seems like a reasonable answer too.

@matthewjasper

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

Stable map looks unsuitable since Idents don't implement Ord.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

I think we usually deal with this via collection into a Vec and sorting just before metadata serialization

That was my first reaction too.
Changing Resolutions, which are used for a thousand other things beside serializing reexports, is such an unproportionally heavy hammer for fixing this.

We actually have a method for visiting resolutions in a stable fashion (fn for_each_child_stable) which is already used in diagnostic reporting with the same purpose.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

@bors try @rust-timer queue

@bors

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

⌛️ Trying commit 2c9bf5f with merge 9f13bf7...

bors added a commit that referenced this pull request Oct 3, 2019
Make re-export collection deterministic

Fixes #65036

Previously, we were using an `FxHashMap` to collect module re-exports.
However, re-exports end up getting serialized into crate metadata, which
means that metadata generation was non-deterministic. This resulted in
spurious error messages changes (e.g. PR #64906) due to pretty-printing
implicitly depending on the order of re-exports when computing the
proper path to show to the user.

See #65042 for a long-term strategy to detect this kind of issue
@Centril

This comment was marked as resolved.

Copy link
Member

commented Oct 3, 2019

(enqueue -> queue)

...or apparently it understands both, cool.

@rust-timer

This comment has been minimized.

Copy link

commented Oct 3, 2019

Awaiting bors try build completion

@bors

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

⌛️ Trying commit 2c9bf5f with merge 0e298d5...

bors added a commit that referenced this pull request Oct 3, 2019
Make re-export collection deterministic

Fixes #65036

Previously, we were using an `FxHashMap` to collect module re-exports.
However, re-exports end up getting serialized into crate metadata, which
means that metadata generation was non-deterministic. This resulted in
spurious error messages changes (e.g. PR #64906) due to pretty-printing
implicitly depending on the order of re-exports when computing the
proper path to show to the user.

See #65042 for a long-term strategy to detect this kind of issue
@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

I personally feel that IndexMap is a more reliable by-construction approach

Actually, I'm not sure that the insertion order itself is stable for the resolutions, if it's not, then IndexMap maintaining it won't matter much.

@Centril

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

Interesting. I'm pretty happy I reassigned this PR. :)

@bors

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

☀️ Try build successful - checks-azure
Build commit: 0e298d5 (0e298d5daf4f422edf242c37161329ee79515942)

@rust-timer

This comment has been minimized.

Copy link

commented Oct 3, 2019

Queued 0e298d5 with parent 032a53a, future comparison URL.

@rust-timer

This comment was marked as outdated.

Copy link

commented Oct 4, 2019

Finished benchmarking try commit 0e298d5, comparison URL.

2 similar comments
@rust-timer

This comment was marked as off-topic.

Copy link

commented Oct 4, 2019

Finished benchmarking try commit 0e298d5, comparison URL.

@rust-timer

This comment was marked as resolved.

Copy link

commented Oct 4, 2019

Finished benchmarking try commit 0e298d5, comparison URL.

@bjorn3

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

I personally feel that IndexMap is a more reliable by-construction approach

Actually, I'm not sure that the insertion order itself is stable for the resolutions, if it's not, then IndexMap maintaining it won't matter much.

We have StableMap since #64131. It works the same as FxHashMap, but you can't iterate it, you have to use .into_sorted_vec() instead.

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2019

@petrochenkov: This looks like a wash performance wise - a few minor speedups and a few minor slowdowns. I suspect that much of it is just noise.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

Yeah, I just wanted to check what performance impact IndexMap can have, sorry for all this noise.
I still think using for_each_child_stable in finalize_resolutions_in is a preferable solution.

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2019

@petrochenkov: All other things being equal, I think it's better to reduce the amount of non-determinism in the compiler. This issue was a pain to track down, and I'm worried that it might crop up again if we're relying on every caller using for_each_child_stable whenever they need to iterate over to it.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

Let's check whether IndexMap provides what we need or not - #65117.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2019

Looks like resolutions are added in stable order, so we can use an IndexMap.

@Aaron1011
Could you remove for_each_child_stable in this PR then, and replace its uses with for_each_child.
(Replacing other iterations through resolutions would also be nice, the use of for_each_child looks a bit cleaner in all cases.)

Previously, we were using an `FxHashMap` to collect module re-exports.
However, re-exports end up getting serialized into crate metadata, which
means that metadata generation was non-deterministic. This resulted in
spurious error messages changes (e.g. PR #64906) due to pretty-printing
implicitly depending on the order of re-exports when computing the
proper path to show to the user.

See #65042 for a long-term strategy to detect this kind of issue
@Aaron1011 Aaron1011 force-pushed the Aaron1011:fix/reexport-determinism branch from 2c9bf5f to 33178a9 Oct 5, 2019
@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2019

@petrochenkov Updated

Now that `Resolutions` has a deterministic iteration order, it's no
longer necessary to sort its entries before iterating over them
@Aaron1011 Aaron1011 force-pushed the Aaron1011:fix/reexport-determinism branch from 33178a9 to add0a42 Oct 5, 2019
@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2019

Thanks!
@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2019

📌 Commit add0a42 has been approved by petrochenkov

@bors

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2019

⌛️ Testing commit add0a42 with merge 0358617...

bors added a commit that referenced this pull request Oct 6, 2019
…nkov

Make re-export collection deterministic

Fixes #65036

Previously, we were using an `FxHashMap` to collect module re-exports.
However, re-exports end up getting serialized into crate metadata, which
means that metadata generation was non-deterministic. This resulted in
spurious error messages changes (e.g. PR #64906) due to pretty-printing
implicitly depending on the order of re-exports when computing the
proper path to show to the user.

See #65042 for a long-term strategy to detect this kind of issue
@bors

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2019

☀️ Test successful - checks-azure
Approved by: petrochenkov
Pushing 0358617 to master...

@bors bors added the merged-by-bors label Oct 6, 2019
@bors bors merged commit add0a42 into rust-lang:master Oct 6, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20191006.2 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
@Aaron1011 Aaron1011 deleted the Aaron1011:fix/reexport-determinism branch Oct 6, 2019
@RalfJung

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

Previously, we were using an FxHashMap to collect module re-exports.
However, re-exports end up getting serialized into crate metadata, which
means that metadata generation was non-deterministic.

Clarification to avoid spreading this myth further: FxHashMap is deterministic. However, inserting a new element can surprisingly affect the entire iteration order. Also see #63713 (comment).

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2019

@RalfJung: I meant 'non-deterministic with respect to platforms' - that is, the crate metadata order wouldn't look the same across different platforms. Sorry for the confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.