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

Using reserve for optimize inserts and minor fixes #723

Merged
merged 1 commit into from
Aug 26, 2023

Conversation

GermanAizek
Copy link
Contributor

Summary

Useful changes, additional insert were found at emplace_back, but memory didnt allocated for last insert after loop.

@@ -169,7 +169,7 @@ class ArcAPI {
static const ArcCodecs& codecs() {
return get()->arc_codecs;
}
static size_t Count7zCodecs() {
static const size_t Count7zCodecs() {
Copy link
Contributor

Choose a reason for hiding this comment

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

const is useless on return types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix it here d832463

@@ -702,8 +702,9 @@ class Plugin
});
const auto error_log = std::make_shared<ErrorLog>();
size_t im = 0, nm = matched_indices.size();
std::vector<UInt32> indices;
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes the semantics: previously on each iteration the vector was recreated, now it's outside the loop and will accumulate all the values.
If you want to optimize here, add indices.clear() to the loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

u ar absolutely right, I have changed original behavior function.
fix it here d832463

@GermanAizek
Copy link
Contributor Author

@alabuzhev,
I fixed code tabulation by making it like in your other project sources.

@@ -233,6 +233,7 @@ class MyCompressCodecsInfo : public ICompressCodecsInfo, public IHashers, privat
}
if (arc_lib.ComHashers) {
UInt32 numHashers = arc_lib.ComHashers->GetNumHashers();
hashers_.reserve(numHashers);
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about it, all this code is a part of a bigger loop, and calling reserve in a loop is a known antipattern: it is too easy to pessimise exponential growth down to linear.

I suggest leaving it as is, it's not like like this place is a bottleneck anyway.

@@ -400,6 +401,7 @@ void ArcAPI::load_codecs(const std::wstring& path) {
const auto& add_hashers = [this](ArcLib &arc_lib, size_t lib_index) {
if (arc_lib.ComHashers) {
UInt32 numHashers = arc_lib.ComHashers->GetNumHashers();
arc_hashers.reserve(numHashers);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above: add_hashers is calling from a for loop below, and then again from a while loop.
reserveing here would effectively kill exponential growth,

while (im < nm) {
std::vector<UInt32> indices;
if (!indices.empty())
Copy link
Contributor

Choose a reason for hiding this comment

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

This condition is redundant, clear checks it internally.

@alabuzhev alabuzhev merged commit 579fcd9 into FarGroup:master Aug 26, 2023
18 of 23 checks passed
@sonarcloud
Copy link

sonarcloud bot commented Aug 26, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

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