Skip to content
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

Add a metric containing the size of generated documentation #1417

Merged
merged 1 commit into from
Aug 26, 2022
Merged

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Aug 19, 2022

This week's triage report said something like "we don't track the size of generated documentation in rustc-perf". Well, I figured, why don't we? It seems easy enough.

CC @GuillaumeGomez @jsha Is this metric interesting for rustdoc developers? Currently I store the size of the whole target/doc directory, should we store something else?

@Kobzol Kobzol requested a review from Mark-Simulacrum August 19, 2022 08:08
collector/src/execute.rs Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member

It sounds like a very good idea! I think storing the number of file and also the mean (total size divided by number of files) would help us have a good idea of how things are evolving.

@Kobzol
Copy link
Contributor Author

Kobzol commented Aug 19, 2022

I also thought about storing the number of files, that sounds useful.

But calculating some derived data (like the mean) is a bit out of scope I think. We already have ~20 metrics and I don't think that we should add derived calculations to them, it seems like we should add some other approach for that (maybe just calculating it in the dashboard on demand). What do you think @Mark-Simulacrum?

@GuillaumeGomez
Copy link
Member

If we store both the number of files and the total size, there is no need to store the mean, it's easy to compute on demand.

@Mark-Simulacrum
Copy link
Member

Definitely agree with not storing it - and not quite sure it makes sense for the dashboard to compute, but obviously something that users can pretty easily do themselves.

I think number of files is unlikely to be too interesting (our benchmarks are largely constant so I'm not sure how often we're going to see a shift there), but storing it seems fine.

@Kobzol
Copy link
Contributor Author

Kobzol commented Aug 19, 2022

@GuillaumeGomez Does rustdoc perform changes that vary the number of files (for a documented crate that stays the same) often enough that it's worth it to save the count?

@Kobzol
Copy link
Contributor Author

Kobzol commented Aug 19, 2022

I added also file count tracking, it's simple enough since we already gather the file size. I'm not sure if the size: prefix makes sense for the file count metric.

@GuillaumeGomez
Copy link
Member

@GuillaumeGomez Does rustdoc perform changes that vary the number of files (for a documented crate that stays the same) often enough that it's worth it to save the count?

No but new types gets added in the std/core libraries. Considering the total number of files, it shouldn't matter much but it's better to be able from one look to tell why there is a small change in the total size.

Copy link
Member

@rylev rylev left a comment

Choose a reason for hiding this comment

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

I have one small nit, but otherwise looks great!

site/src/comparison.rs Outdated Show resolved Hide resolved
@Kobzol
Copy link
Contributor Author

Kobzol commented Aug 24, 2022

@Mark-Simulacrum @GuillaumeGomez any further comments? :)

@GuillaumeGomez
Copy link
Member

Looks good to me!

@Kobzol Kobzol requested a review from rylev August 24, 2022 17:12
Copy link
Member

@rylev rylev 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!

collector/src/execute.rs Outdated Show resolved Hide resolved
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.

4 participants