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

incr.comp.: Use a struct-of-arrays instead of an array-of-structs for SerializedDepGraph #47326

Closed
michaelwoerister opened this issue Jan 10, 2018 · 3 comments
Labels
A-incr-comp Area: Incremental compilation C-enhancement Category: An issue proposing an enhancement or a PR with one. I-compiletime Issue: Problems and improvements with respect to compile times.

Comments

@michaelwoerister
Copy link
Member

michaelwoerister commented Jan 10, 2018

This should improve memory consumption and cache locality a bit because we are wasting space on padding bytes at the moment. It should also enable us to bulk-persist parts or all of the graph to disk at once instead of dealing with each value separately.

@michaelwoerister michaelwoerister added C-enhancement Category: An issue proposing an enhancement or a PR with one. I-compiletime Issue: Problems and improvements with respect to compile times. A-incr-comp Area: Incremental compilation labels Jan 10, 2018
@michaelwoerister michaelwoerister changed the title incr.comp.: Use a struct-of-arrays instead of arrays-of-structs for SerializedDepGraph incr.comp.: Use a struct-of-arrays instead of an array-of-structs for SerializedDepGraph Jan 10, 2018
@michaelwoerister
Copy link
Member Author

Some more information: The thing that could be refactored here is the nodes field in SerializedDepGraph:

pub nodes: IndexVec<SerializedDepNodeIndex, (DepNode, Fingerprint)>,

At the moment it is an array of (DepNode, Fingerprint) but it could be turned into two arrays, one for DepNode, one for Fingerprint.

Then the following step in DepGraph::serialize() would not be necessary:

let nodes: IndexVec<_, (DepNode, Fingerprint)> =
current_dep_graph.nodes.iter_enumerated().map(|(idx, &dep_node)| {
(dep_node, fingerprints[idx])
}).collect();

One could even try to not copy the arrays at all in DepGraph::serialize(). Instead one could use std::mem::swap() to exchange the data in DepGraph for empty arrays and then just move it into the returned SerializedDepGraph. This would leave the dep-graph in an invalid state though. Not sure if we access it again after serialization (some code might still read from it).

@wesleywiser
Copy link
Member

wesleywiser commented Mar 14, 2018

I can take this. Thanks for writing up that additional detail @michaelwoerister!

@michaelwoerister
Copy link
Member Author

Cool! Thanks, @wesleywiser!

wesleywiser added a commit to wesleywiser/rust that referenced this issue Mar 19, 2018
kennytm added a commit to kennytm/rust that referenced this issue Mar 21, 2018
…ister

Convert SerializedDepGraph to be a struct-of-arrays

Fixes rust-lang#47326

I did not try the "`mem::swap()` to avoid copying the arrays" idea because that would leave the DepGraph in an incorrect state and that doesn't seem like a good idea for me.

r? @michaelwoerister
kennytm added a commit to kennytm/rust that referenced this issue Mar 22, 2018
…ister

Convert SerializedDepGraph to be a struct-of-arrays

Fixes rust-lang#47326

I did not try the "`mem::swap()` to avoid copying the arrays" idea because that would leave the DepGraph in an incorrect state and that doesn't seem like a good idea for me.

r? @michaelwoerister
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-incr-comp Area: Incremental compilation C-enhancement Category: An issue proposing an enhancement or a PR with one. I-compiletime Issue: Problems and improvements with respect to compile times.
Projects
None yet
Development

No branches or pull requests

2 participants