Skip to content

several optimizations#163

Merged
danmar merged 7 commits intocppcheck-opensource:masterfrom
firewave:optimizations
Feb 13, 2022
Merged

several optimizations#163
danmar merged 7 commits intocppcheck-opensource:masterfrom
firewave:optimizations

Conversation

@firewave
Copy link
Copy Markdown
Collaborator

@firewave firewave commented Apr 3, 2019

No description provided.

@danmar
Copy link
Copy Markdown
Collaborator

danmar commented Apr 4, 2019

do you have some real life measurements?

@firewave
Copy link
Copy Markdown
Collaborator Author

firewave commented Apr 4, 2019

Running with -j1 to see the actual impact - the library I used to profile and some random ones.

openni-sensor-primesense

real    0m4.441s
user    0m3.859s
sys     0m0.516s
real    0m3.844s
user    0m3.297s
sys     0m0.531s

s390-tools

real    0m5.095s
user    0m4.594s
sys     0m0.500s
real    0m4.660s
user    0m3.938s
sys     0m0.703s

gabedit

real    0m41.950s
user    0m39.781s
sys     0m2.172s
real    0m38.831s
user    0m36.672s
sys     0m2.125s

Edit: I just realized I had two local optimization still enabled. Need to re-test without them...also compared to the actual daca@home runs I forgot to adjust the --library parameters so the time will differ to those runs as well.

@firewave
Copy link
Copy Markdown
Collaborator Author

firewave commented Apr 4, 2019

Okay, now with just the optimization ins this PR. It does not effect all scans.

openni-sensor-primesense 4.7 5.1
gabedit 44.9 44.6
s390-tools 5.5 5.5

Strangely gabedit gives about 2000 warnings less with the changes, so I need to look into this.

Comment thread simplecpp.cpp Outdated
while (istr.good()) {
currentToken += ch;
if (currentToken.size() >= 4U && endsWith(currentToken, "*/"))
static const std::string commentEnd("*/");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as far as I remember we try to make simplecpp threadsafe. So a static local variable should be avoided.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Would a global one be okay?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Comment thread simplecpp.cpp Outdated
void simplecpp::Location::adjust(const std::string &str)
{
if (str.find_first_of("\r\n") == std::string::npos) {
if (!strpbrk(str.c_str(), "\r\n")) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it strange that this would be an optimisation. Is strpbrk optimised better?

However I think it obfuscates the code. I do not intuitively know how strpbrk works.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

callgrind showed and there's this
https://www.codeproject.com/Articles/18473/find-first-of-A-performance-pitfall-among-the-STL#A30. I used the latest GCC so it seems this is still true although that article is 12 years old.

strpbrk is the same as find_first_of.

Copy link
Copy Markdown
Collaborator Author

@firewave firewave Jan 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running simplecpp on -q -Ilib lib/valueflow.cpp:
GCC 335,129,681 -> 325,111,787
Clang 323,433,151 -> 313,133,608

I filed a ticket about this https://trac.cppcheck.net/ticket/10777.

@firewave
Copy link
Copy Markdown
Collaborator Author

firewave commented May 1, 2019

I tried some other measurements and those were a inconclusive. I will look into this in the next days.

@firewave firewave marked this pull request as draft January 27, 2022 19:29
@firewave
Copy link
Copy Markdown
Collaborator Author

firewave commented Jan 27, 2022

Running simplecpp with -q -Ilib -D __GNUC__ lib/valueflow.cpp:
GCC 335,129,681 -> 318,581,229
Clang 323,433,151 -> 312,465,341

@firewave
Copy link
Copy Markdown
Collaborator Author

firewave commented Feb 12, 2022

Using lib/valueflow.cpp from cppcheck-opensource/cppcheck@0a99e3b with -q -Ilib -D __GNUC__ lib/valueflow.cpp.

baseline
GCC 11 337,505,121
Clang 13 325,815,035

Location::adjust()
GCC 11 327,385,765
Clang 13 315,411,381

endsWith()
GCC 11 320,802,714
Clang 13 314,737,794

lastLine()
GCC 11 320,639,877
Clang 13 314,696,243

unordered_map
GCC 11 306,006,031
Clang 13 299,186,851

flags memchr
GCC 11 302,776,749
Clang 13 296,127,177

flags copy ctor
GCC 11 297,134,127
Clang 13 291,144,265

FYI about a third of the instructions is made up by the new/delete of the Token objects and the std::istream reading.

@firewave firewave changed the title a few more optimizations several optimizations Feb 12, 2022
@firewave firewave marked this pull request as ready for review February 12, 2022 20:14
@danmar danmar merged commit 0c36e4b into cppcheck-opensource:master Feb 13, 2022
@firewave firewave deleted the optimizations branch February 13, 2022 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants