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

Do not convert doc comments into #[doc = "..."] form unless necessary for macros #60935

Closed
petrochenkov opened this issue May 18, 2019 · 3 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@petrochenkov
Copy link
Contributor

petrochenkov commented May 18, 2019

Keep them as tokens in AST, this may partially mitigate the perf impact from turning normal comments into doc comments (#60930).

pub struct Attribute {
    pub id: AttrId,
    pub style: AttrStyle,
    pub path: Path,
    pub tokens: TokenStream,
    pub is_sugared_doc: bool,
    pub span: Span,
}

=>

pub struct Attribute {
    pub kind: AttrKind,
    pub span: Span,
    pub id: AttrId,
}

pub enum AttrKind {
    Normal(Path, TokenStream, AttrStyle),
    Doc(Symbol),
}
@Centril Centril added I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-enhancement Category: An issue proposing an enhancement or a PR with one. labels May 18, 2019
@jonas-schievink jonas-schievink added I-compiletime Issue: Problems and improvements with respect to compile times. and removed I-slow Issue: Problems and improvements with respect to performance of generated code. labels May 18, 2019
@nnethercote
Copy link
Contributor

Because TokenStream contains a Lrc<Vec<TreeAndJoint>>, this would avoid two allocations per doc comment line: one for the Lrc and one for the Vec. That would get a big chunk of the potential saving. (We would still need one symbol interning per line, which involves a TLS access and a hash table.)

@nnethercote
Copy link
Contributor

I have a local patch that changes TokenStream to a Lrc<SmallVec<[TreeAndJoint; 2]>>. It's a clear win, but it only gets rid of one of the allocations, so the approach suggested in this PR has a greater potential.

@nnethercote
Copy link
Contributor

I have implemented this idea in #65750. It works well!

bors added a commit that referenced this issue 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 bors closed this as completed in eea6f23 Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants