Skip to content

Remove collect in doctest::run_test #92361

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
Merged
merged 1 commit into from
Mar 25, 2022
Merged

Remove collect in doctest::run_test #92361

merged 1 commit into from
Mar 25, 2022

Conversation

vacuus
Copy link
Contributor

@vacuus vacuus commented Dec 28, 2021

No description provided.

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Dec 28, 2021
@rust-highfive
Copy link
Contributor

r? @CraftSpider

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 28, 2021
@jyn514
Copy link
Member

jyn514 commented Dec 28, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 28, 2021
@bors
Copy link
Collaborator

bors commented Dec 28, 2021

⌛ Trying commit a851256442d55f9f83849a683b428d707be7d54d with merge 0748772981f166ff502b9d9b021d8c90a79df6e8...

@bors
Copy link
Collaborator

bors commented Dec 28, 2021

☀️ Try build successful - checks-actions
Build commit: 0748772981f166ff502b9d9b021d8c90a79df6e8 (0748772981f166ff502b9d9b021d8c90a79df6e8)

@rust-timer
Copy link
Collaborator

Queued 0748772981f166ff502b9d9b021d8c90a79df6e8 with parent 83b15bf, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0748772981f166ff502b9d9b021d8c90a79df6e8): 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. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 28, 2021
@camelid
Copy link
Member

camelid commented Jan 5, 2022

This actually causes slight instructions count regressions. It may improve max-rss, but the reported changes are not very significant.

@camelid
Copy link
Member

camelid commented Jan 5, 2022

As such, since the new code is slightly more complex IMO, I'm going to close this PR.

@camelid camelid closed this Jan 5, 2022
@camelid
Copy link
Member

camelid commented Jan 5, 2022

And now I'm having second thoughts... the new code might actually be better. I'll re-open this and let someone else decide. :P

@camelid camelid reopened this Jan 5, 2022
@CraftSpider
Copy link
Contributor

Personally I think this makes the code look a bit worse, what I'd really like to see is the currently unstable intersperse used here, which would allow a single collect.

@camelid
Copy link
Member

camelid commented Jan 6, 2022

We can use intersperse (rustdoc already has the feature gate enabled), but I'm not certain it'd work in this particular case since we need a trailing newline.

@CraftSpider
Copy link
Contributor

I like the way this looks now, going to try a perf run to see how this way of doing it fairs

@CraftSpider
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 12, 2022
@bors
Copy link
Collaborator

bors commented Jan 12, 2022

⌛ Trying commit 48438ce with merge 2eff7829258896a9ee425872e8cb5ba0eae56e9f...

@bors
Copy link
Collaborator

bors commented Jan 12, 2022

☀️ Try build successful - checks-actions
Build commit: 2eff7829258896a9ee425872e8cb5ba0eae56e9f (2eff7829258896a9ee425872e8cb5ba0eae56e9f)

@rust-timer
Copy link
Collaborator

Queued 2eff7829258896a9ee425872e8cb5ba0eae56e9f with parent 1bd4fdc, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (2eff7829258896a9ee425872e8cb5ba0eae56e9f): 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. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 13, 2022
@camelid
Copy link
Member

camelid commented Jan 13, 2022 •
edited
Loading

Hmm, I don't think the perf tool runs doctests, so that perf run might not even be affected by this change. It does have slight regressions across many doc benchmarks though...

@CraftSpider
Copy link
Contributor

I'll take a look, but if they don't have doctests, then I imagine any difference is more likely noise than real affect of this PR. I'm leaning towards merge if that's the case.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 13, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 20, 2022
@Dylan-DPC
Copy link
Member

@CraftSpider any updates on this?

@CraftSpider
Copy link
Contributor

After another look, no noticeable perf impact, but it does look nice and I believe slightly reduces allocations

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 24, 2022

📌 Commit 48438ce has been approved by CraftSpider

@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 24, 2022
@bors
Copy link
Collaborator

bors commented Mar 25, 2022

⌛ Testing commit 48438ce with merge 4ce257f...

@bors
Copy link
Collaborator

bors commented Mar 25, 2022

☀️ Test successful - checks-actions
Approved by: CraftSpider
Pushing 4ce257f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 25, 2022
@bors bors merged commit 4ce257f into rust-lang:master Mar 25, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 25, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4ce257f): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@vacuus vacuus deleted the doctest-run-test-out-lines branch April 8, 2022 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 approved these changes

Assignees

@CraftSpider CraftSpider

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
Milestone
1.61.0
Development

Successfully merging this pull request may close these issues.
10 participants
@vacuus @rust-highfive @jyn514 @rust-timer @bors @camelid @CraftSpider @Dylan-DPC @JohnCSimon @rustbot