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.

Sign up for GitHub

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

Jump to bottom

readability-identifier-naming reports nothing but does something when no CheckOptions are provided #58096

Closed
firewave opened this issue Oct 1, 2022 · 12 comments · Fixed by #92659
Closed

readability-identifier-naming reports nothing but does something when no CheckOptions are provided #58096

firewave opened this issue Oct 1, 2022 · 12 comments · Fixed by #92659
Labels
clang-tidy performance

Comments

@firewave
Copy link

firewave commented Oct 1, 2022

#57527 (comment) indicates that readability-identifier-naming is one of the heavier clang-tidy checks. But it seems that it doesn't do anything until you actually provide CheckOptions.

Running -dump-config only shows the following default values:

CheckOptions:
  - key:             readability-identifier-naming.IgnoreFailedSplit
    value:           'false'
  - key:             readability-identifier-naming.AggressiveDependentMemberLookup
    value:           'false'
  - key:             readability-identifier-naming.IgnoreMainLikeFunctions
    value:           'false'
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 1, 2022

@llvm/issue-subscribers-clang-tidy

@firewave
Copy link
Author

The check is among the heaviest so it not doing anything is rather bad (the *-identifiers checks are still up there as I am still using LLVM 16 on this system):

   ---User Time---   --System Time--   --User+System--   ---Wall Time---  --- Name ---
   0.2999 ( 19.2%)   0.0807 (  7.3%)   0.3806 ( 14.2%)   0.3791 ( 14.2%)  bugprone-reserved-identifier
   0.1173 (  7.5%)   0.0884 (  8.0%)   0.2056 (  7.7%)   0.2054 (  7.7%)  misc-confusable-identifiers
   0.0804 (  5.2%)   0.0648 (  5.8%)   0.1452 (  5.4%)   0.1449 (  5.4%)  readability-identifier-naming

@firewave
Copy link
Author

It looks like the performance issue was resolved in LLVM 17:

LLVM 16

   0.2969 (  7.9%)   0.2031 (  9.7%)   0.5000 (  8.5%)   0.3507 (  6.0%)  readability-identifier-naming

LLVM 17

   0.0312 (  1.0%)   0.0000 (  0.0%)   0.0312 (  0.6%)   0.0297 (  0.6%)  readability-identifier-naming

LLVM 18

   0.1094 (  1.4%)   0.0156 (  0.3%)   0.1250 (  1.0%)   0.1142 (  0.9%)  readability-identifier-naming

@firewave firewave mentioned this issue May 12, 2024
Open
@zufuliu
Copy link

zufuliu commented May 18, 2024

clang-tidy built from today's 195ba45, for https://github.com/zufuliu/notepad2/blob/main/scintilla/src/Editor.cxx with https://github.com/zufuliu/notepad2/blob/main/.clang-tidy:

with readability-identifier-naming check:

===-------------------------------------------------------------------------===
                          clang-tidy checks profiling
===-------------------------------------------------------------------------===
  Total Execution Time: 14.2812 seconds (21.2410 wall clock)

   ---User Time---   --System Time--   --User+System--   ---Wall Time---  --- Name ---
   1.4375 ( 29.2%)   8.9219 ( 95.3%)  10.3594 ( 72.5%)  14.1364 ( 66.6%)  readability-identifier-naming
   0.2969 (  6.0%)   0.0156 (  0.2%)   0.3125 (  2.2%)   0.6729 (  3.2%)  bugprone-reserved-identifier
   0.2500 (  5.1%)   0.0000 (  0.0%)   0.2500 (  1.8%)   0.5217 (  2.5%)  misc-const-correctness
   0.2031 (  4.1%)   0.0312 (  0.3%)   0.2344 (  1.6%)   0.3473 (  1.6%)  bugprone-standalone-empty
   0.1562 (  3.2%)   0.0156 (  0.2%)   0.1719 (  1.2%)   0.3148 (  1.5%)  bugprone-use-after-move
   0.0781 (  1.6%)   0.0000 (  0.0%)   0.0781 (  0.5%)   0.2655 (  1.3%)  misc-unused-using-decls
   0.0938 (  1.9%)   0.0312 (  0.3%)   0.1250 (  0.9%)   0.2541 (  1.2%)  bugprone-stringview-nullptr

without readability-identifier-naming check:

===-------------------------------------------------------------------------===
                          clang-tidy checks profiling
===-------------------------------------------------------------------------===
  Total Execution Time: 3.7656 seconds (5.6693 wall clock)

   ---User Time---   --System Time--   --User+System--   ---Wall Time---  --- Name ---
   0.3438 ( 10.8%)   0.0000 (  0.0%)   0.3438 (  9.1%)   0.4814 (  8.5%)  bugprone-reserved-identifier
   0.2656 (  8.3%)   0.0000 (  0.0%)   0.2656 (  7.1%)   0.3902 (  6.9%)  misc-const-correctness
   0.1719 (  5.4%)   0.0312 (  5.4%)   0.2031 (  5.4%)   0.2752 (  4.9%)  bugprone-standalone-empty
   0.1250 (  3.9%)   0.0000 (  0.0%)   0.1250 (  3.3%)   0.2540 (  4.5%)  bugprone-use-after-move
   0.1562 (  4.9%)   0.0312 (  5.4%)   0.1875 (  5.0%)   0.2185 (  3.9%)  misc-unused-using-decls
   0.1719 (  5.4%)   0.0312 (  5.4%)   0.2031 (  5.4%)   0.2065 (  3.6%)  bugprone-stringview-nullptr

