rustdoc: reduce number of copies when using parallel IO #88219
Conversation
jyn514
added
T-rustdoc
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit a8956557448a362f90e454eaaf915ecc8d2af9b9 with merge 853cac83440612cc4564f31c8b0ea39ef4389bf1... |
|
☀️ Try build successful - checks-actions |
|
Queued 853cac83440612cc4564f31c8b0ea39ef4389bf1 with parent 9faa714, future comparison URL. |
| // A possible future enhancement after more detailed profiling would | ||
| // be to create the file sync so errors are reported eagerly. | ||
| let path = path.as_ref().to_path_buf(); | ||
| let contents = contents.as_ref().to_vec(); | ||
| let sender = self.errors.clone().expect("can't write after closing"); | ||
| rayon::spawn(move || { |
There was a problem hiding this comment.
Rayon isn't designed for use with blocking IO (https://docs.rs/rayon/1.5.1/rayon/fn.join.html#warning-about-blocking-io). I think it would be a good idea to change this to use a threadpool.
There was a problem hiding this comment.
Ahhh, that explains why it was so much slower locally. Thanks!
There was a problem hiding this comment.
Wait, I'm confused - that warning is specifically on join, and spawn says it already uses a threadpool:
Fires off a task into the Rayon threadpool in the “static” or “global” scope.
|
Finished benchmarking try commit (853cac83440612cc4564f31c8b0ea39ef4389bf1): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. @bors rollup=never |
rustbot
removed
the
S-waiting-on-perf
There was a problem hiding this comment.
Looks like this was not a perf improvement, although I'm not sure why ... I'll change this to only land the first commit for now (Remove unnecessary copies when using parallel IO) and come back to this once it uses the updated version of DocFS rustup is using: #60971 (comment)
jyn514
force-pushed
the
parallel-io
branch
2 times, most recently
from
ff16450
to
77a5f5a
Compare
jyn514
changed the title
Previously, rustdoc was making lots of copies of temporary owned values. Now, it uses the owned value wherever possible.
jyn514
removed
the
S-experimental
JohnCSimon
added
S-waiting-on-review
|
ping @GuillaumeGomez - do you know when you'll get a chance to review this? |
|
I was waiting for a new perf run. Reviewing anyway. :) |
|
@GuillaumeGomez this change has no effect on Linux, so I can't test it using perf.rlo. @bors r=GuillaumeGomez |
|
📌 Commit 5651192 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (d1d8145): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
This is Windows-only for now; I was getting really bad slowdowns from this on linux for some reason.
Helps with #82741. Follow-up to #60971.