Make incremental compilation thread-safe#49732
Conversation
|
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. |
86d1309 to
5bf7762
Compare
| 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 |
src/librustc/dep_graph/graph.rs
Outdated
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.
src/librustc/dep_graph/graph.rs
Outdated
There was a problem hiding this comment.
We should refactor this to not be mutable at all.
src/librustc/dep_graph/graph.rs
Outdated
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
There was a problem hiding this comment.
Could you rename this to create_task?
src/librustc/dep_graph/graph.rs
Outdated
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
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 |
Make incremental compilation thread-safe r? @michaelwoerister
|
☀️ Test successful - status-appveyor, status-travis |
r? @michaelwoerister