@PiotrZSL
Copy link
Member

@zufuliu older clang-tidy also take so long, or you just tested latest ?

@zufuliu
Copy link

zufuliu commented May 18, 2024

tested latest built from today's main.

@zufuliu
Copy link

zufuliu commented May 18, 2024

@PiotrZSL should I create a different issue for the performance problem (regression from 18.1.5), following are results with readability-identifier-naming check using clang-tidy 18.1.5:

===-------------------------------------------------------------------------===
                          clang-tidy checks profiling
===-------------------------------------------------------------------------===
  Total Execution Time: 5.2969 seconds (10.2236 wall clock)

   ---User Time---   --System Time--   --User+System--   ---Wall Time---  --- Name ---
   0.7031 ( 15.1%)   0.0156 (  2.5%)   0.7188 ( 13.6%)   1.4139 ( 13.8%)  bugprone-reserved-identifier
   0.3750 (  8.0%)   0.0000 (  0.0%)   0.3750 (  7.1%)   0.7435 (  7.3%)  misc-const-correctness
   0.2812 (  6.0%)   0.0000 (  0.0%)   0.2812 (  5.3%)   0.5210 (  5.1%)  bugprone-standalone-empty
   0.3281 (  7.0%)   0.0469 (  7.5%)   0.3750 (  7.1%)   0.4787 (  4.7%)  bugprone-use-after-move
   0.1719 (  3.7%)   0.0000 (  0.0%)   0.1719 (  3.2%)   0.3236 (  3.2%)  bugprone-stringview-nullptr
   0.1562 (  3.3%)   0.0156 (  2.5%)   0.1719 (  3.2%)   0.3053 (  3.0%)  misc-unused-using-decls
   0.1094 (  2.3%)   0.0156 (  2.5%)   0.1250 (  2.4%)   0.2490 (  2.4%)  readability-non-const-parameter
   0.0469 (  1.0%)   0.0000 (  0.0%)   0.0469 (  0.9%)   0.2175 (  2.1%)  readability-container-size-empty
   0.0781 (  1.7%)   0.0938 ( 15.0%)   0.1719 (  3.2%)   0.2017 (  2.0%)  bugprone-unused-return-value

@PiotrZSL
Copy link
Member

I don't think that this need separate issue. This issue originally was only about readability-identifier-naming searching for .clang-tidy config files in other locations, and that's why initially this check were heavy. To be honest when overall performance can be still improved for this check. This issue (#58096) cannot be fully fixed, as that's per check design. So cost of check will never be close to 0.

Currently I need to be able to reproduce issue locally, preferably on ubuntu.

@zufuliu
Copy link

zufuliu commented May 18, 2024

Currently I need to be able to reproduce issue locally, preferably on ubuntu.

following steps (based on command line https://github.com/zufuliu/notepad2/blob/main/scintilla/tidy.bat) may be able to reproduce the regression on Ubuntu:

# see https://sourceforge.net/p/scintilla/code/ci/default/tree/ for Read Only access
hg clone http://hg.code.sf.net/p/scintilla/code scintilla
cd scintilla/src
clang-tidy --enable-check-profile -checks=readability-identifier-naming Editor.cxx -- -std=c++20 -I../include 1>tidy.log

@PiotrZSL
Copy link
Member

@zufuliu Whats causing a slowdown in this check is actually an harddrive:
`1.4375 ( 29.2%) 8.9219 ( 95.3%) 10.3594 ( 72.5%) 14.1364 ( 66.6%) readability-identifier-naming

Check is trying to get "realpath" for every finding.
This functionality were added by #81985 in Clang-tidy 19.
Created fix in #92659

@PiotrZSL PiotrZSL mentioned this issue May 18, 2024
Merged
@PiotrZSL PiotrZSL linked a pull request May 18, 2024 that will close this issue
[clang-tidy] Optimize realpath in readability-identifier-naming #92659
Merged
@zufuliu
Copy link

zufuliu commented May 18, 2024

performance regression indeed fixed by the PR:

   0.0469 (  1.1%)   0.0000 (  0.0%)   0.0469 (  0.9%)   0.0435 (  0.8%)  readability-identifier-naming

@firewave
Copy link
Author

@PiotrZSL

This issue (#58096) cannot be fully fixed, as that's per check design. So cost of check will never be close to 0.

Maybe the documentation can be improved by stating that you need to explicitly specify the naming you want to apply with an additional note that you can disable it if if do not use it.

Or is it possible for the check to detect if any configuration is actually specified (well - it has to because somehow it decides to report something or nothing) and bail out?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
clang-tidy performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[clang-tidy] Optimize realpath in readability-identifier-naming PiotrZSL/llvm-project
5 participants
@firewave @PiotrZSL @zufuliu @EugeneZelenko @llvmbot