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.

Sign up for GitHub

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

Jump to bottom

Add warning when multi-thread decompression is requested #3208

Merged
merged 3 commits into from
Jul 29, 2022
Merged

Add warning when multi-thread decompression is requested #3208

merged 3 commits into from
Jul 29, 2022

Conversation

tomcwang
Copy link
Contributor

address #2918

When user pass in argument for both decompression and multi-thread, print a warning message
to indicate that multi-threaded decompression is not supported.

@tomcwang
Add warning when multi-thread decompression is requested
85cac04 
When user pass in argument for both decompression and multi-thread, print a warning message
to indicate that multi-threaded decompression is not supported.
@yoniko
Copy link
Contributor

yoniko commented Jul 25, 2022

Thank you @tomcwang .
I see the check is inserted after -T0 is handled.
That means that a warning will be printed on decompression with -T0 on a multicore system.
Can you move the check to before the -T0 case is handled and nbWorkers are assigned?

@tomcwang
Copy link
Contributor Author

Thank you @tomcwang . I see the check is inserted after -T0 is handled. That means that a warning will be printed on decompression with -T0 on a multicore system. Can you move the check to before the -T0 case is handled and nbWorkers are assigned?

makes sense, I moved the check to before nbWorker is assigned, warning is not thrown for -T0 anymore.

@tomcwang
Move check to before nbWorker is assigned
a2f7bcb 
move check to before nbWorker is assigned so we do not throw warning message when
-T0 is used.
@tomcwang tomcwang force-pushed the tomcwang/add_warning_for_multithread_decompression branch from e10567e to a2f7bcb Compare July 25, 2022 17:50
@yoniko
Copy link
Contributor

yoniko commented Jul 25, 2022

Thank you.
One last thing - can you please add tests to cli-tests/compression/multi-threaded.sh?

@tomcwang
add test case for multi-threaded decoding warning
720c216 
add test case for multi-threaded decoding warning.
Expecation is for -d -T0 we will not throw any warning,
and see warning for any other -d -T(>1) inputs
yoniko
yoniko approved these changes Jul 29, 2022
View reviewed changes
Copy link
Contributor

@yoniko yoniko left a comment

Choose a reason for hiding this comment

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

Looks good!

@yoniko yoniko merged commit d4a5bc4 into facebook:dev Jul 29, 2022
@yoniko yoniko mentioned this pull request Jul 29, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yoniko yoniko yoniko approved these changes

Assignees

@yoniko yoniko

Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tomcwang @yoniko @facebook-github-bot