[SERVER-77158] Enable clang-tidy move const variable check and other viable performance checks Created: 15/May/23 Updated: 29/Dec/23 |
|
| Status: | Open |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Improvement | Priority: | Major - P3 |
| Reporter: | Matt Boros | Assignee: | Unassigned |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | greenerbuild | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||
| Assigned Teams: |
Query Optimization
|
||||||||||||||||
| Sprint: | QO 2023-05-29, QO 2023-06-12, QO 2023-06-26, QO 2023-07-10, QO 2023-07-24, QO 2023-08-07, QO 2023-08-21, QO 2023-09-04, QE 2023-09-18, QE 2023-10-02, QE 2023-10-16, QE 2023-10-30, QE 2023-11-13, QE 2023-11-27, QE 2023-12-11, QE 2023-12-25, QE 2024-01-08 | ||||||||||||||||
| Participants: | |||||||||||||||||
| Linked BF Score: | 135 | ||||||||||||||||
| Description |
|
Clang tidy has a whole suite of performance-related checks we could enable that have not yet been investigated. A check for moving a const variable is one of them. Here is the full list (scroll down to performance-*). We should investigate which of these could be enabled, and fix any issues we find. |
| Comments |
| Comment by Alex Neben [ 29/Dec/23 ] |
|
Reopening and moving to epic |
| Comment by Githook User [ 25/Oct/23 ] |
|
Author: {'name': 'Matt Boros', 'email': 'matt.boros@mongodb.com', 'username': 'mattBoros'}Message: SERVER-77158 apply fixes from move-const-arg to db/s |
| Comment by Githook User [ 08/Oct/23 ] |
|
Author: {'name': 'Matt Boros', 'email': 'matt.boros@mongodb.com', 'username': 'mattBoros'}Message: SERVER-77158 apply performance-move-const-arg clang tidy rule for replication |
| Comment by Githook User [ 07/Oct/23 ] |
|
Author: {'name': 'Matt Boros', 'email': 'matt.boros@mongodb.com', 'username': 'mattBoros'}Message: SERVER-77158 apply performance-move-const-arg clang tidy rule for client |
| Comment by Githook User [ 07/Oct/23 ] |
|
Author: {'name': 'Matt Boros', 'email': 'matt.boros@mongodb.com', 'username': 'mattBoros'}Message: SERVER-77158 apply performance-move-const-arg clang tidy rule for resharding |
| Comment by Githook User [ 29/Sep/23 ] |
|
Author: {'name': 'Matt Boros', 'email': 'matt.boros@mongodb.com', 'username': 'mattBoros'}Message: SERVER-77158 apply performance-move-const-arg clang tidy rule for db/exec |
| Comment by Githook User [ 28/Sep/23 ] |
|
Author: {'name': 'Matt Boros', 'email': 'matt.boros@mongodb.com', 'username': 'mattBoros'}Message: SERVER-77158 apply performance-move-const-arg clang tidy rule for db/storage |
| Comment by Matt Boros [ 22/Sep/23 ] |
|
I'm a bit ahead of the reviews right now, so I'm going to stop posting PRs until more get approved + committed. |
| Comment by Githook User [ 19/Sep/23 ] |
|
Author: {'name': 'Matt Boros', 'email': 'matt.boros@mongodb.com', 'username': 'mattBoros'}Message: SERVER-77158 enable low risk clang tidy perf rules |
| Comment by Githook User [ 17/Sep/23 ] |
|
Author: {'name': 'Matt Boros', 'email': 'matt.boros@mongodb.com', 'username': 'mattBoros'}Message: SERVER-77158 apply performance-move-const-arg clang tidy rule for document sources |
| Comment by Githook User [ 16/Sep/23 ] |
|
Author: {'name': 'Matt Boros', 'email': 'matt.boros@mongodb.com', 'username': 'mattBoros'}Message: SERVER-77158 apply performance-move-const-arg clang tidy rule for bonsai |
| Comment by Matt Boros [ 13/Sep/23 ] |
|
performance-type-promotion-in-math-fn, performance-move-constructor-init, performance-trivially-destructible are in review.
performance-move-const-arg is a larger change, so I've started with three PRs that are reasonably sized. One for SBE code, one for document source code, and one for Bonsai code. All of these diffs are the automatic fixes which I've skimmed but need to look more closely to see if the changes are actually what we want.
|
| Comment by Githook User [ 11/Sep/23 ] |
|
Author: {'name': 'Matt Boros', 'email': 'matt.boros@mongodb.com', 'username': 'mattBoros'}Message: SERVER-77158 apply selected changes from performance-inefficient-vector-operation clang-tidy suggestions |
| Comment by Matt Boros [ 11/Sep/23 ] |
|
Here's the doc I'll be updating as I work on this: https://docs.google.com/document/d/1yHDTEj6M9gsOzELxRTZ3G_-Yu1jivhTMB6upr-M8hJ8/edit?usp=sharing |
| Comment by Matt Boros [ 11/Sep/23 ] |
|
Filed |
| Comment by Matt Boros [ 11/Sep/23 ] |
|
Ah yea good point, I'll make a doc to summarize the decisions for each rule. I'll send the draft out for that later today. We felt the performance-inefficient-vector-operation was too general and can clutter code. In some hot paths a reserve() call is good, but in most cases it's not needed (this showed in the lack of perf improvement). I think the VSCode tooltip is a good compromise. |
| Comment by Matt Boros [ 11/Sep/23 ] |
|
For performance-inefficient-vector-operation, we've decided to not enable the check but commit some of the recommended fixes from this rule. I'll also file a ticket to have VSCode suggest vector.reserve() using the clang tidy rule. So there will be required rules (enforced in evergreen) and recommended rules as tooltips. |
| Comment by Matt Boros [ 23/Jun/23 ] |
|
Still have higher priority BFs unfortunately. I'm hoping that as we get through 7.0 perf issues I'll be able to work on this on BF days. |
| Comment by Matt Boros [ 02/Jun/23 ] |
|
Just to update this, I have some higher priority work right now and will be working on this in the time I have in between. |
| Comment by Matt Boros [ 18/May/23 ] |
|
Yea that would work! I suppose we could do a bunch of PRs with fixes, and then in the last one we would actually commit the new clang-tidy rule. I could try to split it by team, and find assignees from there. I'm looking into other rules as well! This was the cleanest one to implement so far. There are some rules that would certainly be beneficial, but maybe are too strict to enforce. This one comes to mind. I think with enough "AllowedTypes" it could be done, but it also might become a burden to keep adding to this list as we add new code. I'll try to look into the most straightforward rules first. This one would also certainly have some performance benefit. For example all of the NamespaceString copying work that's been done would be detected by this rule. |
| Comment by Alex Neben [ 17/May/23 ] |
|
Can I just suggest arbitrarily breaking the PR up into groups of like 30 fixes or so? It seems like it wouldn't make a difference but our team has found that people will be more focused when reviewing small portions of changes even if the total review burden is the same. For the clang_tidy_fix I'm happy to review that part now matter how you structure your PR(s).
FWIW this looks really good! Also be adding this clang-tidy rule you will prevent issues like this in the future as well! |
| Comment by Matt Boros [ 16/May/23 ] |
|
Thanks for pointing that out! Did not see that at first. |
| Comment by Alex Neben [ 16/May/23 ] |
|
There is an option for https://releases.llvm.org/12.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/performance-move-const-arg.html#cmdoption-arg-checktriviallycopyablemove ignoring trivially copyable move which should make the implementation of this simpler. |