-
Notifications
You must be signed in to change notification settings - Fork 564
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
solve stack overflow
on RecursionLimitExceeded
during debug building
#1171
solve stack overflow
on RecursionLimitExceeded
during debug building
#1171
Conversation
In short, on local machine |
stack overflow
on RecursionLimitExceeded
during debug buildingSolve stack overflow in recursion testsstack overflow
on RecursionLimitExceeded
during debug building
Pull Request Test Coverage Report for Build 8588605869Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much @Nikita-str -- think this change makes sense and your code is very nicely written. I am very much sorry for the delay in reviewing.
It think the new parse_boxed_...
methods should not be pub, at least until someone explicitly needs them, to keep the API surface area of this crate smaller.
I also think the inner module definitions were a little strange -- so I pushed some changes to to follow your other pattern of returning Box
ed bodies.
I also validated that with the changes in this PR the tests from #1166 also pass 🚀 |
…ing (apache#1171) Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> (cherry picked from commit 83c5d81)
…ing (apache#1171) Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
The main idea is move <usage of creation large structures on stack after which they moved into
Box
> into <separate fns>. So for each execution branch would be reserved less stack memory.Current changes focused on case of parsing seq of repeated
Token::LParen
((...(
) because the problem appeared onparse_deeply_nested_parens_hits_recursion_limits
test(the test
).So in first commit instead of
used
In second commit used
Result::map
as wrapper fn to avoid placing object on stack.In third commit move out some cases of parsing in separate fns to split used stack by execution flow. It's done slightly ugly, maybe I shouldn't move it into local mod.
The next words is compiler/OS related
It seems like at least my compiler in debug build in next case reserve separate space on stack for
Result<Ok, Err>
and forOk
because after first commit
the test
worked withDEFAULT_REMAINING_DEPTH = 60;
and before this commit it panics withDEFAULT_REMAINING_DEPTH = 50;
and without this assumption such things shouldn't happen(inthe test
trace changed fromcall:parse_query_body
tocall:parse_boxed_query_body THEN call:parse_query_body
).sizeof(SetExpr) = 0x3C0
and stack allocation inparse_query
decreased by0xB00
(almost 3 times).Locally after all 3 commits the stack memory reservation changed from
0x6020
to0x1C80
forparse_query
& from0x1F30
to0x1780
forparse_query_body
(but in(...(
-case there is additional stack with size ~sizeof(SetExpr)
&sizeof(Query)
). The real decrease in stack memory reserving to one recursion step in(...(
-case is~2.1
times on my local machine.