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.

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

Fix a number of performance issues (reported by Coverity Scan) #3967

Merged
merged 2 commits into from
Nov 21, 2022

Conversation

stweil
Copy link
Contributor

@stweil stweil commented Nov 19, 2022

The latest scan reported a number of performance issues because
several for loops used expensive copy operations.

After adding the const attribute to compare operators, const
could also be used in most for loops.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
@egorpugin
Copy link
Contributor

The fix is not very right.
for (auto && should be used instead.

@stweil
Copy link
Contributor Author

stweil commented Nov 20, 2022

I did not know auto && up to now, and it is currently used only once in Tesseract code. Why would it be better than const auto &?

For two files I replaced const auto & and auto & by auto &&. That did not change the code generated by clang, so the suggested change has no effect on the generated binaries

@egorpugin
Copy link
Contributor

auto && means auto when we have a simple value.
auto && means auto & when we have a reference.
auto && means const auto & when we have a const reference.
auto && means auto && when we have && reference.

In other words auto && selects right thing for you.
If you set auto & and then for some reason you change iteration range to const, you need manually fix all places. With auto && you are already set.

@stweil
Copy link
Contributor Author

stweil commented Nov 20, 2022

In other words auto && selects right thing for you.

That's indeed a nice feature, but an explicit const attribute has also advantages because it helps human readers and detects missing const attributes like the ones in the compare operators. Therefore I'd prefer to use const where it is possible.

So how should we fix the reported performance issues? Use auto && instead of auto & and keep the const auto & cases? Or use auto && and const auto &&?

@egorpugin
Copy link
Contributor

egorpugin commented Nov 20, 2022

but an explicit const attribute has also advantages because it helps human readers and detects missing const attributes like the ones in the compare operators

When you will be updating code const->non const you'll have to fix all places again.

So how should we fix the reported performance issues?

auto &&

Use auto && instead of auto &

Almost always auto &&. There are some rare cases when you have pointer and want explicitly use auto or auto *.

and keep the const auto & cases?

While refreshing codebase all such cases must be changed to auto &&.

const auto &&?

Did not see const auto && usages anywhere. Not sure exactly what is it and how it works. Do not use it.
It can mean const auto &.
But also it may mean copy as in raw auto which is bad.


So, we should use auto && everywhere (almost).

@stweil
Copy link
Contributor Author

stweil commented Nov 20, 2022

I updated the PR now to use auto && (although I still think that const auto & has advantages, too).

Coverity Scan reports "Unnecessary object copies can affect performance"
and suggests using the auto keyword with an &.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
@zdenop zdenop merged commit 5f297dc into tesseract-ocr:main Nov 21, 2022
@zdenop
Copy link
Contributor

zdenop commented Nov 21, 2022

thanks

@stweil stweil deleted the coverity branch November 25, 2022 09:14
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.

4 participants