Introduce parameter to disable CxxSquidSensor #2080

andreydanin · 2021-03-12T19:48:40Z

Fix for #2228

SquidSensor can take significant amount of time on big projects.
Skip execution if sonar.cxx.squid.disabled parameter is set to true.

This change is

guwirth · 2021-03-12T21:23:49Z

Hi @andreydanin, think that is not working because squid is also used for indexing, calculating metrics, syntax highlighting ...

andreydanin · 2021-03-12T21:36:08Z

@guwirth probably you are right. I created this patch to avoid long squid scan for our pull request jobs (I disabled all cxx rules for that job in a Quality Profile). However I still can run squid if at least one cxx rule is enabled in a Quality Profile (for nightly builds in my case).

Another solution that I had in mind is to implement an option that will stop squid from running.
What is your opinion?

guwirth · 2021-03-13T07:22:42Z

@andreydanin think squid in general is not slow, the problem is the preprocessor and the include files. Easiest to try is to set no (less) sonar.cxx.includeDirectories. Have you tried this?

The general issue with this is, that SonarQube needs always all source files and reports, also for a PR. There is no „incremental analysis“. Maybe you can ask this as a general question here: https://community.sonarsource.com/.

To understand this better. How big is your code base? What execution time we are talking about?

andreydanin · 2021-03-13T07:42:05Z

To understand this better. How big is your code base? What execution time we are talking about?

2+ M lines of code. Execution time is 40+ minutes.
Squid is ok, but if project is big enough execution time is significant.

The general issue with this is, that SonarQube needs always all source files and reports, also for a PR. There is no „incremental analysis“. Maybe you can ask this as a general question here: https://community.sonarsource.com/.

We use other analyzers for PR as well. SonarQube (with witsonar-cxx plugin) is one of the tools. It would be nice to be able exclude specific checks/ plugins.

Easiest to try is to set no (less) sonar.cxx.includeDirectories. Have you tried this?

Not yet. I will try different parameters as you suggested.

guwirth · 2021-03-13T08:07:00Z

@andreydanin

It would be nice to be able exclude specific checks/ plugins.

you can turn off a plugin by setting an invalid file extension sonar.cxx.file.suffixes (same for other plugins)
you can turn off checks by creating a Quality Profile with more/less rules and use this for the PR

Did you turn debug info on to see where exactly the time is used up?
https://github.com/SonarOpenCommunity/sonar-cxx/wiki/Get-Debug-Information

2+ M lines of code. Execution time is 40+ minutes.

That‘s slow. Guess it’s because of many includes or #1685

Which version of the cxx plugin and SonarQube are you using?

andreydanin · 2021-03-13T08:16:12Z

* you can turn off a plugin by setting an invalid file extension `sonar.cxx.file.suffixes` (same for other plugins)

Good suggestion. I will try.

* you can turn off checks by creating a Quality Profile with more/less rules and use this for the PR

No, squid for c/c++ is executed even if all rules are disabled.

Did you turn debug info on to see where exactly the time is used up?
https://github.com/SonarOpenCommunity/sonar-cxx/wiki/Get-Debug-Information

Yes. This is how I found that squid for c/c++ is slowest part of check in our environment.

That‘s slow. Guess it’s because of many includes or #1685

Thanks, I will check the discussion.

Which version of the cxx plugin and SonarQube are you using?

SonarQube 7.9.4.35981, cxx plugin - 1.3.2 (with my patch).

guwirth · 2021-03-13T08:51:27Z

@andreydanin in case you wanna proceed in this direction

you should add a switch turning the preprocessor off

sonar-cxx/cxx-squid/src/main/java/org/sonar/cxx/parser/CxxParser.java

Line 54 in 76e71ae

var cxxpp = new CxxPreprocessor(context, squidConfig);

. Try it without the preprocessor.
an internal switch registering only the active rules to be activated

sonar-cxx/sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/CxxSquidSensor.java

Line 283 in 76e71ae

