Remove implicit anchoring optimization from Regex #42408
Merged
Conversation
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
|
Tagging subscribers to this area: @eerhardt, @pgovind, @jeffhandley |
|
/backport to release/5.0-rc2 |
|
Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/260042103 |
danmoseley
reviewed
Sep 17, 2020
src/libraries/System.Text.RegularExpressions/tests/Regex.Match.Tests.cs
Outdated
Show resolved
Hide resolved
danmoseley
reviewed
Sep 17, 2020
src/libraries/System.Text.RegularExpressions/tests/Regex.Match.Tests.cs
Outdated
Show resolved
Hide resolved
pgovind
approved these changes
Sep 17, 2020
jeffhandley
approved these changes
Sep 17, 2020
In .NET 5 we added a bunch of optimizations to Regex. One of them was a transform that optimized for the case where the pattern begins with `.*`. If it does, then we insert an implicit anchor at the beginning in order to avoid unnecessary backtracking. Imagine the pattern `.*a` and the pattern `bcdefghijklmnopqrstuvwxyz`. This is going to start matching at `b`, find the next newline, and then backtrack from there looking for the `a`; it won't find it and will backtrack all the way, failing the match at that position. At that point it'll bump to the next position, starting at `c`, and do it all over. It'll fail, backtrack all the way, and bump again, starting at `d`, and doing it all over. Etc. The optimization recognizes that since `.` will match anything other than newline, after it fails to match at the first position, we can just skip all subsequent positions until the next newline, as they're all going to fail. However, the optimization failed to take into account that someone can explicitly start a match in the middle of the provided text. In that case, the implicitly added anchor will fail the match in the actual "Go" matching logic. There are safe ways to do this optimization, e.g. introducing a variant of these anchors that let FindFirstChar skip ahead but that aren't considered for Go's matching purposes, but we can look at employing those for .NET 6. For now for .NET 5, this commit just deletes the faulty optimization and adds a few tests that were failing it.
stephentoub
force-pushed
the
fixregexopt
branch
from
146e300 to
7c1a0da
Compare
|
Remaining check timeouts are unrelated. |
This was referenced Sep 19, 2020
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
In .NET 5 we added a bunch of optimizations to Regex. One of them was a transform that optimized for the case where the pattern begins with
.*. If it does, then we insert an implicit anchor at the beginning in order to avoid unnecessary backtracking. Imagine the pattern.*aand the patternbcdefghijklmnopqrstuvwxyz. This is going to start matching atb, find the next newline, and then backtrack from there looking for thea; it won't find it and will backtrack all the way, failing the match at that position. At that point it'll bump to the next position, starting atc, and do it all over. It'll fail, backtrack all the way, and bump again, starting atd, and doing it all over. Etc. The optimization recognizes that since.will match anything other than newline, after it fails to match at the first position, we can just skip all subsequent positions until the next newline, as they're all going to fail.However, the optimization failed to take into account that someone can explicitly start a match in the middle of the provided text. In that case, the implicitly added anchor will fail the match in the actual "Go" matching logic.
There are safe ways to do this optimization, e.g. introducing a variant of these anchors that let FindFirstChar skip ahead but that aren't considered for Go's matching purposes, but we can look at employing those for .NET 6. For now for .NET 5, this commit just deletes the faulty optimization and adds a few tests that were failing it.
#42390
#42392
cc: @pgovind, @eerhardt, @danmosemsft