Repository navigation
Skip rescanning operands whose scan already failed - #2090
Open
LucaCappelletti94 wants to merge 1 commit into
Open
LucaCappelletti94 wants to merge 1 commit into
LucaCappelletti94 wants to merge 1 commit into
Conversation
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While fuzzing a crate that uses syn, I landed on this crash case (same as #1864). I usually just steer the fuzzer away from these and that is it, since often such cases are just adversarial and not code that is in any way reasonable, plus often handling these cases reduces the performance of actual proper code parsing, this time on a whim I tried to have Opus 5.5 look into it and it happened to find a simple solution that gives better performance overall. I reviewed well the solution and it looks reasonable to me.
Put briefly, printing an expression whose rightmost operands nest ranges, prefix operators or binary operators takes time exponential in the nesting depth, with each nested level roughly doubling.
In
scan_right, the binary, prefix, range andletarms scan their operand, and when that scan does not returnConsumethey callrightmost_subexpression_precedence. That call scans the same operand a second time, so every level of nesting doubles the work. The second scan only matters if it returnsConsume, and it never does.In the binary, range and
letarms, the second scan uses the same precedence as the first. Only the offsets differ, and they cannot change the result.fail_offsetis always 0 or 1 but is only ever compared with 2, andbailout_offsetcan only cause an earlyConsume. So the second scan returns what the first one did. The prefix arm scans again withPrefixin place of the incoming precedence. The few operands where that changes the result still do not returnConsume, or are already excluded byscan_left, or bynext_operator_can_continue_expr, which is always false whennext_operatorisJump.This skips the second scan at those four call sites, so each scan visits an operand once, which is what allows for the generally improved performance reported below.
| ..0repetitionsFixes #1864.