var visitors = new ArrayList<SquidAstVisitor<Grammar>>(checks.all());

. To play you can forward an empty list here to see if this makes any difference.

guwirth · 2021-03-14T09:10:55Z

@andreydanin maybe you can try and give feedback. Is this faster?

sonar-cxx/sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/CxxSquidSensor.java

Line 283 in 76e71ae

var visitors = new ArrayList<SquidAstVisitor<Grammar>>(checks.all());

@Override
  public void execute(SensorContext context) {
    this.context = context;

    // add vistor only if corresponding rule is active
    var visitors = new ArrayList<SquidAstVisitor<Grammar>>();
    for (var check : checks.all()) {
      RuleKey key = checks.ruleKey(check);
      if (key != null) {
        if (context.activeRules().find(key) != null) {
          visitors.add(check);
        }
      }
    }

    AstScanner<Grammar> scanner = CxxAstScanner.create(createConfiguration(),
                                                       visitors.toArray(new SquidAstVisitor[visitors.size()]));
...

- add visitor only if corresponding rule is active - close SonarOpenCommunity#2080

guwirth · 2021-03-14T18:19:12Z

@andreydanin see also #2082, will wait for you feedback...

andreydanin · 2021-03-17T21:26:53Z

@andreydanin see also #2082, will wait for you feedback...

Thanks for the quick responses. I really appreciate it!
Unfortunately I can try your patch not earlier than this weekend.

guwirth · 2021-03-26T18:11:52Z

@andreydanin any news?

andreydanin · 2021-03-28T22:18:50Z

@andreydanin any news?

Sorry for the delay. It took more time than I expected. I've performed few tests and unfortunately I don't see big difference on full and empty CXX profile with "optimize squid sensor #2082" patch. Full test is much faster (~10 minutes vs ~36 minutes) on my machine (i7 with SSD) then on machine where I performed tests initially.
Here is short version of logs (-X -Dsonar.verbose=true): https://pastebin.com/qAZnSSwQ

"-Dsonar.exclusions=**/*.cpp" disables CXX (execution time is much faster than 8-9 minutes) but I haven't checked if cppcheck report is processed in this case.

Please let me know if you need more information or have other ideas about things that I should check.

guwirth · 2021-03-29T09:44:39Z

@andreydanin thx for testing. It's hard for me to see from the log file which log is with/without the patch?

I don't see big difference

How big is the difference?

andreydanin · 2021-03-29T10:42:15Z

@andreydanin thx for testing. It's hard for me to see from the log file which log is with/without the patch?
How big is the difference?

Branch:
optimize-squid-visitors-and-squid-conditional - contains both patches (my and yours)
optimize-squid-visitors - contains only your patch

Mode:
cpp means that I enabled only .cxx and .cpp extensions (I ignored .c, .h, etc.)

Profile:
empty - "Sonar way", 0 enabled rules
full - 5425 cxx rules are enabled

CXX_time:
value from log string "Sensor CXX [cxx] (done) | time=<value>"
270000ms = 4.5 min

Total_time:
value from log string "INFO: Total time: <value> s"

Branch                                         Mode  Profile  CXX_time  Total_time
optimize-squid-visitors-and-squid-conditional  cpp   empty    none      2:40.589
optimize-squid-visitors-and-squid-conditional  cpp   full     277759ms  7:36.565
optimize-squid-visitors                        cpp   empty    340701ms  9:19.440
optimize-squid-visitors                        cpp   empty    267705ms  7:25.019
optimize-squid-visitors                        cpp   empty    280359ms  7:37.511
optimize-squid-visitors                        cpp   empty    265813ms  7:29.725
optimize-squid-visitors                        cpp   empty    262919ms  7:20.962
optimize-squid-visitors                        cpp   full     270328ms  7:21.942
optimize-squid-visitors                        cpp   full     334975ms  8:40.202
optimize-squid-visitors                        cpp   full     278051ms  7:44.835
optimize-squid-visitors                        cpp   full     276215ms  7:30.724

guwirth · 2021-03-29T11:38:49Z

@andreydanin thx

Still wondering which are two comparable lines in the list for
empty - "Sonar way", 0 enabled rules
full - 5425 cxx rules are enabled

What is the time difference between full and empty?

Branch                                         Mode  Profile  CXX_time  Total_time
optimize-squid-visitors                        cpp   empty    340701ms  9:19.440
optimize-squid-visitors                        cpp   empty    267705ms  7:25.019
optimize-squid-visitors                        cpp   empty    280359ms  7:37.511
optimize-squid-visitors                        cpp   empty    265813ms  7:29.725
optimize-squid-visitors                        cpp   empty    262919ms  7:20.962
optimize-squid-visitors                        cpp   full     270328ms  7:21.942
optimize-squid-visitors                        cpp   full     334975ms  8:40.202
optimize-squid-visitors                        cpp   full     278051ms  7:44.835
optimize-squid-visitors                        cpp   full     276215ms  7:30.724

andreydanin · 2021-03-29T11:51:44Z

I performed multiple tests in each configuration to eliminate interference with background tasks on my machine.
If we remove longest test in each group we can see that total execution time is close to 7 minutes 30 seconds for both configurations. IMHO it's hard to tell that one configuration was faster than the other.

guwirth · 2021-03-31T16:42:11Z

Hi @andreydanin, I merged the PR #2082 to 2.0. Don‘t know if faster but can not hurt...

andreydanin · 2021-10-04T14:53:29Z

Hi @guwirth

I've rebased my patch on top of 2.0.5 and performed some tests on our projects. For tests I enabled debug log (-X parameter for scanner).

Longes execution was found for "Sensor CXX". I checked output like this:

INFO: Sensor CXX [cxx]
...
INFO: Sensor CXX [cxx] (done) | time=1958588ms

Note: for cppcheck related part we expect log line:

INFO: Sensor CXX Cppcheck report import [cxx]

I checked 3 configurations:

1. Without "Sensor CXX"
No rules for squid. Only cppcheck related rule is enabled.

'INFO: Sensor CXX [cxx]' was not found in log
Execution time: ~ 1.5-2 min

2. With "Sensor CXX" (rule "Avoid too many code lines in source file" is enabled)

'INFO: Sensor CXX [cxx]' was found in log
Execution time: ~ 50 min

3. With "Sensor CXX" (rule "Avoid too many code lines in source file" is enabled, Preprocessor::handlePreprocessorDirective is disabled)
I checked process stack during execution (using jstack) and found that in most cases process was within Preprocessor::handlePreprocessorDirective.
I replaced this function with "return oneConsumedToken(token);" to check how long it will process files without it. But I don't know what functionality is affected. Probably it's a bad idea to disable this function.

'INFO: Sensor CXX [cxx]' was found in log
Execution time: ~ 9 min

Do you have any ideas what should I check next?

guwirth · 2021-10-05T06:17:02Z

Hi @andreydanin,

thx for testing.

Like to understand it better:

In case of 1) did you do any other changes on the source code?
Case 2) is same as 1) except you activated rule "Avoid too many code lines in source file" is enabled? Also no changes on the sources code?
With 3) you changed the source code. For my understanding this turns off the preprocessor. Are the activated rules here same as 1) or 2)?

