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

CStore switch FxHashMap to IndexVec #46913

Merged
merged 1 commit into from Jan 3, 2018

Conversation

Projects
None yet
7 participants
@Eh2406
Contributor

Eh2406 commented Dec 21, 2017

This is a first attempt to fix #46876.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Dec 21, 2017

Collaborator

r? @eddyb

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

Collaborator

rust-highfive commented Dec 21, 2017

r? @eddyb

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

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Dec 21, 2017

Contributor

Your code does not compile:

[00:15:32] error[E0507]: cannot move out of borrowed content
[00:15:32]    --> /checkout/src/librustc_metadata/cstore.rs:114:9
[00:15:32]     |
[00:15:32] 114 |         self.metas.borrow().get(cnum).unwrap().unwrap().clone()
[00:15:32]     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot move out of borrowed content
[00:15:32] 
[00:15:32] error[E0596]: cannot borrow immutable local variable `met` as mutable
[00:15:32]    --> /checkout/src/librustc_metadata/cstore.rs:121:13
[00:15:32]     |
[00:15:32] 119 |         let met = self.metas.borrow_mut();
[00:15:32]     |             --- consider changing this to `mut met`
[00:15:32] 120 |         while met.len() <= cnum.index() {
[00:15:32] 121 |             met.push(None);
[00:15:32]     |             ^^^ cannot borrow mutably
[00:15:32] 
[00:15:32] error[E0596]: cannot borrow immutable local variable `met` as mutable
[00:15:32]    --> /checkout/src/librustc_metadata/cstore.rs:123:9
[00:15:32]     |
[00:15:32] 119 |         let met = self.metas.borrow_mut();
[00:15:32]     |             --- consider changing this to `mut met`
[00:15:32] ...
[00:15:32] 123 |         met[cnum] = Some(data);
[00:15:32]     |         ^^^ cannot borrow mutably
[00:15:32] 
Contributor

arielb1 commented Dec 21, 2017

Your code does not compile:

[00:15:32] error[E0507]: cannot move out of borrowed content
[00:15:32]    --> /checkout/src/librustc_metadata/cstore.rs:114:9
[00:15:32]     |
[00:15:32] 114 |         self.metas.borrow().get(cnum).unwrap().unwrap().clone()
[00:15:32]     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot move out of borrowed content
[00:15:32] 
[00:15:32] error[E0596]: cannot borrow immutable local variable `met` as mutable
[00:15:32]    --> /checkout/src/librustc_metadata/cstore.rs:121:13
[00:15:32]     |
[00:15:32] 119 |         let met = self.metas.borrow_mut();
[00:15:32]     |             --- consider changing this to `mut met`
[00:15:32] 120 |         while met.len() <= cnum.index() {
[00:15:32] 121 |             met.push(None);
[00:15:32]     |             ^^^ cannot borrow mutably
[00:15:32] 
[00:15:32] error[E0596]: cannot borrow immutable local variable `met` as mutable
[00:15:32]    --> /checkout/src/librustc_metadata/cstore.rs:123:9
[00:15:32]     |
[00:15:32] 119 |         let met = self.metas.borrow_mut();
[00:15:32]     |             --- consider changing this to `mut met`
[00:15:32] ...
[00:15:32] 123 |         met[cnum] = Some(data);
[00:15:32]     |         ^^^ cannot borrow mutably
[00:15:32] 
@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Dec 21, 2017

Contributor

Thanks for the heads up. I got distracted trying to test locally.

Contributor

Eh2406 commented Dec 21, 2017

Thanks for the heads up. I got distracted trying to test locally.

@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Dec 21, 2017

Contributor

Ok, I am confused, what does that test fail have to do with my changes?

Contributor

Eh2406 commented Dec 21, 2017

Ok, I am confused, what does that test fail have to do with my changes?

@pnkfelix pnkfelix added the T-compiler label Dec 21, 2017

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Dec 21, 2017

Member

@Eh2406 Assuming that you are referring to the change to the output for ui/print_type_sizes/niche-filling.rs ... here is my current understanding...

I think we are seeing some (currently undiagnosed) non-deterministic treatment of std::foo::bar vs core::foo::bar , and your change is causing a ripple effect there.

You can see some recent notes from me on this at #46905, in particular the commit message for 5f85764

Note in particular that that tests originally said core::cmp::Ordering; it was only changed to std::cmp::Ordering very recently, in #46708 (which I am becoming increasingly concerned about and am considering reverting in favor of #46838 ...).

  • Of course the ideal thing would be for someone to explain why it switched from core to std in the first place as part of #46708 ; I had thought it was obviously better at the time when I wrote #46708, but I have since come to understand the point of the comment noted in the log message for 5f85764 :
    Entry::Occupied(mut entry) => {
        // If `child` is defined in crate `cnum`, ensure
        // that it is mapped to a parent in `cnum`.
        if child.krate == cnum && entry.get().krate != cnum {
            entry.insert(parent);
        }
    }
  • namely, I think the idea of that code is that its better to prefer the path that corresponds to an items definition, if that path is public, rather than a path of a mere re-export of the item.

In the meantime, I personally would be fine with you just updating the print-type-sizes.stdout to match the new output. (Which after all is the same as the old output...)

Member

pnkfelix commented Dec 21, 2017

@Eh2406 Assuming that you are referring to the change to the output for ui/print_type_sizes/niche-filling.rs ... here is my current understanding...

I think we are seeing some (currently undiagnosed) non-deterministic treatment of std::foo::bar vs core::foo::bar , and your change is causing a ripple effect there.

You can see some recent notes from me on this at #46905, in particular the commit message for 5f85764

Note in particular that that tests originally said core::cmp::Ordering; it was only changed to std::cmp::Ordering very recently, in #46708 (which I am becoming increasingly concerned about and am considering reverting in favor of #46838 ...).

  • Of course the ideal thing would be for someone to explain why it switched from core to std in the first place as part of #46708 ; I had thought it was obviously better at the time when I wrote #46708, but I have since come to understand the point of the comment noted in the log message for 5f85764 :
    Entry::Occupied(mut entry) => {
        // If `child` is defined in crate `cnum`, ensure
        // that it is mapped to a parent in `cnum`.
        if child.krate == cnum && entry.get().krate != cnum {
            entry.insert(parent);
        }
    }
  • namely, I think the idea of that code is that its better to prefer the path that corresponds to an items definition, if that path is public, rather than a path of a mere re-export of the item.

In the meantime, I personally would be fine with you just updating the print-type-sizes.stdout to match the new output. (Which after all is the same as the old output...)

@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Dec 22, 2017

Contributor

Ok so I switched from core::cmp::Ordering to std::cmp::Ordering in the test, we will see if that fixes things.

Contributor

Eh2406 commented Dec 22, 2017

Ok so I switched from core::cmp::Ordering to std::cmp::Ordering in the test, we will see if that fixes things.

@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Dec 22, 2017

Contributor

Any ideas what is up with these errors? Why would this change lead to differences in the error messages?

Contributor

Eh2406 commented Dec 22, 2017

Any ideas what is up with these errors? Why would this change lead to differences in the error messages?

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb
Member

eddyb commented Dec 22, 2017

@rust-highfive rust-highfive assigned pnkfelix and unassigned eddyb Dec 22, 2017

@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Dec 22, 2017

Contributor

Is there any Ideas on how to proceed? Should I try a rebase?

Contributor

Eh2406 commented Dec 22, 2017

Is there any Ideas on how to proceed? Should I try a rebase?

@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Dec 27, 2017

Contributor

All the PRs referenced by @pnkfelix have been merged, so I tried a rebase (and I squashed while I was doing). We will see if it works.

Contributor

Eh2406 commented Dec 27, 2017

All the PRs referenced by @pnkfelix have been merged, so I tried a rebase (and I squashed while I was doing). We will see if it works.

@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Jan 2, 2018

Contributor

Ping. The test got fixed by other prs. So this is now back to being a small simple change, with green CI.

Contributor

Eh2406 commented Jan 2, 2018

Ping. The test got fixed by other prs. So this is now back to being a small simple change, with green CI.

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Jan 2, 2018

Member

@bors r+

Member

eddyb commented Jan 2, 2018

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 2, 2018

Contributor

📌 Commit 2c69473 has been approved by eddyb

Contributor

bors commented Jan 2, 2018

📌 Commit 2c69473 has been approved by eddyb

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 3, 2018

Contributor

⌛️ Testing commit 2c69473 with merge 10f980c...

Contributor

bors commented Jan 3, 2018

⌛️ Testing commit 2c69473 with merge 10f980c...

bors added a commit that referenced this pull request Jan 3, 2018

Auto merge of #46913 - Eh2406:master, r=eddyb
CStore switch FxHashMap to IndexVec

This is a first attempt to fix #46876.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 3, 2018

Contributor

💔 Test failed - status-appveyor

Contributor

bors commented Jan 3, 2018

💔 Test failed - status-appveyor

@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Jan 3, 2018

Contributor

Time out on appveyor. So did the last few commits. So we are on hold for that to get straightened out, yes?

Contributor

Eh2406 commented Jan 3, 2018

Time out on appveyor. So did the last few commits. So we are on hold for that to get straightened out, yes?

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Jan 3, 2018

Member

@bors retry #46903 (3 hour timeout)

Member

kennytm commented Jan 3, 2018

@bors retry #46903 (3 hour timeout)

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 3, 2018

Contributor

⌛️ Testing commit 2c69473 with merge 8d1a302...

Contributor

bors commented Jan 3, 2018

⌛️ Testing commit 2c69473 with merge 8d1a302...

bors added a commit that referenced this pull request Jan 3, 2018

Auto merge of #46913 - Eh2406:master, r=eddyb
CStore switch FxHashMap to IndexVec

This is a first attempt to fix #46876.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 3, 2018

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 8d1a302 to master...

Contributor

bors commented Jan 3, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 8d1a302 to master...

@bors bors merged commit 2c69473 into rust-lang:master Jan 3, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment