Page MenuHomePhabricator

[clang][ASTContext] Simplify caching for declaration-related comments
AbandonedPublic

Authored by jkorous on Apr 8 2019, 5:44 PM.

Details

Summary
  • I assume we can cache comments for canonical declarations only, not for every redeclaration.
  • Caching that we didn't find comments seems to be unnecessary since we try to search for comment again again next time if we find this data in cache.
    • We might implement proper cache invalidation in the future and get back to using this information.
  • Origin of comment (directly from declaration / from some redeclaration) seems to not be used anywhere.

I plan to do some performance testing before committing but like to have some feedback first.

BTW there's another cache for comments in the ASTContext - ParsedComments. We could try to experiment with different caching approaches to see how it affects performance - maybe caching mapping from canonical declarations to raw comments and separately caching mapping from raw comments to full comments.

Diff Detail

Event Timeline

jkorous created this revision.Apr 8 2019, 5:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2019, 5:44 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
arphaman added inline comments.Apr 9 2019, 8:16 AM
clang/lib/AST/ASTContext.cpp
372

Why are we now checking for the canonical declaration instead of D as before?

jkorous marked an inline comment as done.Apr 9 2019, 6:28 PM
jkorous added inline comments.
clang/lib/AST/ASTContext.cpp
372

Before this we were caching comment for every redeclaration separately but for every redeclaration the comment was the same.

As I understand it - for a given declaration we found the first comment in the redeclaration chain and then saved it to the cache for every redeclaration (the same comment).
Later, during lookup, we iterated the redeclaration chain again and did a lookup for every redeclaration (see L392 in the original implementation).

Unless I missed something, it's suboptimal in both memory consumption and runtime overhead.

That's the reason why I want to cache the comment for the redeclaration group as a whole. The only thing I am not entirely sure about is what key to use to represent the whole redeclaration chain - maybe the first declaration in the redeclaration chain would be better then the canonical declaration...

WDYT?

gribozavr added inline comments.Apr 10 2019, 2:26 AM
clang/include/clang/AST/ASTContext.h
756

The name of this variable (and RawCommentAndCacheFlags) don't make sense after the change.

clang/lib/AST/ASTContext.cpp
372

As I understand it - for a given declaration we found the first comment in the redeclaration chain and then saved it to the cache for every redeclaration (the same comment).

Only if there was only one comment in the redeclaration chain. If a redeclaration had a different comment, this function would return that comment.

jkorous marked an inline comment as done.Apr 10 2019, 3:55 PM
jkorous added inline comments.
clang/lib/AST/ASTContext.cpp
372

I see. I made a wrong assumption - that for any two redeclarations the redeclaration chain always starts from the same declaration.

jkorous updated this revision to Diff 194604.Apr 10 2019, 4:03 PM

Second attempt on reducing the cache size and number of operations.

I try in this order

  1. cache lookup for the specific provided declaration
  2. try to find comment attached to the specific provided declaration without using cache (and cache it using the specific declaration as a key)
  3. cache lookup using the canonical declaration (which would return comment from any redeclaration - see the next step)
  4. try to find comment attached to any redeclaration (and cache it using the canonical declaration as a key)
  5. give up, return nullptr
jkorous updated this revision to Diff 194609.Apr 10 2019, 4:10 PM

rename RawCommentAndCacheFlags -> CommentAndOrigin

jkorous marked an inline comment as done.Apr 10 2019, 4:11 PM
gribozavr added inline comments.Apr 12 2019, 2:10 AM
clang/include/clang/AST/ASTContext.h
728

RawCommentAndOrigin?

751

Please remove "canonical", it is not correct according to the implementation.

Please also update the patch description that currently says "I assume we can cache comments for canonical declarations only, not for every redeclaration."

clang/lib/AST/ASTContext.cpp
372

I still don't think this patch is a no-op.

Here is the intended behavior:

/// First
void foo(); // (1), canonical decl

void foo(); // (2)

/// Third
void foo() {} // (3)

void foo(); // (4)

getRawCommentForAnyRedecl(1) => "First"
getRawCommentForAnyRedecl(2) => "First"
getRawCommentForAnyRedecl(3) => "Third"
getRawCommentForAnyRedecl(4) => "First"

373

GetCommentFromCache?

405

No space after *.

415

I don't understand the comment -- I read it as "if we got here, and the decl was canonical, and we haven't found anything already, we should return nullptr". The code does something different.

Also, just to be clear, here is the intended behavior:

void foo(); // (1), canonical decl

/// Second
void foo(); // (2)

getRawCommentForAnyRedecl(1) => "Second"
getRawCommentForAnyRedecl(2) => "Second"

418

When is it possible to not have a canonical decl?

421

No space after *.

425

80 columns.

jkorous abandoned this revision.Apr 25 2019, 4:04 PM