Regards,

andreydanin · 2021-10-05T06:59:57Z

Hi. Thanks for the quick response.

In case of 1) did you do any other changes on the source code?

Yes. All three test cases are tested with the patch from this pull request applied. It should only have effect in test case 1 (when squid execution is skipped because it's rules are not active).

is same as 1) except you activated rule rule "Avoid too many code lines in source file" is enabled?

Yes.

Also no changes on the sources code?

Source code was changed. I can run all three test cases without my changes but I expect that test case 2 will be the same and case 1 will become same as 2 (squid will be active even rules are disabled).

With 3) you changed the source code. For my understanding this turns off the preprocessor.

Yes. I believe I disabled preprocessor in test case 3.

Are the activated rules here same as 1) or 2)?

Rules were same as in test case 2.

Test case 1 - only cppcheck rules are active. Squid ("Sensor CXX") was not executed as expected because patch from this pull request is applied.

Test cases 2 and 3 - rules from 1 plus rule "Avoid too many code lines in source file". Additional rule activated squid ("Sensor CXX") in test cases 2 and 3.

guwirth · 2021-10-06T07:52:08Z

Hi @andreydanin,

my main concern is still that the software metrics are not working with your patch? Did you compare the values, e.g. LoC, Complexity, … with the cases 1) to 3)…

Regards

andreydanin · 2021-10-06T09:22:30Z

Hi @guwirth

my main concern is still that the software metrics are not working with your patch? Did you compare the values, e.g. LoC, Complexity, … with the cases 1) to 3)…

LoCs, complexity and other metrics will not be present if squid is disabled. The problem for me and some other users is that this estimations performed for a long time on big projects. Also we are focused on code defects that were reported by external scanner(s) (cppcheck, PVS Studio, clang analyzer, etc.). Probably we can split basic measures (like LoCs) and more advanced measures (that require file analysis) into different components. In this case we will have squid that calculates LoCs very fast and new component that performs code analysis in depth to identify issues, code duplication and so on.

Code duplication, functions complexity and some other metrics are also important. But I think it's better to improve analysis speed for these measures in a separate patch. I didn't perform profiling and I'm still not very familiar with the codebase and sonar API to plan and design such improvements alone (at least yet :).

Thanks.

guwirth · 2021-10-07T08:23:35Z

Hi @andreydanin,

think there are still different ways:

Improve the performance of the squid sensor. Maybe you can help me by doing some measurements with a profiler.
Disable the squid sensor in the configuration. Here I would prefer sonar.cxx.squid.disabled (default =false) over your solution, this is more explizit.
Disable the preprozessor only. Here I’m not sure if completely or only the include-file handling part?

If you can provide a solution for 2) I would merge it.

If you could provide more detailed numbers with https://www.ej-technologies.com/products/jprofiler/overview.html it would help to take a better decision. If you ask them they will provide a free license for open source projects.

Regards

guwirth · 2021-10-07T08:42:24Z

@andreydanin sorry typo: …provide a solution for 2) I would merge it.

Fix for SonarOpenCommunity#2228 SquidSensor can take significant amount of time on big projects. Skip execution if sonar.cxx.squid.disabled parameter is set to true.

guwirth · 2021-10-08T07:03:38Z

Hello @andreydanin,

thx. I will merge it asap.

Regards

andreydanin force-pushed the squid-conditional branch from 0d37ed6 to 815e2cf Compare March 12, 2021 19:54

guwirth added a commit to guwirth/sonar-cxx that referenced this pull request Mar 14, 2021

optimize squid sensor

b6952ff

- add visitor only if corresponding rule is active - close SonarOpenCommunity#2080

guwirth mentioned this pull request Mar 14, 2021

optimize squid sensor #2082

Merged

guwirth closed this in #2082 Mar 31, 2021

guwirth reopened this Oct 5, 2021

andreydanin force-pushed the squid-conditional branch 2 times, most recently from 19ba6bb to b78dfa6 Compare October 5, 2021 20:48

andreydanin force-pushed the squid-conditional branch from b78dfa6 to 2421d1c Compare October 7, 2021 11:24

andreydanin changed the title ~~CxxSquidSensor: run sensor only when related rules are active~~ Introduce parameter to disable CxxSquidSensor Oct 7, 2021

Introduce parameter to disable CxxSquidSensor

38b726d

Fix for SonarOpenCommunity#2228 SquidSensor can take significant amount of time on big projects. Skip execution if sonar.cxx.squid.disabled parameter is set to true.

