Make incremental compilation thread-safe #49732
Conversation
rust-highfive
added
the
S-waiting-on-review
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☔ The latest upstream changes (presumably #49390) made this pull request unmergeable. Please resolve the merge conflicts. |
Zoxc
force-pushed
the
sync-dep-graph
branch
2 times, most recently
from
86d1309 to
5bf7762
Compare
Zoxc
changed the title
| if did_allocation { | ||
| // Only the thread which did the allocation emits the error messages | ||
|
|
||
| // FIXME: Ensure that these are printed before returning for all threads. |
There was a problem hiding this comment.
I left this FIXME as future work and added it to #48685
|
I've cleaned this up a bit now. |
|
@bors try |
Make incremental compilation thread-safe r? @michaelwoerister
|
☀️ Test successful - status-travis |
|
@Mark-Simulacrum Can I get a perf run? |
|
Started. |
|
Ping from triage @Mark-Simulacrum! What's the status of the perf run? |
|
We are a little behind on collecting master commits due to an error on the collector, but should get to this soon-ish. Please ping me if I haven't said anything in a day or two... |
|
@Mark-Simulacrum well, ping :) |
|
Hm, looks like we're still backlogged. I've made the commit we need to compare against have a higher priority. |
|
The results have some strange numbers for nll check: http://perf.rust-lang.org/compare.html?start=252a459d373f40512ed9137d59e4b6bea5d6aaee&end=23547a1db3bacc9e92f777b72c9cb3e34cf6a8db&stat=instructions%3Au @Mark-Simulacrum Do you know what that is about? |
|
ignore nll-check for now, I think it was semi-broken because we removed the |
| bug!("forbidden edge {:?} -> {:?} created", | ||
| source, | ||
| target) | ||
| ty::tls::with_context_opt(|icx| { |
There was a problem hiding this comment.
In theory all reads should always come from the same thread. I wonder if we could get rid of this lock.
There was a problem hiding this comment.
That's not true. Queries may use multiple threads internally and use queries there.
There was a problem hiding this comment.
A particular query invocation should only ever write to one vec and sub-invocations must get their own vec, so the same vec should never be shared among two invocations. Otherwise we would have a logical bug.
There was a problem hiding this comment.
A particular query invocation can write to that one vec using multiple threads.
There was a problem hiding this comment.
Oh, you mean when the query would use parallelism internally?
There was a problem hiding this comment.
As a minor optimization, the Lock could be moved into OpenTask maybe. Then there'd at least be no locking for Ignore and EvalAlways.
|
|
||
| /// When we load, there may be `.o` files, cached mir, or other such | ||
| /// things available to us. If we find that they are not dirty, we | ||
| /// load the path to the file storing those work-products here into | ||
| /// this map. We can later look for and extract that data. | ||
| previous_work_products: RefCell<FxHashMap<WorkProductId, WorkProduct>>, | ||
| previous_work_products: RwLock<FxHashMap<WorkProductId, WorkProduct>>, |
There was a problem hiding this comment.
We should refactor this to not be mutable at all.
|
|
||
| /// Work-products that we generate in this run. | ||
| work_products: RefCell<FxHashMap<WorkProductId, WorkProduct>>, | ||
| work_products: RwLock<FxHashMap<WorkProductId, WorkProduct>>, |
There was a problem hiding this comment.
This could be moved out of the dep-graph completely.
There was a problem hiding this comment.
If I didn't overlook something then this list could be threaded through as a return value: save_trans_partition() -> copy_module_artifacts_into_incr_comp_cache() -> OngoingCrateTranslation::join().
Doesn't need to happen in this PR though.
src/librustc/dep_graph/graph.rs
Outdated
| arg: A, | ||
| no_tcx: bool, | ||
| task: fn(C, A) -> R, | ||
| get_task: fn(DepNode) -> OpenTask, |
There was a problem hiding this comment.
Could you rename this to create_task?
src/librustc/dep_graph/graph.rs
Outdated
| no_tcx: bool, | ||
| task: fn(C, A) -> R, | ||
| get_task: fn(DepNode) -> OpenTask, | ||
| get_index: fn(&Lock<CurrentDepGraph>, |
There was a problem hiding this comment.
And this to finish_task_and_alloc_depnode? (I take full responsibility for this name :))
There was a problem hiding this comment.
Or finish_task_and_alloc_depnode, which would be more consistent with the other method names in this module.
src/librustc/dep_graph/graph.rs
Outdated
| pub(super) fn pop_task(&mut self, key: DepNode) -> DepNodeIndex { | ||
| let popped_node = self.task_stack.pop().unwrap(); | ||
|
|
||
| pub(super) fn complete_task(&mut self, key: DepNode, task: OpenTask) -> DepNodeIndex { |
There was a problem hiding this comment.
Now that raii.rs is gone this can probably be private.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I added refactoring |
👍 |
|
OK, I think this looks great! @Zoxc, I'll let you decide if you want to move the |
|
@bors r=michaelwoerister |
|
📌 Commit 3f802ee has been approved by |
bors
added
S-waiting-on-bors
Make incremental compilation thread-safe r? @michaelwoerister
|
☀️ Test successful - status-appveyor, status-travis |
r? @michaelwoerister