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

Bug fix: get_used_symbols #467

Merged
merged 1 commit into from
Nov 20, 2024
Merged

Bug fix: get_used_symbols #467

merged 1 commit into from
Nov 20, 2024

Conversation

koniksedy
Copy link
Collaborator

This PR fixes bugs in the method get_used_symbols.

@koniksedy koniksedy requested review from jurajsic and Adda0 November 20, 2024 12:02
@jurajsic
Copy link
Member

This PR fixes bugs in the method get_used_symbols.

Which bugs?

Copy link
Collaborator

@Adda0 Adda0 left a comment

Choose a reason for hiding this comment

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

LGTM.

@Adda0
Copy link
Collaborator

Adda0 commented Nov 20, 2024

This PR fixes bugs in the method get_used_symbols.

Which bugs?

The optimized variants of the method by @kilohsakul had some bugs with using uninitialized memory, which caused segfaults. @koniksedy fixed these, and apparently some variants are now generally faster than our currently used implementation. We can experiment with the variants in, for example, benchmarks from Noodler and see whether there is any noticeable impact on the performance.

@Adda0 Adda0 merged commit 65cb9c6 into devel Nov 20, 2024
18 checks passed
@Adda0 Adda0 deleted the get_used_symbols-bug branch November 20, 2024 12:31
@koniksedy
Copy link
Collaborator Author

The function utils::reserve_on_insert that @kilohsakul used inside Delta::get_used_symbols_bv and Delta::get_used_symbols_chv is tricky.

template<class Vector>
void inline reserve_on_insert(Vector & vec,size_t needed_capacity = 0,size_t extension = 32) {
//return; //Try this to see the effect of calling this. It should not affect functionality.
if (vec.capacity() < extension) //if the size is already large enough, leave it to the default doubling strategy. This if seems to make a barely noticeable difference :).
{
if (vec.capacity() < std::max(vec.size() + 1, needed_capacity))
vec.reserve(vec.size() + extension);
}
}

It should only be used before insert operations, not direct access. The function extends the vector size only if it is less than extension = 32. Since this function was the only one used in Delta::get_used_symbols_chv to resize the output vector, it never actually resized the vector, leading to a segmentation fault.

Perhaps the function utils::reserve_on_insert should be renamed to something like first_reserve_before_insert.

@Adda0
Copy link
Collaborator

Adda0 commented Nov 20, 2024

More like reserve_before_first_insert, or reserve_on_first_insert no?

@koniksedy
Copy link
Collaborator Author

reserve_on_first_insert sounds the best.

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.

3 participants