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

small improvements to interning #5153

Closed
wants to merge 2 commits into from

Conversation

Eh2406
Copy link
Contributor

@Eh2406 Eh2406 commented Mar 8, 2018

In #5121 Eh2406@411355a I tried to use the InternedString In more places, but test failed so that commit was backed out. This PR is an attempt to redo that in tiny steps, so as to track down exactly what caused the error.

The first commit hear, is just some housekeeping. This should still be green.

@rust-highfive
Copy link

r? @alexcrichton

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

@Eh2406
Copy link
Contributor Author

Eh2406 commented Mar 8, 2018

This second commit is the smallest change I have found that reproduces the test fails. It took several trys to find something this small.

I still don't see why it is problematic. @alexcrichton any thoughts on how this is wrong?

@Eh2406
Copy link
Contributor Author

Eh2406 commented Mar 8, 2018

I don't know what is going on with nightly and beta, that is not the error I see locally.

@alexcrichton
Copy link
Member

Aha! I believe the problem is that the hash value for an InternedString is making its way into the fingerprint for a crate which means that if it changes the crate is recompiled. That I think means that this would always recompile all the crates!

Perhaps the Hash implementation should look at the contents rather than just the raw pointer?

@alexcrichton
Copy link
Member

Ah and for nightly/beta feel free to just remove the #![deny(unused)]

@Eh2406
Copy link
Contributor Author

Eh2406 commented Mar 8, 2018

Thanks! That seems to have fixed it. It was nice to have constant time hashing, but I think the inconsistent hash is too much of a foot gun to leave in.

@Eh2406 Eh2406 closed this Mar 8, 2018
@Eh2406 Eh2406 deleted the more_interning_find_the_bug branch March 8, 2018 22:54
@alexcrichton
Copy link
Member

Closed?

@Eh2406
Copy link
Contributor Author

Eh2406 commented Mar 9, 2018

Ya you identified the bug, which fixed the more ambitious versions. When I have had a chance to polish one of them, it will come in as a new PR.

@alexcrichton
Copy link
Member

Aha I see!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants