84304 - rustdoc: shrink Item::Attributes #84494

tdelabro · 2021-04-23T20:24:23Z

Helps with #84304

rust-highfive · 2021-04-23T20:24:25Z

Some changes occurred in intra-doc-links.

cc @jyn514

rust-highfive · 2021-04-23T20:24:27Z

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

tdelabro · 2021-04-23T20:25:02Z

r? @jyn514

jyn514

This is a great change :) but I think the title is not quite accurate? It should say something like "Shrink Item::Attributes", Attributes still calculates some info ahead of time.

To fix the CI error you can run x.py fmt.

src/librustdoc/clean/types.rs

jyn514 · 2021-04-23T21:30:20Z

src/librustdoc/clean/types.rs

+    fn other_attrs(&self) -> Vec<ast::Attribute> {
+        self.iter().filter_map(|attr| match attr.doc_str() {
+            Some(_) => None,
+            None => Some(attr.clone()),
+        }).collect()
+    }


Hmm, you added this function, but didn't remove the other_attrs field - was that intentional?

This implementation was required to remove cfg from Attributes but to be able to entirely remove the other_attrs field from Attributes we will have to move it in Item. It will be done in a future MR.

jyn514 · 2021-04-23T21:31:03Z

src/librustdoc/clean/types.rs

+    fn other_attrs(&self) -> Vec<ast::Attribute> {
+        self.iter().filter_map(|attr| match attr.doc_str() {
+            Some(_) => None,
+            None => Some(attr.clone()),
+        }).collect()


Suggested change

fn other_attrs(&self) -> Vec<ast::Attribute> {

self.iter().filter_map(|attr| match attr.doc_str() {

Some(_) => None,

None => Some(attr.clone()),

}).collect()

fn other_attrs(&self) -> impl Iterator<Item = &ast::Attribute> {

self.iter().filter(|attr| attr.doc_str().is_none())

Hmm, I realized while reviewing that attr.doc_str().is_some() and attr.is_doc_comment() behave differently :/ I'll look into fixing that.

I noticed you change this to use .filter() but didn't change the return type - was that intentional? Did you run into trouble?

The reason I suggested removing .collect() is it avoids an allocation for the common case where you just want to check each attribute one at a time.

Ho ok. That's why !

I didn't notice the change on the return type, so yes I got some trouble and had to clone and collect the attributes.
I will try your way instead !

I tried, didn't go well ;)

jyn514 · 2021-04-23T21:35:33Z

src/librustdoc/clean/types.rs

-            .iter()
-            .find(|a| a.doc_str().is_some())
-            .map_or(true, |a| a.style == AttrStyle::Inner);
-
        Attributes {
            doc_strings,


Would it possible to also calculate this on demand? I think extract_include reads from the filesystem, so you probably want to add a cache - I'm fine with putting that off for a later PR.

It had been moved as an AttributeExt method

src/librustdoc/clean/types.rs

jyn514 · 2021-04-23T21:40:08Z

src/librustdoc/html/render/cache.rs

-    e.attrs
+    ast_attrs


It looks like this was the only place the attributes for ExternalCrate was used, so you could turn it into a parameter instead - that's a great change, but it makes it a little harder to see what changes are related to removing files from Attributes itself. Do you mind splitting it into a separate commit?

Should I revert the changes on ExternalCrate and put them in a separate MR ?

Also I didn't really get the "turn it into a parameter instead" part. Can you give me a little bit more explanation ?

Should I revert the changes on ExternalCrate and put them in a separate MR ?

Nah, don't worry about it, this is fine :) It's a good change.

Also I didn't really get the "turn it into a parameter instead" part. Can you give me a little bit more explanation ?

This is something you're already doing: instead of storing the attributes on ExternalCrate, you pass them in to extern_location, which is the only place that uses them.

src/librustdoc/passes/collect_intra_doc_links.rs

jyn514 · 2021-04-24T21:38:11Z

@tdelabro do you mind using rebasing instead of merging? There are instructions in https://rustc-dev-guide.rust-lang.org/git.html#conflicts.

tdelabro · 2021-04-25T16:54:54Z

@tdelabro do you mind using rebasing instead of merging? There are instructions in https://rustc-dev-guide.rust-lang.org/git.html#conflicts.

Sorry about this. I fixed it with a push force.

tdelabro · 2021-04-25T17:00:35Z

At this point Attributes is only two fields large:

#[derive(Clone, Debug, Default)]
crate struct Attributes {
    crate doc_strings: Vec<DocFragment>,
    crate other_attrs: Vec<ast::Attribute>,
}

Next I will try to store other_attrs in Item so they will be available at any moment to build doc_strings, which is based on the combination of the Item attributes (that we can query at any time using txc.get_attrs(def_id)) and these other_attrs. Once this will be done, I think we will be able to remove Attributes form the codebase.

However, I don't know what we should do with all the functions defined in impl Attributes (eg: extract_include or has_doc_flags). Probably move them to impl Item.
What do you think ?

jyn514

Next I will try to store other_attrs in Item so they will be available at any moment to build doc_strings, which is based on the combination of the Item attributes and these other_attrs. Once this will be done, I think we will be able to remove Attributes form the codebase.

I think we should save this for a follow-up PR, it will be easier to catch regressions.

However, I don't know what we should do with all the functions defined in impl Attributes (eg: extract_include or has_doc_flags). Probably move them to impl Item.

I would move them to AttributesExt where possible.

src/librustdoc/clean/types.rs

jyn514 · 2021-04-25T17:00:13Z

src/librustdoc/formats/cache.rs

+            self.extern_locations.insert(
+                n,
+                (name, src_root, extern_location(e, extern_url, tcx.get_attrs(did), &dst, tcx)),
+            );


Doesn't have to be here, but it would be nice to store the CrateNum here instead of the name and attributes. Both of those are available from the tcx.

Actually all of this looks like it could be calculated on-demand with only the CrateNum, tcx, and extern_html_root_urls.

I'm not sure I understood correctly:
Yo would like to replace crate external_paths: FxHashMap<DefId, (Vec<String>, ItemType)> by crate extern_html_urls: FxHashMap<DefId, &BTreeMap<String, String>>, get name and attributes from tcx, combine them with cache.extern_html_root_url[crate_num] and then compute extern_locations with all this ?

I would like to get rid of extern_locations altogether, and calculate name, src_root, and external_location on demand. external_paths can stay the same.

I'll open an issue for it, it doesn't belong here.

Opened #84588.

tdelabro · 2021-04-26T13:01:15Z

@rustbot modify labels to -S-waiting-on-author and +S-waiting-on-review

bors · 2021-04-26T14:52:16Z

☀️ Try build successful - checks-actions
Build commit: 762707b31816d73aa7eb2aadf5961e9bc3d8e434 (762707b31816d73aa7eb2aadf5961e9bc3d8e434)

rust-timer · 2021-04-26T14:52:18Z

Queued 762707b31816d73aa7eb2aadf5961e9bc3d8e434 with parent 8212de8, future comparison URL.

rust-timer · 2021-04-26T17:30:54Z

Finished benchmarking try commit (762707b31816d73aa7eb2aadf5961e9bc3d8e434): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

GuillaumeGomez · 2021-04-26T17:34:28Z

Nice improvement on both the RSS usage and on the number of instructions (max -1.0% in both cases).

jyn514

r=me with the nit addressed :)

jyn514 · 2021-04-26T20:33:47Z

src/librustdoc/clean/inline.rs

+            if let Some(new_id) = parent_module {
+                Attributes::from_ast(old_attrs, Some((inner, new_id)))
+            } else {
+                both.clean(cx)
+            },


It was like this before, but I think it would be more clear to use from_ast consistently:

Suggested change

if let Some(new_id) = parent_module {

Attributes::from_ast(old_attrs, Some((inner, new_id)))

} else {

both.clean(cx)

},

if let Some(new_id) = parent_module {

Attributes::from_ast(old_attrs, Some((inner, new_id)))

} else {

Attributes::from_ast(both, None)

},

Indeed it's better this way

jyn514 · 2021-04-26T20:37:49Z

Oh, and if you could squash the commits a little that would be nice too, but I won't block on it.

…t of the fields in Attributes, as functions in AttributesExt. refacto use from_def_id_and_attrs_and_parts instead of an old trick most of josha suggestions + check if def_id is not fake before using it in a query Removed usage of Attributes in FnDecl and ExternalCrate. Relocate part of the Attributes fields as functions in AttributesExt.

tdelabro · 2021-04-27T08:21:32Z

Oh, and if you could squash the commits a little that would be nice too, but I won't block on it.

Here we are. I just kept two of them

check item.is_fake() instead of self_id.is_some() Remove empty branching in Attributes::from_ast diverse small refacto after Josha review cfg computation moved in merge_attrs refacto use from_ast twice for coherence take cfg out of Attributes and move it to Item

jyn514 · 2021-04-27T17:25:27Z

@bors r+

bors · 2021-04-27T17:25:28Z

📌 Commit 727f904 has been approved by jyn514

bors · 2021-04-27T20:20:01Z

⌛ Testing commit 727f904 with merge 727d101...

bors · 2021-04-27T22:46:08Z

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing 727d101 to master...

rust-highfive assigned GuillaumeGomez Apr 23, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 23, 2021

rust-highfive assigned jyn514 and unassigned GuillaumeGomez Apr 23, 2021

tdelabro force-pushed the 84304-bis branch from 14d5b42 to f45717f Compare April 23, 2021 21:19

jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Apr 23, 2021

This comment has been minimized.

Sign in to view

jyn514 reviewed Apr 23, 2021

View reviewed changes

tdelabro changed the title ~~84304 - rustdoc: Get rid of Item::Attributes~~ WIP 84304 - rustdoc: shrink Item::Attributes Apr 24, 2021

tdelabro marked this pull request as draft April 24, 2021 18:58

This comment has been minimized.

Sign in to view

jyn514 reviewed Apr 24, 2021

View reviewed changes

src/librustdoc/passes/collect_intra_doc_links.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

tdelabro force-pushed the 84304-bis branch from 0e52d86 to 1094ea8 Compare April 25, 2021 16:18

jyn514 reviewed Apr 25, 2021

View reviewed changes

tdelabro changed the title ~~WIP 84304 - rustdoc: shrink Item::Attributes~~ 84304 - rustdoc: shrink Item::Attributes Apr 25, 2021

tdelabro marked this pull request as ready for review April 25, 2021 19:58

tdelabro marked this pull request as draft April 25, 2021 19:59

tdelabro marked this pull request as ready for review April 25, 2021 20:54

jyn514 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 26, 2021

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 26, 2021

jyn514 removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 26, 2021

jyn514 reviewed Apr 26, 2021

View reviewed changes

tdelabro force-pushed the 84304-bis branch from 4e8d01a to 7566c96 Compare April 27, 2021 08:19

This comment has been minimized.

Sign in to view

tdelabro force-pushed the 84304-bis branch from 7566c96 to 727f904 Compare April 27, 2021 08:30

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 Apr 27, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 27, 2021

bors merged commit 727d101 into rust-lang:master Apr 27, 2021

rustbot added this to the 1.53.0 milestone Apr 27, 2021

tdelabro deleted the 84304-bis branch April 28, 2021 09:12

84304 - rustdoc: shrink Item::Attributes #84494

84304 - rustdoc: shrink Item::Attributes #84494

Conversation

tdelabro commented Apr 23, 2021 • edited by jyn514 Loading

rust-highfive commented Apr 23, 2021

rust-highfive commented Apr 23, 2021

tdelabro commented Apr 23, 2021

This comment has been minimized.

jyn514 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdelabro Apr 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

jyn514 commented Apr 24, 2021

This comment has been minimized.

tdelabro commented Apr 25, 2021

tdelabro commented Apr 25, 2021 • edited Loading

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdelabro Apr 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 Apr 26, 2021 • edited Loading

Choose a reason for hiding this comment

tdelabro commented Apr 26, 2021

bors commented Apr 26, 2021

rust-timer commented Apr 26, 2021

rust-timer commented Apr 26, 2021

GuillaumeGomez commented Apr 26, 2021

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 commented Apr 26, 2021 • edited Loading

tdelabro commented Apr 27, 2021

This comment has been minimized.

jyn514 commented Apr 27, 2021

bors commented Apr 27, 2021

bors commented Apr 27, 2021

bors commented Apr 27, 2021

tdelabro commented Apr 23, 2021 •

edited by jyn514

Loading

jyn514 left a comment •

edited

Loading

tdelabro Apr 25, 2021 •

edited

Loading

tdelabro commented Apr 25, 2021 •

edited

Loading

tdelabro Apr 25, 2021 •

edited

Loading

jyn514 Apr 26, 2021 •

edited

Loading

jyn514 commented Apr 26, 2021 •

edited

Loading