Skip to content
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

Merged

Conversation

Nikita-str
Copy link
Contributor

@Nikita-str Nikita-str commented Mar 12, 2024

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 on parse_deeply_nested_parens_hits_recursion_limits test(the test).

So in first commit instead of

let large_struct = self.parse_large_struct()?;
let boxed_large_struct = Box::new(large_struct);
// without compiler optimization on stack reserves
// memory for `LargeStruct`

used

fn parse_boxed_large_struct(&mut self) -> Result<Box<LargeStruct>, ..> {
   self.parse_large_struct().map(Box::new)
}
..
let boxed_large_struct = self.parse_boxed_large_struct()?;
// on stack reserves only memory for pointer

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 for Ok

let x = return_result()?; // seems like here reserved space for `Result<Ok, Err>` and for `Ok`

because after first commit the test worked with DEFAULT_REMAINING_DEPTH = 60; and before this commit it panics with DEFAULT_REMAINING_DEPTH = 50; and without this assumption such things shouldn't happen(in the test trace changed from call:parse_query_body to call:parse_boxed_query_body THEN call:parse_query_body). sizeof(SetExpr) = 0x3C0 and stack allocation in parse_query decreased by 0xB00 (almost 3 times).

Locally after all 3 commits the stack memory reservation changed from 0x6020 to 0x1C80 for parse_query & from 0x1F30 to 0x1780 for parse_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.

@Nikita-str
Copy link
Contributor Author

In short, on local machine stack_overflow in parse_deeply_nested_parens_hits_recursion_limits test fixed. It panics in debug build only after const DEFAULT_REMAINING_DEPTH: usize = 125;.

@Nikita-str Nikita-str changed the title Trying to solve stack overflow on RecursionLimitExceeded during debug buildingSolve stack overflow in recursion tests solve stack overflow on RecursionLimitExceeded during debug building Mar 13, 2024
@coveralls
Copy link

coveralls commented Apr 7, 2024

Pull Request Test Coverage Report for Build 8588605869

Warning: 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

  • 35 of 35 (100.0%) changed or added relevant lines in 1 file are covered.
  • 156 unchanged lines in 6 files lost coverage.
  • Overall coverage decreased (-0.03%) to 87.906%

Files with Coverage Reduction New Missed Lines %
src/dialect/duckdb.rs 1 70.0%
src/test_utils.rs 2 89.66%
src/dialect/mod.rs 8 82.11%
tests/sqlparser_postgres.rs 34 97.94%
tests/sqlparser_common.rs 53 97.12%
src/parser/mod.rs 58 82.84%
Totals Coverage Status
Change from base Build 8582586495: -0.03%
Covered Lines: 20629
Relevant Lines: 23467

💛 - Coveralls

Copy link
Collaborator

@alamb alamb left a 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 Boxed bodies.

@alamb
Copy link
Collaborator

alamb commented Apr 7, 2024

I also validated that with the changes in this PR the tests from #1166 also pass 🚀

@alamb alamb merged commit 83c5d81 into sqlparser-rs:main Apr 7, 2024
10 checks passed
lustefaniak pushed a commit to getsynq/sqlparser-rs that referenced this pull request Apr 10, 2024
…ing (sqlparser-rs#1171)

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

(cherry picked from commit 83c5d81)
JichaoS pushed a commit to luabase/sqlparser-rs that referenced this pull request May 7, 2024
…ing (sqlparser-rs#1171)

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants