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

Make is_decompression_disabled and is_compression_disabled thread-safe #13240

Merged
merged 1 commit into from
Apr 28, 2023

Conversation

vuule
Copy link
Contributor

@vuule vuule commented Apr 27, 2023

Description

Adds a mutex to guard the use of the memoization map for the status of nvCOMP features.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@vuule vuule self-assigned this Apr 27, 2023
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Apr 27, 2023
@vuule vuule added bug Something isn't working cuIO cuIO issue non-breaking Non-breaking change and removed libcudf Affects libcudf (C++/CUDA) code. labels Apr 27, 2023
@vuule vuule changed the title Make is_decompression_disabled and is_compression_disabled thread safe Make is_decompression_disabled and is_compression_disabled thread-safe Apr 27, 2023
@ttnghia
Copy link
Contributor

ttnghia commented Apr 27, 2023

I wonder if we can just call is_(de)compression_disabled_impl directly without the caching mechanism, as these functions are very cheap.

@vuule
Copy link
Contributor Author

vuule commented Apr 27, 2023

I wonder if we can just call is_(de)compression_disabled_impl directly without the caching mechanism, as these functions are very cheap.

Yes, we could. I only added this mechanism to be able to log the result of each query once. Otherwise we would either need a similar map for logging, or we would have to allow logs to be very repetitive.

@abellina
Copy link
Contributor

abellina commented Apr 28, 2023

@vuule I ran with this and it did not reproduce. Thanks for the quick fix.

@abellina abellina linked an issue Apr 28, 2023 that may be closed by this pull request
@vuule vuule marked this pull request as ready for review April 28, 2023 06:34
@vuule vuule requested a review from a team as a code owner April 28, 2023 06:34
@vuule vuule requested review from karthikeyann and ttnghia April 28, 2023 06:34
@abellina
Copy link
Contributor

abellina commented Apr 28, 2023

We are hitting all kinds of segfaults in CI that I think are all related to this issue: NVIDIA/spark-rapids#8194, so the spark-rapids CI is currently blocked or limping along. Could we merge @vuule's solution and then perhaps follow up with more modifications if the memoization approach should be modified?

@ttnghia
Copy link
Contributor

ttnghia commented Apr 28, 2023

/merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cuIO cuIO issue non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] sigsev while writing orc data
3 participants