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

Improved performance of rz_bv_copy_nbits and rz_bv_set_range #4740

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

rajRishi22
Copy link

@rajRishi22 rajRishi22 commented Nov 25, 2024

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the rizin book with the relevant information (if needed)

Detailed description
This pull request optimizes the performance of the rz_bv_set_range function. The original implementation iterated through bits one at a time, leading to inefficiencies for large ranges. The updated implementation:

Processes aligned chunks of bits using system word size for faster operations.
Dynamically adjusts chunk size for different architectures (e.g., 32-bit, 64-bit).
Handles unaligned prefix and suffix bits separately while optimizing the main loop.
Adds robust boundary validation to ensure correctness.
This change reduces iteration overhead and improves performance while maintaining compatibility and correctness.

Test plan

  1. Verify functionality for small ranges, large ranges, and edge cases (unaligned ranges).
  2. Test on various architectures to confirm portability (32-bit, 64-bit).
  3. Use unit tests to ensure the results match the original functionality.
  4. Check for any regressions with existing test suites.

Closing issues
Partially addresses #4716
...

Copy link
Member

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

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

Nice start!
I can imagine we can even optimize the unaligned cases. But let's do this later.

I also changed your PR message. Because it closes #4716 only partially (missing unaligned cases).

You can add test cases in test/unit/test_bitvector.c.
Once everything is implemented and passes, we can run the Travis CI to test it on big endian machines.

librz/util/bitvector.c Outdated Show resolved Hide resolved
librz/util/bitvector.c Outdated Show resolved Hide resolved
librz/util/bitvector.c Outdated Show resolved Hide resolved
librz/util/bitvector.c Outdated Show resolved Hide resolved
librz/util/bitvector.c Outdated Show resolved Hide resolved
librz/util/bitvector.c Outdated Show resolved Hide resolved
librz/util/bitvector.c Outdated Show resolved Hide resolved
librz/util/bitvector.c Outdated Show resolved Hide resolved
librz/util/bitvector.c Outdated Show resolved Hide resolved
librz/util/bitvector.c Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the rz-test label Nov 25, 2024
librz/util/bitvector.c Outdated Show resolved Hide resolved
librz/util/bitvector.c Outdated Show resolved Hide resolved
librz/util/bitvector.c Outdated Show resolved Hide resolved
librz/util/bitvector.c Outdated Show resolved Hide resolved
Copy link
Member

@wargio wargio left a comment

Choose a reason for hiding this comment

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

missing implementation of rz_bv_get_chunk

Comment on lines +265 to +266
first_word &= ~(0xFFFFFFFF >> bit_offset); // Clear the upper bits
second_word &= (0xFFFFFFFF >> (32 - bit_offset)); // Clear the lower bits
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
first_word &= ~(0xFFFFFFFF >> bit_offset); // Clear the upper bits
second_word &= (0xFFFFFFFF >> (32 - bit_offset)); // Clear the lower bits
first_word &= ~(UT32_MAX >> bit_offset); // Clear the upper bits
second_word &= (UT32_MAX >> (32 - bit_offset)); // Clear the lower bits

Copy link
Member

@wargio wargio left a comment

Choose a reason for hiding this comment

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

You are touching something very critical within rizin, so please provide benchmark and C test for correctness and edge cases.

@Rot127
Copy link
Member

Rot127 commented Dec 12, 2024

Are you still interested in finishing this? If yes, please fix the build and add tests.
Otherwise, I would close it in the next days.

@rajRishi22
Copy link
Author

Sorry , i had my college exams so i was busy , i will start working on this issue again from today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants