Skip to content

Conversation

camelid
Copy link
Member

@camelid camelid commented Oct 29, 2021

The rustc fork of rayon integrates with Cargo's jobserver to limit the
amount of parallelism. However, rustdoc's use case is concurrent I/O,
which is not CPU-heavy, so it should be able to use mainline rayon.

See this discussion for more details.

Note: I chose rayon 1.3.1 so that the rayon version used elsewhere in
the workspace does not change.

r? @Mark-Simulacrum
cc @jyn514

The rustc fork of rayon integrates with Cargo's jobserver to limit the
amount of parallelism. However, rustdoc's use case is concurrent I/O,
which is not CPU-heavy, so it should be able to use mainline rayon.

See this discussion [1] for more details.

[1]: rust-lang#90227 (comment)

Note: I chose rayon 1.3.1 so that the rayon version used elsewhere in
the workspace does not change.
@camelid camelid added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Oct 29, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 29, 2021
@camelid
Copy link
Member Author

camelid commented Oct 29, 2021

The code using rustc-rayon was originally added in #60971. @rbtcollins was there a particular reason you used rustc-rayon instead of mainline rayon, or should it be fine to change? (Also cc @GuillaumeGomez, who worked on that change as well.)

@camelid
Copy link
Member Author

camelid commented Oct 29, 2021

Also, it looks like rustc-rayon and rayon 1.3.1 are fairly similar, but I'm not certain.

@camelid
Copy link
Member Author

camelid commented Oct 29, 2021

I think a perf run probably won't measure anything because rustc-perf uses Linux, right?

@camelid
Copy link
Member Author

camelid commented Oct 29, 2021

Just in case (and so perf could be measured independently from rustc-perf if people so choose): @bors rollup=never

@jyn514
Copy link
Member

jyn514 commented Oct 29, 2021

I think a perf run probably won't measure anything because rustc-perf uses Linux, right?

Correct, I ran into the same trouble in #88219.

I think the new code is simpler and easier to understand.
@GuillaumeGomez
Copy link
Member

I don't think it'll impact performance in any case so should be fine.

@rbtcollins
Copy link
Contributor

rbtcollins commented Oct 29, 2021 via email

@jyn514
Copy link
Member

jyn514 commented Oct 29, 2021

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 29, 2021

📌 Commit 581dc75 has been approved by jyn514

@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 Oct 29, 2021
@bors
Copy link
Collaborator

bors commented Oct 29, 2021

⌛ Testing commit 581dc75 with merge deb4572...

@bors
Copy link
Collaborator

bors commented Oct 29, 2021

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing deb4572 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 29, 2021
@bors bors merged commit deb4572 into rust-lang:master Oct 29, 2021
@rustbot rustbot added this to the 1.58.0 milestone Oct 29, 2021
@camelid camelid deleted the rustdoc-rayon branch October 29, 2021 18:49
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (deb4572): 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants