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

Issue-118, Auto compressor selection fix #119

Merged
merged 5 commits into from
Sep 18, 2024
Merged

Issue-118, Auto compressor selection fix #119

merged 5 commits into from
Sep 18, 2024

Conversation

worryg0d
Copy link
Collaborator

@worryg0d worryg0d commented Sep 9, 2024

This PR provides a fix for a bug described in #118.

CompressorFrame::compress_best picks the best compressor by comparing its space usage, but it doesn't consider the max_error allowed. This PR fixes this problem by filtering the results by max_error before picking a compressor that compresses the best.

Closes #118

@worryg0d worryg0d self-assigned this Sep 9, 2024
@worryg0d worryg0d added the bug Something isn't working label Sep 9, 2024
@worryg0d
Copy link
Collaborator Author

worryg0d commented Sep 9, 2024

Test fails because CompressorFrame::compress_best filters all compressors out because some returned inf and NaN values as computed errors.

I'm not sure how to deal with this one. Should it return some default value? Or is panic not a problem here?

@worryg0d worryg0d requested a review from cjrolo September 9, 2024 08:59
@worryg0d
Copy link
Collaborator Author

worryg0d commented Sep 9, 2024

@cjrolo Hello!

Could you please look at this and #117 PRs?

@cjrolo
Copy link
Collaborator

cjrolo commented Sep 17, 2024

Should I do this first or #117 ?

@worryg0d
Copy link
Collaborator Author

Should I do this first or #117 ?

I think this one is more prioritised, because it actually should allow #117 to pass tests

@cjrolo
Copy link
Collaborator

cjrolo commented Sep 17, 2024

Ok, I think I've found the issue! Look into the code and if you agree with it, merge it!

@cjrolo cjrolo requested a review from rukai September 17, 2024 23:16
@rukai
Copy link
Contributor

rukai commented Sep 18, 2024

I pushed a commit that maintains the same logic as carlos' commit but reduces the allocations.

Previously we were cloning the results Vec<u8> stored in the iterator, one for each compression.
This results in a new allocation for each compression type (currently 2 but that could change in the future) and also a potentially large memcpy for each.

With this commit we just allocate once for the vec to hold each compression.

I also bumped to the latest rust version to make use of the new lint ignore reason field.

brro-compressor/src/frame/mod.rs Show resolved Hide resolved
brro-compressor/src/frame/mod.rs Show resolved Hide resolved
brro-compressor/src/frame/mod.rs Show resolved Hide resolved
@cjrolo cjrolo merged commit 5f9e562 into main Sep 18, 2024
2 checks passed
@cjrolo cjrolo deleted the issue-118 branch September 18, 2024 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Constant compressor is always choosed after auto compressor selection
3 participants