Refactor compressors/decompressors for laziness + safety #445
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes #396. As noted in the originally reported issue, enabling debug logging when writing arrow data with compression can result in segfaults because the underlying CodecX package have debug logs, causing task switches/migration and thus making the pattern of using a single `X_COMPRESSOR` array indexed by `Threads.threadid()` unsafe since multiple threads may try using the same compressor at the same time. We fix this by wrapping each compressor in a `Lockable` and ensuring the `compress` (or `uncompress`) operation holds the lock for the duration of the operation. We also: * Add a decompressor per thread to avoid recreating them over and over during reading * Lazily initialize compressors/decompressors in a way that is 1.9+ safe and only creates the object when needed by a specific thread * Switch from WorkerUtilities -> ConcurrentUtilities (the package was renamed) Co-authored-by: J S <49557684+svilupp@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## main #445 +/- ##
==========================================
+ Coverage 87.25% 87.31% +0.05%
==========================================
Files 26 26
Lines 3210 3256 +46
==========================================
+ Hits 2801 2843 +42
- Misses 409 413 +4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #396.
As noted in the originally reported issue, enabling debug logging when writing arrow data with compression can result in segfaults because the underlying CodecX package have debug logs, causing task switches/migration and thus making the pattern of using a single
X_COMPRESSORarray indexed byThreads.threadid()unsafe since multiple threads may try using the same compressor at the same time.We fix this by wrapping each compressor in a
Lockableand ensuring thecompress(oruncompress) operation holds the lock for the duration of the operation. We also:Successor to #397; I've added @svilupp as a co-author here since they started the original movement for the code to go in this direction.