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

Huffman refactor #3434

Merged
merged 2 commits into from
Jan 20, 2023
Merged

Huffman refactor #3434

merged 2 commits into from
Jan 20, 2023

Conversation

terrelln
Copy link
Contributor

@terrelln terrelln commented Jan 18, 2023

The first commit deletes all unused Huffman functions.

The second commit replaces boolean parameters (bmi2, suspectUncompressible, preferRepeat, optimalDepth) with a flags bit field. This makes it easier to add flags, and makes the parameter list shorter. I've also added another flag disableAsm which disables the assembly decompressor, which is useful for fuzzing. I've renamed the functions to remove the _bmi2() suffix, since the first commit deleted all the non-bmi2 variants.

In a stacked PR, I will be adding a fast C variant of the decoders, and will add another flag disableFast, which will allow us to opt out of the fast decoder for fuzzing. This is the motivation for switching to the flags.

@terrelln terrelln changed the title Delete unused Huffman functions Huffman refactor Jan 19, 2023
@Cyan4973
Copy link
Contributor

The first part is a welcome reduction in code size, so it's basically good to go.

For the second part, I wonder sometimes if the new multi-purposes flags field makes it more difficult for the compiler to enable constant propagation, which was actually a goal in some cases, in order to generate specialized function.

I think bmi2 specialization is the most important one that comes to mind, though there might be others.

If a constant propagation case is missing, this should be visible in benchmarks, assuming we can produce the relevant benchmark test which triggers the target code path.

@terrelln
Copy link
Contributor Author

If a constant propagation case is missing, this should be visible in benchmarks, assuming we can produce the relevant benchmark test which triggers the target code path.

None of these flags are used in constant propagation, or in hot code. They are all passed in through the public interface, so we don't know the value at compile time.

Specifically the BMI2 flag just picks which version of the function we call.

I benchmarked the code before and after with these 2 commands, and don't see any changes:

zstd -b1e1 ~/datasets/silesia.tar
zstd -b1e1 ~/datasets/silesia.tar --zstd=tlen=131072 --compress-literals

* If unset: Allow using assembly implementations
*/
HUF_flags_disableAsm = (1 << 4),
} HUF_flags_e;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The flags that we are replacing are all listed here.

The bmi2/disableAsm flags select which decoding loop to call, but aren't actually passed down to the hot decoding loop.

The others are all used during table building, but aren't used during inlining or constant prop.

@terrelln terrelln merged commit 3291691 into facebook:dev Jan 20, 2023
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