andreydanin force-pushed the squid-conditional branch from 2421d1c to 38b726d Compare October 7, 2021 11:27

guwirth self-requested a review October 11, 2021 18:12

guwirth assigned andreydanin Oct 11, 2021

guwirth added the enhancement label Oct 11, 2021

guwirth added this to the 2.0.6 milestone Oct 11, 2021

guwirth linked an issue Oct 11, 2021 that may be closed by this pull request

Ability to disable CxxSquidSensor #2228

Closed

guwirth approved these changes Oct 12, 2021

View reviewed changes

guwirth mentioned this pull request Oct 12, 2021

Ability to disable CxxSquidSensor #2228

Closed

guwirth merged commit 573c293 into SonarOpenCommunity:master Oct 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce parameter to disable CxxSquidSensor #2080

Introduce parameter to disable CxxSquidSensor #2080

andreydanin commented Mar 12, 2021 •

edited

Loading

guwirth commented Mar 12, 2021

andreydanin commented Mar 12, 2021 •

edited

Loading

guwirth commented Mar 13, 2021

andreydanin commented Mar 13, 2021 •

edited

Loading

guwirth commented Mar 13, 2021

andreydanin commented Mar 13, 2021

guwirth commented Mar 13, 2021

guwirth commented Mar 14, 2021

guwirth commented Mar 14, 2021 •

edited

Loading

andreydanin commented Mar 17, 2021 •

edited

Loading

guwirth commented Mar 26, 2021

andreydanin commented Mar 28, 2021 •

edited

Loading

guwirth commented Mar 29, 2021

andreydanin commented Mar 29, 2021

guwirth commented Mar 29, 2021

andreydanin commented Mar 29, 2021

guwirth commented Mar 31, 2021

andreydanin commented Oct 4, 2021

guwirth commented Oct 5, 2021 •

edited

Loading

andreydanin commented Oct 5, 2021

guwirth commented Oct 6, 2021

andreydanin commented Oct 6, 2021

guwirth commented Oct 7, 2021 •

edited

Loading

guwirth commented Oct 7, 2021

guwirth commented Oct 8, 2021

Introduce parameter to disable CxxSquidSensor #2080

Introduce parameter to disable CxxSquidSensor #2080

Conversation

andreydanin commented Mar 12, 2021 • edited Loading

guwirth commented Mar 12, 2021

andreydanin commented Mar 12, 2021 • edited Loading

guwirth commented Mar 13, 2021

andreydanin commented Mar 13, 2021 • edited Loading

guwirth commented Mar 13, 2021

andreydanin commented Mar 13, 2021

guwirth commented Mar 13, 2021

guwirth commented Mar 14, 2021

guwirth commented Mar 14, 2021 • edited Loading

andreydanin commented Mar 17, 2021 • edited Loading

guwirth commented Mar 26, 2021

andreydanin commented Mar 28, 2021 • edited Loading

guwirth commented Mar 29, 2021

andreydanin commented Mar 29, 2021

guwirth commented Mar 29, 2021

andreydanin commented Mar 29, 2021

guwirth commented Mar 31, 2021

andreydanin commented Oct 4, 2021

guwirth commented Oct 5, 2021 • edited Loading

andreydanin commented Oct 5, 2021

guwirth commented Oct 6, 2021

andreydanin commented Oct 6, 2021

guwirth commented Oct 7, 2021 • edited Loading

guwirth commented Oct 7, 2021

guwirth commented Oct 8, 2021

andreydanin commented Mar 12, 2021 •

edited

Loading

andreydanin commented Mar 12, 2021 •

edited

Loading

andreydanin commented Mar 13, 2021 •

edited

Loading

guwirth commented Mar 14, 2021 •

edited

Loading

andreydanin commented Mar 17, 2021 •

edited

Loading

andreydanin commented Mar 28, 2021 •

edited

Loading

guwirth commented Oct 5, 2021 •

edited

Loading

guwirth commented Oct 7, 2021 •

edited

Loading