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

Block splitter : minor reformatting #3376

Merged
merged 1 commit into from
Dec 19, 2022
Merged

Block splitter : minor reformatting #3376

merged 1 commit into from
Dec 19, 2022

Conversation

Cyan4973
Copy link
Contributor

@Cyan4973 Cyan4973 commented Dec 18, 2022

After reading #3204, I wanted to have a look at the block splitter to understand what's going on.
This exercise lead to a (rather large) number of minor changes, which initially were cosmetic only, but eventually added a few correctness updates. They don't change the behavior of the functionality, but hopefully improve maintenance.

List of changes:

  • Shortened many long lines
  • Conversion from offset to offbase was done manually, instead of employing the dedicated macros
  • const correctness : seqStore was passed as a mutable buffer in scenarios where it was read only. The pb ran multiple levels deep, so several functions where controlled and their prototype updated to reflect this property.
  • Several variables and pointers could be made const, improving the understanding of what happens to them. Some variables could be removed, others could be moved to a more restrictive scope
  • Several fixes for -Wconversion warning level (note: not activated by default on zstd code base)
  • Some assert() were added, to better document what's the expected state of the system.
  • Added a few code comments

Only minor stuff, but I figure it was still valuable enough for maintenance to be merged.
None of these changes impact the behavior of the program. The source code is stricter, but it still does the same thing.

A side effect of this exercise is that I have a better understanding of the block splitter now.

The estimation logic, which judges if a splitting decision is beneficial or not, looks fine and seems generally re-usable.

The splitting decision though is still pretty raw, blindly mechanical.
Hence, that's where improvements could be inserted.

That being said, adding complexity at this stage is also likely going to impact speed.
So it might become desirable to feature multiple "levels" of block splitting.

and minor reliability and maintenance changes
@terrelln
Copy link
Contributor

The splitting decision though is still pretty raw, blindly mechanical.
Hence, that's where improvements could be inserted.

Agreed, we definitely need better splitting logic, which can hopefully be (mostly) compression-algorithm agnostic.

@Cyan4973 Cyan4973 merged commit 9073fe0 into dev Dec 19, 2022
@Cyan4973 Cyan4973 deleted the split2 branch January 13, 2023 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants