[NFC] Reduce SwiftParser stack usage in debug mode #3160
Conversation
beccadax
requested review from
ahoppen,
bnbarham,
hamishknight and
rintaro
as code owners
|
Ooo, thanks for looking into this @beccadax!
|
I’ve had trouble running SwiftParser’s tests—particularly the torture tests in the swiftlang/swift repo—locally in debug mode because some common parser functions would use many kilobytes of stack for each call, consuming the stack too quickly for the `maximumNestingLevel` to control. Break up a number of problematic functions to help with this, and also insert a missing stack overflow check in `parseClosureExpression()`.
|
I'll wait for Hamish's input, but a quick run on the swift-syntax/Sources directory was actually about 1% faster than main, so that's promising. |
|
@swift-ci please test |
|
@swift-ci please test Windows platform |
|
Running the stress tester job on swiftlang/swift#58827 to get performance data |
| flavor: ExprFlavor, | ||
| pattern: PatternContext | ||
| ) -> RawExprSyntax? { | ||
| EXPR_PREFIX: switch self.at(anyIn: ExpressionModifierKeyword.self) { |
There was a problem hiding this comment.
While here, could you get rid of this statement label? Seems like was already unnecessary, all the uses of it are directly within the switch (and now with your patch could actually be turned into return nil)
| } | ||
| let element = elementKind!.makeElement(trailingComma: keepGoing, arena: self.arena) | ||
| if element.isEmpty { | ||
| break COLLECTION_LOOP |
There was a problem hiding this comment.
Can we also get rid of this statement label now? (I really dislike statement labels 😄)
As requested by Hamish.
|
@swift-ci smoke test |
|
@swift-ci please test |
3 participants
I’ve had trouble running SwiftParser’s tests—particularly the torture tests in the swiftlang/swift repo—locally in debug mode because some common parser functions would use many kilobytes of stack for each call, consuming the stack too quickly for even a low
maximumNestingLevelto control. Break up a number of problematic functions to help with this, and also insert a missing stack overflow check inparseClosureExpression().Each of the functions I've broken up previously used 4-11K of stack and appeared in a backtrace of a test case that crashed because of a stack overflow, either repeatedly or very close to the top of the stack.
I haven't determined if these changes regress release-mode performance; if they do, we probably shouldn't take them.