Improve cancellation checks for CompressedRelationReader
#1637
Conversation
Conformance check passed ✅No test result changes. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1637 +/- ##
=======================================
Coverage 89.24% 89.24%
=======================================
Files 374 374
Lines 35683 35685 +2
Branches 4027 4027
=======================================
+ Hits 31845 31848 +3
+ Misses 2538 2537 -1
Partials 1300 1300 ☔ View full report in Codecov by Sentry. |
|
| auto decompressedBlockAndMetadata = decompressAndPostprocessBlock( | ||
| compressedBlock, blockMetadata.numRows_, scanConfig, blockMetadata); | ||
| cancellationHandle->throwIfCancelled(); |
There was a problem hiding this comment.
These changes probably don't hurt, but I don't understand what they are supposed to fix.
In theory, these lead to poor performance, as the cancellationHandle is concurrently accessed by many threads, and so we have added 2 L2-Cache-misses, but probably even those are not fixable.
The issues we saw, were reported here, but the missed cancellation check was somewhere else (your meachnism doesn't report, where the cancellation checks were missing, but the first check after the missed deadline. With materialized inputs that is often in the same loop, but with lazy inputs, it might be completely somewhere else, in a different generator.
So I don't know, what the benefit of these checks is (reading one block should definitely be orders of magnituds faster than the timeout interval, so it suffices if the consumer checks the timeout.
|
I still don't think that there was something wrong here. Timeout checks that appear to be in |
Improves the placement of cancellation checks slightly to avoid spamming the console with warnings.