[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:
Duplicate
Problem/Incident
Related
related to SERVER-80944 Enable more clang-tidy rules as VSCod... Closed
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
Branch: master
https://github.com/mongodb/mongo/commit/2cb802018f3ac5505bc401081a46b01ddb17962e

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
Branch: master
https://github.com/mongodb/mongo/commit/57624bec5559121c5c526c8164357dbf48a57e77

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
Branch: master
https://github.com/mongodb/mongo/commit/3e13066dddf16a36226c7dea1a0aad5bd3dbab2b

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
Branch: master
https://github.com/mongodb/mongo/commit/6d33c1402599d95d46f50728c14d86a6ec3c1070

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
Branch: master
https://github.com/mongodb/mongo/commit/acd780fc1ea623fb067332b48b72ee3c80b2d20a

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
Branch: master
https://github.com/mongodb/mongo/commit/10e00900a10bd75e06ed81868f6177653c5031ed

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
Branch: master
https://github.com/mongodb/mongo/commit/6bca90642592cfa9ef57f852adc472c07e6b3a06

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
Branch: master
https://github.com/mongodb/mongo/commit/669f87e05810e70c9586a5ff46d44aeb550992ad

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
Branch: master
https://github.com/mongodb/mongo/commit/fb2b091ba4903be99fbe9b3535cc309814199460

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
Branch: master
https://github.com/mongodb/mongo/commit/9d27bb3f4e66c6cf1982f436aaea4ec5229a3ddf

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 SERVER-80944 for the VSCode suggestions.

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.

Generated at Thu Feb 08 06:34:41 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.