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

Rustdoc: reduce size of Clean types #92514

Closed
wants to merge 3 commits into from

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Jan 3, 2022

This is an experiment to see how much we could reduce the size of various clean types and if it's even worth it. Most of the clean types that contain a Vec do not actually need any Vec functionality, since the Vec is essentially read only. It could thus be replaced with a boxed slice to reduce its size.

The PR is split into three commits:

  1. The first simply replaces most Vec<T>s with Box<[T]>. This reduces the size from 24 to 16 bytes. Conversion from Box<[T]> into a Vec is mostly free, however in the other direction it can be a bit more complex (see next commit).

  2. When converting from a vec to a boxed slice, the into_boxed_slice method first shrinks the Vec to reduce any excess capacity. I hoped that this shrinking would not happen in rustdoc code and that the capacity would equal the length in most cases, since the vecs are usually built out of a vec! macro or a collect call. Sadly, this was not the case, and the reallocations from the shrinking outweighed any RSS improvements.

    To alleviate this, I introduced the into_boxed_slice2 method, which avoids this shrinking. It is of course a horrible hack, but I didn't know how to do this from outside the vec module, so this was done just to make it work. This would need to be done in a different way of course if we decide to use this approach.

  3. The third commit uses the thin-slice crate that offers a thin boxed slice, which further reduces the size from 16 to 8 bytes. It can hold slices up to length 65535 without an additional indirection.

I'm not sure if the RSS improvements will be worth it, but I suppose that it's worth a perf. run.
I suspect that there are many other places in the compiler when a Vec could be replaced by a (thin) boxed slice to reduce its size.

@rust-highfive
Copy link
Collaborator

Some changes occurred in clean/types.rs.

cc @camelid

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 3, 2022
@rust-highfive
Copy link
Collaborator

r? @ollie27

(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 Jan 3, 2022
@Kobzol
Copy link
Contributor Author

Kobzol commented Jan 3, 2022

r? rust-lang/rustdoc

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.debug-assertions := True
configure: rust.overflow-checks := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
* highest error code: E0786
Found 502 error codes
Found 0 error codes with no tests
Done!
tidy error: invalid license `MPL-2.0` in `thin-slice 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)`
tidy error: Dependencies not explicitly permitted:
* thin-slice 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)
some tidy checks failed



command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/rust-tidy" "/checkout" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "/checkout/obj/build" "16"


Build completed unsuccessfully in 0:00:12

@Kobzol
Copy link
Contributor Author

Kobzol commented Jan 3, 2022

Hmm, it seems that thin-slice's license is not allowed. Unfortunate. Can this be circumvented at least for a perf. run?

@bors
Copy link
Contributor

bors commented Jan 9, 2022

☔ The latest upstream changes (presumably #92690) made this pull request unmergeable. Please resolve the merge conflicts.

@Kobzol
Copy link
Contributor Author

Kobzol commented Jan 24, 2022

r? @camelid

@rust-highfive rust-highfive assigned camelid and unassigned ollie27 Jan 24, 2022
@camelid
Copy link
Member

camelid commented Jan 25, 2022

I'm not sure if it's worth micro-optimizing clean since the ultimate goal is to get rid of it altogether.

In terms of the license issue, is it worth doing the perf run given that we won't be able to land this code anyway? If you want, you can also do a perf run locally: https://github.com/rust-lang/rustc-perf/.

@camelid camelid 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 Jan 25, 2022
@Kobzol
Copy link
Contributor Author

Kobzol commented Jan 25, 2022

Yeah I did multiple perf. runs locally, I just wanted to check it with PGO, LTO and all the "bells and whistles" on CI :) But anyway, the results weren't stellar, and if the ultimate goal is to remove the clean types, then it's not worth it to complicate it like this. Closing.

@Kobzol Kobzol closed this Jan 25, 2022
@camelid
Copy link
Member

camelid commented Jan 26, 2022

Thanks for trying anyway! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. 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.

7 participants