Skip to content

Optimize indentation in the pretty printer. #59507

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
Apr 1, 2019
Merged

Optimize indentation in the pretty printer. #59507

merged 1 commit into from
Apr 1, 2019

Conversation

nnethercote
Copy link
Contributor

Currently the pretty-printer calls write! for every space of
indentation. On some workloads the indentation level can exceed 100, and
a faster implementation reduces instruction counts by up to 7% on a few
workloads.

@rust-highfive
Copy link
Contributor

r? @petrochenkov

(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 Mar 28, 2019
@nnethercote
Copy link
Contributor Author

Some local measurements. webrender_api is not part of rustc-perf, but I added it locally because of rust-lang/rustc-perf#312.

crates.io-check
        avg: -4.1%      min: -7.4%      max: -1.8%
webrender_api-check
        avg: -4.2%      min: -6.8%      max: -3.1%
cargo-check
        avg: -1.5%      min: -2.6%      max: -0.6%

The bigger improvements are for incremental compilation.

@nnethercote
Copy link
Contributor Author

@bors try

@bors
Copy link
Collaborator

bors commented Mar 28, 2019

⌛ Trying commit bca0279 with merge cf0a738...

bors added a commit that referenced this pull request Mar 28, 2019
@bors
Auto merge of #59507 - nnethercote:indent-with-SPACES, r=<try>
cf0a738 
Optimize indentation in the pretty printer.

Currently the pretty-printer calls `write!` for every space of
indentation. On some workloads the indentation level can exceed 100, and
a faster implementation reduces instruction counts by up to 7% on a few
workloads.
@nnethercote nnethercote mentioned this pull request Mar 28, 2019
Closed
@bors
Copy link
Collaborator

bors commented Mar 29, 2019

☀️ Try build successful - checks-travis
Build commit: cf0a738

@nnethercote
Copy link
Contributor Author

@rust-timer build cf0a738

@rust-timer
Copy link
Collaborator

Success: Queued cf0a738 with parent 237bf32, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit cf0a738

petrochenkov
petrochenkov reviewed Mar 29, 2019
View reviewed changes
petrochenkov
petrochenkov reviewed Mar 29, 2019
View reviewed changes
@petrochenkov
Copy link
Contributor

r=me with nits addressed

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 29, 2019
eddyb
eddyb reviewed Mar 29, 2019
View reviewed changes
@nnethercote
Copy link
Contributor Author

I addressed the nits. Measurements show that avoiding write! made a tiny improvement, saving another 0.1% on the best cases.

@bors r=petrochenkov

@bors
Copy link
Collaborator

bors commented Mar 29, 2019

📌 Commit a7292a7216dae4d40856c62bdfac7fd2432bc351 has been approved by petrochenkov

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 29, 2019
ollie27
ollie27 suggested changes Mar 29, 2019
View reviewed changes
Copy link
Contributor

@ollie27 ollie27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

write isn't guaranteed to write all of the bytes.

@bors r-

@Centril
Copy link
Contributor

Centril commented Mar 29, 2019

^--- @bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 29, 2019
@nnethercote
Optimize indentation in the pretty printer.
606f315 
Currently the pretty-printer calls `write!` for every space of
indentation. On some workloads the indentation level can exceed 100, and
a faster implementation reduces instruction counts by up to 7% on a few
workloads.
@nnethercote
Copy link
Contributor Author

I changed the write calls to write_all.

@bors r=petrochenkov

@bors
Copy link
Collaborator

bors commented Mar 31, 2019

📌 Commit 606f315 has been approved by petrochenkov

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 31, 2019
@bors
Copy link
Collaborator

bors commented Mar 31, 2019

⌛ Testing commit 606f315 with merge eab3eb3...

bors added a commit that referenced this pull request Mar 31, 2019
@bors
Auto merge of #59507 - nnethercote:indent-with-SPACES, r=petrochenkov
eab3eb3 
Optimize indentation in the pretty printer.

Currently the pretty-printer calls `write!` for every space of
indentation. On some workloads the indentation level can exceed 100, and
a faster implementation reduces instruction counts by up to 7% on a few
workloads.
@bors
Copy link
Collaborator

bors commented Apr 1, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: petrochenkov
Pushing eab3eb3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 1, 2019
@bors bors merged commit 606f315 into rust-lang:master Apr 1, 2019
@nnethercote nnethercote deleted the indent-with-SPACES branch April 2, 2019 06:06
@nnethercote nnethercote mentioned this pull request Apr 2, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

+2 more reviewers

@eddyb eddyb eddyb left review comments

@ollie27 ollie27 ollie27 requested changes

Reviewers whose approvals may not affect merge requirements
Assignees

@petrochenkov petrochenkov

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@nnethercote @rust-highfive @bors @rust-timer @petrochenkov @Centril @eddyb @ollie27