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 #3692: Modified methods to use zstdmt binary #3703

Merged
merged 4 commits into from
Oct 16, 2023

Conversation

PromiseFru
Copy link
Collaborator

@PromiseFru PromiseFru commented Oct 9, 2023

Closes #3692

Hello @marco-c @suhaibmujahid,

I've made changes to the zstd_decompress and extract_tar_zst methods. They now use the zstdmt binary instead of the zstd binary for decompression tasks.

Please, I wish to find out if this change is needed just for decompression tasks, or should I update other methods like zstd_compress and create_tar_zst that use the zstd binary for compression.

Additionally, I noticed the presence of the zstandard Python library, alongside the use of the zstd binary for certain tasks. I'm curious about why both are used when they seem to perform similar tasks.

@suhaibmujahid
Copy link
Member

Thank you for the PR, @PromiseFru! Could you please test/execute the change code locally to confirm that it is working?

Closes #3692

Please add the linking keyword in the description

Please, I wish to find out if this change is needed just for decompression tasks, or should I update other methods like zstd_compress and create_tar_zst that use the zstd binary for compression.

The scope of the issue concerns the decompression tasks only.

Additionally, I noticed the presence of the zstandard Python library, alongside the use of the zstd binary for certain tasks. I'm curious about why both are used when they seem to perform similar tasks.

Originally zstandard was used everywhere but it was replaced to improve the performance.

@PromiseFru
Copy link
Collaborator Author

Thank you for the PR, @PromiseFru! Could you please test/execute the change code locally to confirm that it is working?

Closes #3692

Please add the linking keyword in the description

Please, I wish to find out if this change is needed just for decompression tasks, or should I update other methods like zstd_compress and create_tar_zst that use the zstd binary for compression.

The scope of the issue concerns the decompression tasks only.

Additionally, I noticed the presence of the zstandard Python library, alongside the use of the zstd binary for certain tasks. I'm curious about why both are used when they seem to perform similar tasks.

Originally zstandard was used everywhere but it was replaced to improve the performance.

Hello @suhaibmujahid

I ran the tests that focused on the methods I had updated, and all of them passed. I have attached their logs below:

pytest -vk "test_zstd_compress_decompress"

test_zstd_compress_decompress.txt

pytest -vk "test_extract_db_zst"

test_extract_db_zst.txt

@suhaibmujahid
Copy link
Member

Thank you, @PromiseFru! We have now extract_tar_zst() still not tested. Could you execute it locally or add a test for it?

BTW. If you add a test, it will run automatically on our CI.

@PromiseFru
Copy link
Collaborator Author

PromiseFru commented Oct 13, 2023

Thank you, @PromiseFru! We have now extract_tar_zst() still not tested. Could you execute it locally or add a test for it?

BTW. If you add a test, it will run automatically on our CI.

Hello @suhaibmujahid, I appreciate your feedback. I've included a test_create_extract_tar_zst for testing the create_tar_zst and extract_tar_zst methods since they didn't have tests. I would appreciate any additional feedback. 🙏🏾. Also, I ran the test locally, and it passed.

pytest -vk "test_create_extract_tar_zst"

test_create_extract_tar_zst.txt

Copy link
Member

@suhaibmujahid suhaibmujahid left a comment

Choose a reason for hiding this comment

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

Thank you, @PromiseFru! LGTM!

@marco-c
Copy link
Collaborator

marco-c commented Oct 14, 2023

Couldn't create_tar_zst also use zstdmt?

@suhaibmujahid
Copy link
Member

Couldn't create_tar_zst also use zstdmt?

I advised in #3703 (comment) to only change the decompressing functions to stay in the scope of issue #3692. I will create a separate issue for that.

Copy link
Collaborator

@marco-c marco-c left a comment

Choose a reason for hiding this comment

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

Thanks!

@marco-c marco-c merged commit 9633b89 into mozilla:master Oct 16, 2023
1 check passed
@PromiseFru PromiseFru deleted the issue-3692 branch October 16, 2023 18:16
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.

Use all threads when decompressing with zstd
3 participants