chore: get rid of possible recursion when unwinding structured reply #5012
Conversation
| auto& last = stack_.top(); | ||
| last.first->arr.push_back(std::move(val)); | ||
| if (collapse_if_needed && last.second-- == 1) { | ||
| CollapseFilledCollections(); |
There was a problem hiding this comment.
Logically, we still have conditional recursion.
It appears more dangerous because the unconditional collapsing collections must process all elements.
Now it should skip part of them. Correct?
There was a problem hiding this comment.
I am not sure I understand this. Imagine a stack of size 2. We enter the body here and call CollapseFilledCollection() which pops the top of stack_ and calls Capture(). Now Capture() calls again CollapseFilledCollection which pops the top of the stack_ and calls Capture() again. The sequence we get is Capture -> Collapse -> Capture -> Collapse. This recursion can blow out the stack of the fiber. By passing false to Capture() within CollapseFilledCollections we enforce that we drain the stack_ within the body of CollapseFilledCollections(). So now we would get something like: Capture -> CollapseFilledCollections iteration 1 -> iteration 2 -> done.
| } | ||
|
|
||
| void CapturingReplyBuilder::CollapseFilledCollections() { | ||
| while (!stack_.empty() && stack_.top().second == 0) { | ||
| auto pl = std::move(stack_.top()); | ||
| stack_.pop(); | ||
| Capture(std::move(pl.first)); | ||
| Capture(std::move(pl.first), false); |
There was a problem hiding this comment.
Does it mean that we always have a 1-level nested collection?
There was a problem hiding this comment.
No, it can have several levels. My changes should not change functionality. Are you saying they do?
There was a problem hiding this comment.
I guess it changes the behavior, because Capture(std::move(pl.first), false); call never causes call CollapseFilledCollections(); inside Capture(). But CollapseFilledCollections(); was called unconditionally before.
There was a problem hiding this comment.
@vyavdoshenko CollapseFilledCollections body is executed only when !stack_empty(). The function applies only for the branch if (!stack_.empty()) { of Capture() above.
|
By behavior change I meant if it is possible to reach a different state at
the end. My claim is that after any call to capture, we will end up in the
same state for both implementations. The implementation details are
irrelevant
Roman Gershman
CTO
---
*www.dragonflydb.io <http://www.dragonflydb.io>*
…On Sun, Apr 27, 2025, 10:21 PM Volodymyr Yavdoshenko < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/facade/reply_capture.cc
<#5012 (comment)>
:
> }
void CapturingReplyBuilder::CollapseFilledCollections() {
while (!stack_.empty() && stack_.top().second == 0) {
auto pl = std::move(stack_.top());
stack_.pop();
- Capture(std::move(pl.first));
+ Capture(std::move(pl.first), false);
I guess it changes the behavior, because Capture(std::move(pl.first),
false); call never causes call CollapseFilledCollections(); inside
Capture(). But CollapseFilledCollections(); was called unconditionally
before.
—
Reply to this email directly, view it on GitHub
<#5012 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4BFCGK4IRB5JDT7GTRXUT23UU3PAVCNFSM6AAAAAB36ZGZRSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDOOJXG4ZDCMJWGA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
| auto& last = stack_.top(); | ||
| last.first->arr.push_back(std::move(val)); | ||
| if (collapse_if_needed && last.second-- == 1) { | ||
| CollapseFilledCollections(); |
There was a problem hiding this comment.
I am not sure I understand this. Imagine a stack of size 2. We enter the body here and call CollapseFilledCollection() which pops the top of stack_ and calls Capture(). Now Capture() calls again CollapseFilledCollection which pops the top of the stack_ and calls Capture() again. The sequence we get is Capture -> Collapse -> Capture -> Collapse. This recursion can blow out the stack of the fiber. By passing false to Capture() within CollapseFilledCollections we enforce that we drain the stack_ within the body of CollapseFilledCollections(). So now we would get something like: Capture -> CollapseFilledCollections iteration 1 -> iteration 2 -> done.
| } | ||
|
|
||
| void CapturingReplyBuilder::CollapseFilledCollections() { | ||
| while (!stack_.empty() && stack_.top().second == 0) { | ||
| auto pl = std::move(stack_.top()); | ||
| stack_.pop(); | ||
| Capture(std::move(pl.first)); | ||
| Capture(std::move(pl.first), false); |
There was a problem hiding this comment.
@vyavdoshenko CollapseFilledCollections body is executed only when !stack_empty(). The function applies only for the branch if (!stack_.empty()) { of Capture() above.
| stack_.top().second--; | ||
| auto& last = stack_.top(); | ||
| last.first->arr.push_back(std::move(val)); | ||
| if (collapse_if_needed && last.second-- == 1) { |
No description provided.