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: Get rid of Item::Attributes #84304

Open
jyn514 opened this issue Apr 18, 2021 · 5 comments
Open

rustdoc: Get rid of Item::Attributes #84304

jyn514 opened this issue Apr 18, 2021 · 5 comments
Assignees
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@jyn514
Copy link
Member

jyn514 commented Apr 18, 2021

This is a special case of #76382. Attributes is 72 bytes large and used in quite a few places:

This has two downsides:

  1. Attributes takes up a lot of space in memory, so this bloats memory usage
  2. It means attributes have to calculated ahead of time, instead of on-demand. In particular, this requires adding a new attrs field whenever render needs to look at the attrs of a specific clean type (Add stability tags to ImportItem. #83900 (comment)).

It would be better to calculate this on-demand, so it uses less memory and doesn't have to be calculated for each and every item.

@jyn514 jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. labels Apr 18, 2021
@jyn514
Copy link
Member Author

jyn514 commented Apr 18, 2021

Steps to implement this:

  • Change all the fields on Attributes to functions on AttributesExt.
    • These are calculated in from_ast, so you'll have to pass in some context. I wonder if reporting a diagnostic there is actually necessary - I'd expect rustc to verify that doc(cfg) has valid syntax, if it doesn't already, we could move it into check_doc_attrs:
      fn check_doc_attrs(&self, attr: &Attribute, hir_id: HirId, target: Target) -> bool {
    • The hard thing will be getting rid of additional_attrs - this is used for inlining:
      Attributes::from_ast(diag, old_attrs, Some((inner, new_id)))
      . I wonder if it makes sense to have an inlined_attrs field on Cache - that would let render calculate all this on-demand without having to come up with some crazy scheme to update the attrs in the main TyCtxt. Hopefully a simple HashMap<DefId, (Attributes, /*parent_module: */ DefId)> mapping will be enough; I don't know if we ever need to deal with attributes for something without a DefId.
  • Change all functions on Attributes to functions on AttributesExt; this is the easy part.

@tdelabro
Copy link
Contributor

So the new Attributes fields should be diagnostic, attrs and additional_attrs, aka the arguements required by a call to from_ast ?

@jyn514
Copy link
Member Author

jyn514 commented Apr 19, 2021

There should be no diagnostic at all - I have a suspicion it's not doing anything currently, but in case it is, those checks should be moved out of rustdoc and into rustc.

Hmm, keeping Attributes but only storing attrs and additional_attrs is an interesting idea. I would be ok with that too - my original plan was to get rid of Attributes altogether, but that would be harder than holding on to additional_attrs.

@jyn514 jyn514 added E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Apr 19, 2021
@tdelabro
Copy link
Contributor

@rustbot claim

@tdelabro
Copy link
Contributor

I will give it a try. I'm not familiar with the codebase but keeping a lightened Attributes and compute its fields on the fly doesn't seem to hard of a refacto.

I will message you on Zulip if I have any questions.

bors added a commit to rust-lang-ci/rust that referenced this issue Apr 23, 2021
rustdoc: Remove most fields from ExternalCrate

Once rust-lang#84304 is fixed, I can get rid of ExternCrate altogether in favor of CrateNum, but in the meantime, this shrinks ExternalCrate quite a lot.

This might hurt compile-times; if it does, I can add `primitive` and `keyword` queries. I expect this to improve compilemem.

Helps with rust-lang#76382.

r? GuillaumeGomez
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 27, 2021
84304 - rustdoc: shrink Item::Attributes

Helps with rust-lang#84304
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants