Add recursive spill for RowNumber #8654
Conversation
✅ Deploy Preview for meta-velox canceled.
|
490c6c6 to
e41d72b
Compare
e41d72b to
6833fc1
Compare
c0d7b31 to
2bcd1bd
Compare
velox/exec/tests/RowNumberTest.cpp
Outdated
| newQueryCtx(memoryManager, executor_, kMemoryCapacity * 2); | ||
|
|
||
| struct { | ||
| int32_t numSpill; |
|
@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
1551087 to
2d3e376
Compare
|
@xiaoxmeng Hi Xuan, I've made a small refactor based on your newly simplified |
8b94e78 to
49bb0d1
Compare
49bb0d1 to
0bb4614
Compare
59fda5e to
91e6cb4
Compare
91e6cb4 to
6159d6c
Compare
|
@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
velox/exec/RowNumber.cpp
Outdated
|
|
||
| void RowNumber::addInputInternal(const RowVectorPtr& input, bool fromSpill) { | ||
| ensureInputFits(input); | ||
| if (!fromSpill && inputSpiller_ != nullptr) { |
There was a problem hiding this comment.
do we expect to encounter this condition in any case? if not, should this be a VELOX_CHECK instead of a no-op return ?
There was a problem hiding this comment.
Yes, this happens after the first spill before the noMoreInput stage. This function is unreadable and hard to understand. I will refactor it and use addSpillInput. Thanks for your advice.
velox/exec/RowNumber.cpp
Outdated
| return; | ||
| } | ||
|
|
||
| if (input == nullptr) { |
There was a problem hiding this comment.
ditto as comment on L485
velox/exec/RowNumber.cpp
Outdated
| } | ||
|
|
||
| if (yield_) { | ||
| VELOX_CHECK_NULL(input_); |
There was a problem hiding this comment.
nit: do we need this check? since we only reach here if condition on L227 is true where input_ == nullptr
6159d6c to
9106413
Compare
There was a problem hiding this comment.
@bikramSingh91 Thanks for your review, I've resolved all the comments and updated the doc. Could you help take another look? Thanks a lot.
velox/exec/RowNumber.cpp
Outdated
| } | ||
|
|
||
| if (yield_) { | ||
| VELOX_CHECK_NULL(input_); |
velox/exec/RowNumber.cpp
Outdated
|
|
||
| void RowNumber::addInputInternal(const RowVectorPtr& input, bool fromSpill) { | ||
| ensureInputFits(input); | ||
| if (!fromSpill && inputSpiller_ != nullptr) { |
There was a problem hiding this comment.
Yes, this happens after the first spill before the noMoreInput stage. This function is unreadable and hard to understand. I will refactor it and use addSpillInput. Thanks for your advice.
3330f39 to
8178c03
Compare
| @@ -97,8 +101,14 @@ void RowNumber::addInput(RowVectorPtr input) { | |||
| } | |||
|
|
|||
| void RowNumber::addSpillInput() { | |||
| ensureInputFits(input_); | |||
There was a problem hiding this comment.
this will be called twice when invoked via addInput(), will this result in the reservation being increased twice?
There was a problem hiding this comment.
No, the addInput is called before noMoreInput, while addInputSpill is called after noMoreInput.
The addInputSpill is specifically called right after the spillInputReader_ reads a batch in either the restoreNextSpillPartition or getOutput. The spillInputReader_ is null before noMoreInput is called, and it first becomes non-null at the end of restoreNextSpillPartition when the noMoreInput is called.
9bf9bd8 to
b814c65
Compare
|
@bikramSingh91 Hi Bikram, any further comments? Thanks. |
|
@mbasmanova Hi Masha, all the prerequisite PRs have been merged, could you please help take another look? Thanks. |
|
@duanmeng Thank you for adding "The data layout may look like as follows." section to PR description. It is very helpful. Spilling is rather complicated and we are seeing many bugs in it. I wonder how much testing have been done for this change and what's the confidence of it being bug free. I don't believe we have fuzzer coverage for RowNumber operator. Would that be necessary to ensure there are no (or very few) bugs? Also, I wonder what's the motivation for this change. Is there a real-world scenario that benefits from this change and justifies added complexity and maintenance costs. Would you describe that scenario? In this scenario, what's the impact on latency and CPU usage of spilling? How much CPU and wall time a query would use without spilling and how does that compare to spilling? |
220b463 to
4e09fff
Compare
|
@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@xiaoxmeng merged this pull request in 4963d71. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
At present, RowNumber only supports single-level spill partitions.
Once a spill takes place, it won't be spilled again. This means that
even if a restored spill partition uses an excessive amount of memory,
it still cannot be reclaimed. Therefore, it would be beneficial to implement
recursive spill support, as outlined in
https://facebookincubator.github.io/velox/develop/spilling.html.
1 Advance 'spillPartitionBits_' based on the partition bit of the currently
restoring spill partition to prepare for potential subsequent spills (recursive).
2 During the recursive spill input process, read the spilled input data from the
previous spill run via 'spillInputReader_', then spill it back into several sub-partitions.
Then restore one of the newly spilled partitions and reset 'spillInputReader' accordingly.
3 It's crucial to separate the recursive input spill process from the spill process itself,
as it can be time-consuming and should yield if it exceeds the CPU time limit.
In summary, the first spill operation writes all data to the disk. Subsequently,
RowNumber reads the data from the disk one partition at a time and clears
the data from the restored partition once it has been processed (#8413).
In the case of a recursive spill, only the data from the currently restored partition
in memory is spilled to the disk (next level), and only one partition from the next
level is loaded back into memory.
The data layout may look like as follows.