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

compiler code cleanup: replace metas: HashMap with IndexVec #46876

Closed
pnkfelix opened this issue Dec 20, 2017 · 9 comments · Fixed by #46913
Closed

compiler code cleanup: replace metas: HashMap with IndexVec #46876

pnkfelix opened this issue Dec 20, 2017 · 9 comments · Fixed by #46913
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.

Comments

@pnkfelix
Copy link
Member

While investigating the root cause(s) of #46112 and discussing (in the #rustc IRC chat room) the use of a hashmap mapping crate-numbers to crate-metadata, @eddyb at some point exclaimed "why is that not an IndexVec?"

One word: Legacy. Big time: the metas field has been a hashmap ever since metadata::cstore was introduced, 6.5 years ago in b23ecd4 (relevant line in diff)

It should be a useful (and hopefully simple?) exercise to replace the hashmap that is now there with an IndexVec.

@eddyb eddyb added C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Dec 20, 2017
@Eh2406
Copy link
Contributor

Eh2406 commented Dec 20, 2017

So is it just replacing the type with?

pub struct IndexVec<I: Idx, T> {

If sow what is the equivalent for insert?

self.metas.borrow_mut().insert(cnum, data);

@arielb1
Copy link
Contributor

arielb1 commented Dec 20, 2017

@Eh2406

Should be as simple as that

@Eh2406
Copy link
Contributor

Eh2406 commented Dec 21, 2017

The only way I am seeing to add to an IndexVec is to push to the end at index == len

pub fn push(&mut self, d: T) -> I {

But the hashmap uses the ability to insert at an arbitrary index:

self.metas.borrow_mut().insert(cnum, data);

So I don't quite see how to make the conversion.

@pnkfelix
Copy link
Member Author

@Eh2406 from my quick skimming, fn set_crate_data is only called from one place: CrateLoader::register_crate, here:

self.cstore.set_crate_data(cnum, cmeta.clone());

The cnum in that context is always the steadily incremented next_crate_num.

So I would suggest removing fn set_crate_data entirely, and replacing it with a some method that indeed pushes to the end at index == len, as you state, since it seems that will have the same end effect.

@pnkfelix
Copy link
Member Author

BTW if one really did need to support overwriting for arbitrary cnums, that is easy too, as long as they already exist in the IndexVec, since IndexVec supports the IndexMut method that underlying a[i] = b: https://doc.rust-lang.org/std/ops/trait.IndexMut.html ; but I don't think we will need that in this case.

@arielb1
Copy link
Contributor

arielb1 commented Dec 21, 2017

@Eh2406

Maybe you want an IndexVec<CrateNum, Option<Rc<CrateMetadata>>>?

@arielb1
Copy link
Contributor

arielb1 commented Dec 21, 2017

The cnum in that context is always the steadily incremented next_crate_num.

That's not 100% correct. Crate numbers are assigned in preorder - if a crate (say std) has a dependency (say core), then std will get the earlier crate number, then core will be loaded, and only then will the metadata for std be assigned.

But it's ok to use an IndexVec<CrateNum, Option<Rc<CrateMetadata>>> to handle this.

@pnkfelix
Copy link
Member Author

pnkfelix commented Dec 21, 2017

That's not 100% correct. Crate numbers are assigned in preorder ...

You are right. I overlooked the fact that we 1. first increment the crate num (reserving the number, like for std in your example), then 2. call resolve_crate_deps (which may recursively re-enter register_crate), before finally 3. calling set_crate_data at the end.

@Eh2406
Copy link
Contributor

Eh2406 commented Dec 21, 2017

Ok, what is the best incantation to test this locally?

In the meantime I will open a pr.

bors added a commit that referenced this issue Jan 3, 2018
CStore switch FxHashMap to IndexVec

This is a first attempt to fix #46876.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants