This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] New checker performance-trivially-destructible-check
ClosedPublic

Authored by AntonBikineev on Oct 25 2019, 9:10 AM.

Details

Summary

Checks for types which can be made trivially-destructible by removing
out-of-line defaulted destructor declarations.

The check is motivated by the work on C++ garbage collector in Blink
(rendering engine in Chrome), which strives to minimize destructor usage
and improve runtime of sweeping phase.

In the entire chromium codebase the check hits over ~2500 times.

Diff Detail

Event Timeline

AntonBikineev created this revision.Oct 25 2019, 9:10 AM
lebedev.ri added inline comments.Oct 25 2019, 9:24 AM
clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp
23

I believe static functions are preferred to anon namespaces;
most (if not all) of this checking should/could be done in the matcher itself,
i.e. the check() could be called to unconditionally issue diag.

51

https://en.cppreference.com/w/cpp/language/destructor says defaulted destructor is C++11 feature,
so not register if this isn't C++11?

78

This always drops out-of-line defaulting.
Is there/can there be any case where such defaultig should just be moved in-line into the class?

clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.h
19–20

This doesn't read right, should be closer to this maybe

/// A check that finds classes that would be trivial if not for the defaulted destructors declared out-of-line:
clang-tools-extra/docs/ReleaseNotes.rst
142

s/destrictuble/destructible/?

clang-tools-extra/docs/clang-tidy/checks/performance-trivially-destructible.rst
7

s/destrictuble/destructible/?

clang-tools-extra/test/clang-tidy/checkers/performance-trivially-destructible.cpp
2

Do you also want to check that fix-it applies and result passes sema?

Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp
30

Please don't use auto unless type is spelled in same statement or iterator.

32

Please don't use auto unless type is spelled in same statement or iterator.

39

Please don't use auto unless type is spelled in same statement or iterator.

62

Please don't use auto unless type is spelled in same statement or iterator.

67

Please don't use auto unless type is spelled in same statement or iterator.

clang-tools-extra/docs/ReleaseNotes.rst
139

Please move into new checks list (in alphabetical order).

Eugene.Zelenko edited reviewers, added: alexfh, hokein; removed: klimek.Oct 25 2019, 10:19 AM
AntonBikineev marked 9 inline comments as done.Oct 25 2019, 12:00 PM
AntonBikineev added inline comments.
clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp
23

I couldn't find matchers to check triviallity of special functions or something like isFirstDecl(). Perhaps, would make sense to add some.

51

Right. However, I think it will also make sense to extend the check for destructors with empty bodies.

78

I've thought about this. I think it mostly comes down to code-style preference (rule-of-5 vs rule-of-0). But after giving it another thought, I think, the current implementation may lead to more surprises for the user, so probably moving = default to the first declaration would be a better idea :)

clang-tools-extra/test/clang-tidy/checkers/performance-trivially-destructible.cpp
2

Aren't fix-its already checked by CHECK-FIXES?
It would be great to check if the result is compilable. Should I run another clang_tidy instance to check that or is there another way to do so?

AntonBikineev marked 2 inline comments as done.

Thanks for the suggestions! Addressed some of them.

dmajor added a subscriber: dmajor.Oct 25 2019, 6:27 PM

Roman, could you please take another look at it?

lebedev.ri added inline comments.Oct 28 2019, 5:29 AM
clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp
23
clang-tools-extra/test/clang-tidy/checkers/performance-trivially-destructible.cpp
2
aaron.ballman added inline comments.Oct 28 2019, 1:06 PM
clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp
27

Drop top-level const qualifiers unless it's a pointer or reference (it's not a style we use elsewhere).

39

We don't usually attach names to TODOs -- are you planning to work on that for this patch?

71

it -> its

Thanks for the suggestions!

Added isTriviallyDestructible matcher to utils/Matchers.h.

Does anybody have suggestions on this?

lebedev.ri marked an inline comment as done.EditedOct 30 2019, 11:43 PM

Does anybody have suggestions on this?

Can you be more specific what the question is?

clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp
43

Thanks, this looks good.

Roman, thanks for reviewing and the ideas.

Can you be more specific what the question is?

I meant to ping if anybody has more comments on this patch. Otherwise I would go ahead and submit it.

Did you see some significant perf improvements for Chromium?

Did you see some significant perf improvements for Chromium?

I don't see major improvements in browser benchmarks like Speedometer, but in the Blink garbage collector, which queues destructors to be executed later, this change proved to reduce the number of them by 5%. That, in turn, reduces GC time on the main thread which is significant for latency.

BTW, this very contrived benchmark shows 5x: http://quick-bench.com/sczBi_lVndKut9jOj4UofC0HYew

There is no difference in perf for GCC.

There is no difference in perf for GCC.

Yes, but that's because gcc still optimizes the call to noinlined function. In the common scenario which this check addresses (destructor defaulted in .cpp file) gcc is not able to optimize the call.

LGTM modulo the isFirstDeclComment. Will approve after we resolve that discussion.

clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp
46

The "isFirstDecl" part probably will always work, however, it is semantically not really the right predicate to check. What we want to check is where the constructor is declared -- in the class or outside the class, correct? If so, then the matcher should be looking at the DeclContext of the constructor decl.

AntonBikineev marked an inline comment as done.Oct 31 2019, 2:22 PM
AntonBikineev added inline comments.
clang-tools-extra/clang-tidy/performance/TriviallyDestructibleCheck.cpp
46

Thanks for the comment.

What we want to check is where the constructor is declared -- in the class or outside the class, correct?

I was following the Standard's wording, which says that trivial destructor is not user-provided, where user-provided means

user-declared and not explicitly defaulted or deleted on its first declaration.

Technically, I think both ways should work as C++ doesn't allow more than 2 special function declarations.

mgehre removed a subscriber: mgehre.Oct 31 2019, 3:01 PM
gribozavr2 accepted this revision.Oct 31 2019, 5:02 PM

Alright then. (Although I don't know whether the redeclaration chain always models that wording in the standard, and what that wording means when we don't have a linear ordering of text -- that is, in modules.)

Do you have commit access or do you need me to commit for you?

This revision is now accepted and ready to land.Oct 31 2019, 5:02 PM

Alright then. (Although I don't know whether the redeclaration chain always models that wording in the standard, and what that wording means when we don't have a linear ordering of text -- that is, in modules.)

Do you have commit access or do you need me to commit for you?

Thanks for taking the look, I'll commit.

This revision was automatically updated to reflect the committed changes.