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

Remove TypeIdHasher and replace it with StableHasher in debuginfo and symbol hashing #50424

Closed
michaelwoerister opened this issue May 3, 2018 · 8 comments
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. I-compiletime Issue: Problems and improvements with respect to compile times. WG-compiler-performance Working group: Compiler Performance

Comments

@michaelwoerister
Copy link
Member

At the moment we have two implementations for hashing Ty values, the StableHasher (which is used for incr. comp. and TypeId) and the TypeIdHasher which was formerly used for TypeId and has some leftover uses in symbol hash generation and debuginfo.

We should be able to switch the two remaining uses to StableHasher quite easily. This might also be a small performance win, since StableHasher is more optimized.

cc @rust-lang/wg-compiler-performance

@michaelwoerister michaelwoerister added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. C-cleanup Category: PRs that clean code up or issues documenting cleanup. I-compiletime Issue: Problems and improvements with respect to compile times. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. WG-compiler-performance Working group: Compiler Performance labels May 3, 2018
@iancormac84
Copy link
Contributor

I'd like to work on this one, please.

@michaelwoerister
Copy link
Member Author

Sure! The first thing I would do is handling the debuginfo case linked above. Here is an example where you can see how to use the StableHasher to hash a type:

pub fn type_id_hash(self, ty: Ty<'tcx>) -> u64 {
let mut hasher = StableHasher::new();
let mut hcx = self.create_stable_hashing_context();
// We want the type_id be independent of the types free regions, so we
// erase them. The erase_regions() call will also anonymize bound
// regions, which is desirable too.
let ty = self.erase_regions(&ty);
hcx.while_hashing_spans(false, |hcx| {
hcx.with_node_id_hashing_mode(NodeIdHashingMode::HashDefPath, |hcx| {
ty.hash_stable(hcx, &mut hasher);
});
});
hasher.finish()
}

Let me know if you need further directions.

@iancormac84
Copy link
Contributor

@michaelwoerister how best to show you any progress I've made?

@iancormac84
Copy link
Contributor

I removed the TypeIdHasher instance in the debuginfo case and replaced it with this. I have some questions arising from reading the code you showed me, and code that TypeIdHasher uses:
Do I need to erase regions?
Is the code in TypeIdHasher's implementation of TypeVisitor, in particular visit_ty equivalent to what I have now?

@michaelwoerister
Copy link
Member Author

how best to show you any progress I've made?

You can just post here, as you already did. Alternatively, you can open a work-in-progress PR. Both are fine.

Do I need to erase regions?

Yes, we want to ignore differences in regions for debuginfo.

Is the code in TypeIdHasher's implementation of TypeVisitor, in particular visit_ty equivalent to what I have now?

Yes. The StableHasher is not based on TypeVisitor. Instead it recursively calls hash_stable() on components to do the visiting. StableHasher is also a bit more general at this point, since it has to handle more cases than TypeIdHasher.

@iancormac84
Copy link
Contributor

iancormac84 commented May 7, 2018

So does the code I wrote also cover the use of hash_discriminant_u8?

How do I write tests for the changes I've made?

Is it now fine to move on to the symbol hash generation code?

@michaelwoerister
Copy link
Member Author

So does the code I wrote also cover the use of hash_discriminant_u8?

Yes, the corresponding code is here:

mem::discriminant(self).hash_stable(hcx, hasher);

How do I write tests for the changes I've made?

The existing tests should cover it. Especially the src/test/debuginfo suite.

Is it now fine to move on to the symbol hash generation code?

Yes. Feel free to open a PR with the changes you did so far, as those should be independent of the symbol hash changes.

@iancormac84
Copy link
Contributor

iancormac84 commented May 7, 2018

Yes, the corresponding code is here:

Ah okay, seems as though I missed that implementation for TypeVariants during my searches. Thanks.

bors added a commit that referenced this issue May 9, 2018
[WIP] Cleanup uses of TypeIdHasher and replace them with StableHasher

Fixes #50424

r? @michaelwoerister
kennytm added a commit to kennytm/rust that referenced this issue May 19, 2018
…nup, r=michaelwoerister

Cleanup uses of TypeIdHasher and replace them with StableHasher

Fixes rust-lang#50424

r? @michaelwoerister
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-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. I-compiletime Issue: Problems and improvements with respect to compile times. WG-compiler-performance Working group: Compiler Performance
Projects
None yet
Development

No branches or pull requests

2 participants