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.

Sign up for GitHub

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

Jump to bottom

Convert SerializedDepGraph to be a struct-of-arrays #49069

Merged
merged 1 commit into from
Mar 22, 2018
Merged

Convert SerializedDepGraph to be a struct-of-arrays #49069

merged 1 commit into from
Mar 22, 2018

Conversation

wesleywiser
Copy link
Member

Fixes #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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 16, 2018
michaelwoerister
michaelwoerister reviewed Mar 16, 2018
View reviewed changes
Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @wesleywiser! I think this is nicer now. The two tuple components were never used together.

Maybe we can make serialize() take the DepGraph by value at some point, then just moving the arrays would certainly be safe.

src/librustc/dep_graph/graph.rs Outdated
(dep_node, fingerprints[idx])
}).collect();
let fingerprints: IndexVec<_, _> = fingerprints.iter().cloned().collect();
let nodes : IndexVec<_, _> = current_dep_graph.nodes.iter().cloned().collect();
Copy link
Member

@michaelwoerister michaelwoerister Mar 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just use .clone() on the IndexVecs directory directly. That will make it more likely that this is a simple memcpy.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! I was so busy looking for impl Clone for IndexVec I totally missed the derive(Clone) line. 😆

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄

@michaelwoerister
Copy link
Member

Ping @wesleywiser, did you maybe forget to push your changes?

@wesleywiser
Copy link
Member Author

Unfortunately, I don't quite have them finished yet. I'm running into #48910 on my Mac which is hindering my progress. I do have a Linux box available which isn't running into the same issue but it's not as fast.

Since we actually need to change the index type, .clone() isn't sufficient (we need to go from IndexVec<DepNodeIndex, _> to IndexVec<SerializedDepNodeIndex, _>). I have some changes locally which use mem::transmute() to do that but I still need to run the test suite to make sure that isn't going to break anything. I also tried making serialize() take the DepGraph by value but I ran into some issues and decided to revert that.

@michaelwoerister
Copy link
Member

You can use ./x.py build --stage 1 src/libtest and ./x.py test --stage 1 src/test/incremental, with which you should not run into the bug. But the bug is problem. I'm running into it too. I'm looking into it at the moment, actually.

Good point regarding the type of the index. mem::transmute() sounds a bit unsafe though. Could you add a convert_index_type() method to IndexVec? Here's what I mean:
https://play.rust-lang.org/?gist=fdaab1c658e04d903dc401e006d705df&version=stable

@wesleywiser
Copy link
Member Author

You can use ./x.py build --stage 1 src/libtest and ./x.py test --stage 1 src/test/incremental, with which you should not run into the bug.

That's very helpful. Thanks!!

I'm looking into it at the moment, actually.

👍 👍 👍 👍

Could you add a convert_index_type() method to IndexVec?

Yes, that's a great idea! I wasn't liking the mem::transmute() approach very much anyway.

@wesleywiser
Copy link
Member Author

@michaelwoerister Changes pushed

@michaelwoerister
Copy link
Member

Looking good now! Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 20, 2018

📌 Commit c43b1a0 has been approved by michaelwoerister

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 20, 2018
@kennytm kennytm mentioned this pull request Mar 21, 2018
Closed
kennytm added a commit to kennytm/rust that referenced this pull request Mar 21, 2018
@kennytm
Rollup merge of rust-lang#49069 - wesleywiser:incr_soa, r=michaelwoer…
5589dc3 
…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 kennytm mentioned this pull request Mar 22, 2018
Merged
kennytm added a commit to kennytm/rust that referenced this pull request Mar 22, 2018
@kennytm
Rollup merge of rust-lang#49069 - wesleywiser:incr_soa, r=michaelwoer…
50850c6 
…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
bors added a commit that referenced this pull request Mar 22, 2018
@bors
Auto merge of #49264 - kennytm:rollup, r=kennytm
5092c6b 
Rollup of 23 pull requests

- Successful merges: #48374, #48596, #48759, #48939, #49029, #49069, #49093, #49109, #49117, #49140, #49158, #49188, #49189, #49209, #49211, #49216, #49225, #49231, #49234, #49242, #49244, #49105, #49038
- Failed merges:
@alexcrichton alexcrichton merged commit c43b1a0 into rust-lang:master Mar 22, 2018
@michaelwoerister michaelwoerister mentioned this pull request Dec 22, 2018
Merged
@Zoxc Zoxc mentioned this pull request Dec 23, 2018
Closed
bors added a commit that referenced this pull request Dec 23, 2018
@bors
Auto merge of #57070 - Zoxc:ser-graph, r=<try>
d453555 
[do not merge] Revert "Convert SerializedDepGraph to be a struct-of-arrays"

Let's check if #49069 was actually helpful.

r? @michaelwoerister
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelwoerister michaelwoerister michaelwoerister left review comments

Assignees

@michaelwoerister michaelwoerister

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@wesleywiser @michaelwoerister @bors @alexcrichton @rust-highfive