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

Intern semver::Version/semver::VersionReq #6468

Closed
wants to merge 7 commits into from

Conversation

dwijnand
Copy link
Member

Resolves #6207

@rust-highfive
Copy link

r? @alexcrichton

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

@ehuss
Copy link
Contributor

ehuss commented Dec 20, 2018

Just curious, would it be possible to implement Intern<T> to avoid all the duplication? Or maybe use one of the many crates like internment?

@dwijnand
Copy link
Member Author

That's the next challenge: "intern" this cache infrastructure 😉

@bors
Copy link
Contributor

bors commented Dec 20, 2018

☔ The latest upstream changes (presumably #6469) made this pull request unmergeable. Please resolve the merge conflicts.

@dwijnand dwijnand force-pushed the intern-semver-Version/Req branch from a0d257e to f6124e5 Compare December 20, 2018 09:26
@dwijnand dwijnand force-pushed the intern-semver-Version/Req branch from f6124e5 to 0fd2ca2 Compare December 20, 2018 10:27
@dwijnand dwijnand force-pushed the intern-semver-Version/Req branch from 0fd2ca2 to 99192bf Compare December 20, 2018 10:54
@alexcrichton
Copy link
Member

Out of curiosity, do we know a location where we clone these a lot or create a lot of duplicate ones? It's not as clear to me with source/package ids that Cargo would benefit from this

@dwijnand
Copy link
Member Author

Paging @Eh2406 😄

@Eh2406
Copy link
Contributor

Eh2406 commented Dec 20, 2018

@dwijnand good question why did I say that...

looking back at the original, it is not so much that we cloned it a lot. Rather they are larger structures then they seem, and we probably have the same values menny times. (How many crates have a version 0.1.0?) So it may save memory by deduping. Also we already are leaking some of them by PackageId being interned.

Now that I am saying it I am not entirely convinced. Maybe given that PackageId Is Interned it is redundant to inter this ass well? Or maybe on the flip side, If we intern this then PackageId is just 3 pointers, and maybe it should not be interned?

@ehuss this interning strategy was inspired by internment. Each time we have copied this code we have needed to override the impl of Hash, so I don't think we can use something off the shelf. But there is definitely room to remove some of the duplication.

@alexcrichton
Copy link
Member

@dwijnand would you be up for doing some measurements perhaps? Checking memory differences for resolving large crate graphs like rustc or servo?

@dwijnand
Copy link
Member Author

Sure! How?

@alexcrichton
Copy link
Member

The "easiest" is probably Valgrind with massif on Linux perhaps?

@dwijnand
Copy link
Member Author

I don't have the bandwidth. The PR is here and it's not going anywhere, so we can always reconsider this change down the line. I'll salvage some of the diffs in a new PR.

@dwijnand dwijnand closed this Dec 22, 2018
@dwijnand dwijnand deleted the intern-semver-Version/Req branch December 22, 2018 17:13
@dwijnand dwijnand mentioned this pull request Dec 22, 2018
3 tasks
@Eh2406 Eh2406 mentioned this pull request Dec 22, 2018
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.

6 participants