Skip to content

Add rudimentary timing of parsing and formatting phases #2149

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.

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

Closed
wants to merge 2 commits into from
Closed

Add rudimentary timing of parsing and formatting phases #2149

wants to merge 2 commits into from

Conversation

marcusklaas
Copy link
Contributor

@marcusklaas marcusklaas commented Nov 14, 2017

I was interested to see how much time is spent parsing vs formatting. This adds a timer to Summary to measure these things.

When run on rustfmt itself, using $ cargo run --release --bin rustfmt -- --verbose src/lib.rs, I get the following results on my machine:

Spent PT0.109887983S in the parsing phase, and PT0.414788861S in the formatting phase

In this case, formatting takes about 4x as long as parsing. This suggests that it should be worthwhile to look into trimming the fat from rustfmt.

cc #338

Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! I'd prefer if we could use std rather than add a dep though.

Interesting stats too. I'd be interested to see if that varies with the crate or whether it is fairly consistent.

So, it looks like parsing time includes IO time reading files from disk, but the formatting time does not include writing to disk. That would make the ratio look even worse for rustfmt (I would naturally expect formatting to take longer than parsing, but not by that much). It would be really interesting to record IO vs computation time, but I guess that needs a profiler, rather than just timing code.

Cargo.toml Outdated
@@ -44,6 +44,7 @@ log = "0.3"
env_logger = "0.4"
getopts = "0.2"
derive-new = "0.5"
time = "0.1"
Copy link
Member

Choose a reason for hiding this comment

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

Could you use std::time instead of the time crate? I think it has everything you need without the extra dep.

@marcusklaas
Copy link
Contributor Author

Updated!

@nrc
Copy link
Member

nrc commented Nov 20, 2017

This still has the dependency info in the Cargo.lock, I think you need to build before committing in order to update the lock file.

@davidalber
Copy link
Contributor

I have a rebased (onto current master) version of @marcusklaas's branch here that also updates Cargo.lock, in case you want to move this PR forward. You are welcome to use that, just "manually" update the original branch, or I'd be happy to open a new PR with that branch as the source.

@nrc
Copy link
Member

nrc commented Dec 27, 2017

rebased and merged. Thanks!

@nrc nrc closed this Dec 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants