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

Cheaper doc comments #65750

Merged
merged 2 commits into from
Nov 7, 2019
Merged

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Oct 24, 2019

This PR implements the idea from #60935: represent doc comments more cheaply, rather than converting them into #[doc="..."] attribute form. Unlike #60936 (which is about coalescing doc comments to reduce their number), this approach does not have any backwards compatibility concerns, and it eliminates about 80-90% of the current cost of doc comments (as estimated using the numbers in #60930, which eliminated the cost of doc comments entirely by treating them as normal comments).

r? @petrochenkov

@nnethercote
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 24, 2019

⌛ Trying commit 0b907f7ed55cd4b79ccb380e68fcca0dacc0f0e2 with merge b773dddc5a6434753e502b8beb1868fd476103f8...

@nnethercote
Copy link
Contributor Author

@Manishearth, @nrc: this PR will affect Clippy and rustfmt.

@petrochenkov petrochenkov added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 24, 2019
@nnethercote
Copy link
Contributor Author

@rust-timer build b773dddc5a6434753e502b8beb1868fd476103f8

@rust-timer
Copy link
Collaborator

Queued b773dddc5a6434753e502b8beb1868fd476103f8 with parent 4a8c5b2, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit b773dddc5a6434753e502b8beb1868fd476103f8, comparison URL.

@nnethercote
Copy link
Contributor Author

Perf results are excellent, as expected.

@Centril
Copy link
Contributor

Centril commented Oct 25, 2019

r? @Centril @bors r+

@bors
Copy link
Contributor

bors commented Oct 25, 2019

📌 Commit 42bac153bb9e9baaf30f9aaa2830faf69042ac99 has been approved by Centril

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 25, 2019
@Centril
Copy link
Contributor

Centril commented Oct 25, 2019

@bors rollup=never (perf)

@petrochenkov
Copy link
Contributor

@bors r-
I really wanted to review this, can it wait for a day or so?

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 25, 2019
@petrochenkov petrochenkov self-assigned this Oct 25, 2019
@Centril
Copy link
Contributor

Centril commented Oct 25, 2019

Sure -- I don't mind.
(I did double check the diff and it all seems good.)

@nrc
Copy link
Member

nrc commented Oct 25, 2019

cc @topecongiro for possible rustfmt breakage

@bors
Copy link
Contributor

bors commented Oct 25, 2019

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

@nnethercote
Copy link
Contributor Author

I have rebased.

This kind of thing just makes the code harder to read.
`AttrKind` is a new type with two variants, `Normal` and `DocComment`. It's a
big performance win (over 10% in some cases) because `DocComment` lets doc
comments (which are common) be represented very cheaply.

`Attribute` gets some new helper methods to ease the transition:
- `has_name()`: check if the attribute name matches a single `Symbol`; for
  `DocComment` variants it succeeds if the symbol is `sym::doc`.
- `is_doc_comment()`: check if it has a `DocComment` kind.
- `{get,unwrap}_normal_item()`: extract the item from a `Normal` variant;
  panic otherwise.

Fixes rust-lang#60935.
@nnethercote
Copy link
Contributor Author

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Nov 6, 2019

📌 Commit eea6f23 has been approved by petrochenkov

@bors 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 Nov 6, 2019
@bors
Copy link
Contributor

bors commented Nov 7, 2019

⌛ Testing commit eea6f23 with merge caf0187...

bors added a commit that referenced this pull request Nov 7, 2019
Cheaper doc comments

This PR implements the idea from #60935: represent doc comments more cheaply, rather than converting them into `#[doc="..."]` attribute form. Unlike #60936 (which is about coalescing doc comments to reduce their number), this approach does not have any backwards compatibility concerns, and it eliminates about 80-90% of the current cost of doc comments (as estimated using the numbers in #60930, which eliminated the cost of doc comments entirely by treating them as normal comments).

r? @petrochenkov
@bors
Copy link
Contributor

bors commented Nov 7, 2019

☀️ Test successful - checks-azure
Approved by: petrochenkov
Pushing caf0187 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 7, 2019
@bors bors merged commit eea6f23 into rust-lang:master Nov 7, 2019
msizanoen1 added a commit to msizanoen1/rust-clippy that referenced this pull request Nov 7, 2019
@tesuji
Copy link
Contributor

tesuji commented Nov 12, 2019

The final result: https://perf.rust-lang.org/compare.html?start=38048763e885a3ee139abf39d59a530b16484150&end=caf018714189db0b15f9f803adfcb4572ab7a988&stat=instructions%3Au

@nnethercote nnethercote deleted the cheaper-doc-comments branch November 13, 2019 21:53
Centril added a commit to Centril/rust that referenced this pull request Dec 3, 2019
syntax: Unify macro and attribute arguments in AST

The unified form (`ast::MacArgs`) represents parsed arguments instead of an unstructured token stream that was previously used for attributes.
It also tracks some spans and delimiter kinds better for fn-like macros and macro definitions.

I've been talking about implementing this with @nnethercote in rust-lang#65750 (comment).
The parsed representation is closer to `MetaItem` and requires less token juggling during conversions, so it potentially may be faster.

r? @Centril
Centril added a commit to Centril/rust that referenced this pull request Dec 3, 2019
syntax: Unify macro and attribute arguments in AST

The unified form (`ast::MacArgs`) represents parsed arguments instead of an unstructured token stream that was previously used for attributes.
It also tracks some spans and delimiter kinds better for fn-like macros and macro definitions.

I've been talking about implementing this with @nnethercote in rust-lang#65750 (comment).
The parsed representation is closer to `MetaItem` and requires less token juggling during conversions, so it potentially may be faster.

r? @Centril
Centril added a commit to Centril/rust that referenced this pull request Dec 14, 2019
doc comments: Less attribute mimicking

Make sure doc comments are not converted into intermediate meta-items, or not mixed with `doc(inline)` or something like that.

Follow-up to rust-lang#65750.
bors added a commit that referenced this pull request Dec 28, 2019
doc comments: Less attribute mimicking

Make sure doc comments are not converted into intermediate meta-items, or not mixed with `doc(inline)` or something like that.

Follow-up to #65750.
calebcartwright added a commit to calebcartwright/rustfmt that referenced this pull request Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants