Skip to content

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

Closed
dwijnand wants to merge 7 commits intorust-lang:masterfrom
dwijnand:intern-semver-Version/Req
Closed

Intern semver::Version/semver::VersionReq#6468
dwijnand wants to merge 7 commits intorust-lang:masterfrom
dwijnand:intern-semver-Version/Req

Conversation

@dwijnand
Copy link
Contributor

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
Contributor 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
Contributor 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
Contributor Author

Sure! How?

@alexcrichton
Copy link
Member

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

@dwijnand
Copy link
Contributor 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