Skip to content

Conversation

@danmoseley
Copy link
Member

@danmoseley danmoseley commented Jul 16, 2020

Summary

Fix #39390

Some simple regex patterns will not match. In this case the pattern "H#" would not match "#H#" iff RegexOptions.IgnoreCase | RegexOptions.Compiled.

Because the pattern contains a literal prefix (indeed it is the entire pattern) we will use Boyer-Moore to find the first instance of it. (One could imagine a more efficient way to search for a 2-character prefix.) Because the IgnoreCase was passed, we lowercase the pattern immediately to "h#", and when we match against a character in the text, we must lower case that character to compare it.

As a performance optimization, in the Compiled path, we avoid calling ToLower on the text candidate if we can cheaply verify that the character we are searching for is not be affected by case conversion. In this case, for example, we need not bother to lower case the text candidate character when we are searching for "#" because it is in a UnicodeCategory ("OtherPunctuation") which we know is not affected by case conversion. This optimization, like many others, does not exist in the non Compiled path.

The bug was that when deciding whether to lowercase the text candidate, instead of examining the character we were searching for, we were examining the last character of the prefix instead. In this repro case that is "#" so when searching for "H" we would not lower case it.

Customer Impact

This blocks Bing updating to the latest build.

Regression

Certainly yes

Risk

Very low. The change is localized and the problem well understood. I added a test that fails without this fix.

@danmoseley danmoseley added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 16, 2020
@ghost
Copy link

ghost commented Jul 16, 2020

Tagging subscribers to this area: @eerhardt, @pgovind
Notify danmosemsft if you want to be subscribed.

@danmoseley
Copy link
Member Author

git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=20 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/39421/merge:refs/remotes/pull/39421/merge
fatal: couldn't find remote ref refs/pull/39421/merge
##[warning]Git fetch failed with exit code 128, back off 1.319 seconds before retry.

@dotnet/dnceng ?

@lukas-lansky
Copy link
Contributor

That's strange. Let me check.

@danmoseley danmoseley closed this Jul 16, 2020
@danmoseley danmoseley reopened this Jul 16, 2020
@danmoseley danmoseley added Servicing-consider Issue for next servicing release review and removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) labels Jul 16, 2020
@danmoseley danmoseley requested a review from eerhardt July 16, 2020 14:41
@MattGal
Copy link
Member

MattGal commented Jul 16, 2020

git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=20 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/39421/merge:refs/remotes/pull/39421/merge
fatal: couldn't find remote ref refs/pull/39421/merge
##[warning]Git fetch failed with exit code 128, back off 1.319 seconds before retry.

@dotnet/dnceng ?

Have seen this before in both github and AzDO, it's usually a replication delay in the backing storage where one side thinks it's created files and the other side is fetching from a replicated version. Not much we can do when it's in github other than retry.

@danmoseley
Copy link
Member Author

@MattGal was it retrying and I somehow interrupted it? or it retried a few times and still no luck?

@danmoseley
Copy link
Member Author

Unlucky snap point?

2020-07-16T15:21:26.8944660Z F:\workspace\_work\1\s\.packages\microsoft.dotnet.apicompat\5.0.0-beta.20330.3\build\Microsoft.DotNet.ApiCompat.targets(145,5): error : TypesMustExist : Type 'System.Runtime.Intrinsics.Arm.AdvSimd' does not exist in the reference but it does exist in the implementation. [F:\workspace\_work\1\s\src\libraries\System.Text.Encodings.Web\src\System.Text.Encodings.Web.csproj]
2020-07-16T15:21:26.8991131Z F:\workspace\_work\1\s\.packages\microsoft.dotnet.apicompat\5.0.0-beta.20330.3\build\Microsoft.DotNet.ApiCompat.targets(145,5): error : TypesMustExist : Type 'System.Runtime.Intrinsics.Arm.ArmBase' does not exist in the reference but it does exist in the implementation. [F:\workspace\_work\1\s\src\libraries\System.Text.Encodings.Web\src\System.Text.Encodings.Web.csproj]
2020-07-16T15:21:26.9299761Z F:\workspace\_work\1\s\.packages\microsoft.dotnet.apicompat\5.0.0-beta.20330.3\build\Microsoft.DotNet.ApiCompat.targets(159,5): error : MatchingRefApiCompat failed - The reference assembly doesn't match all the APIs in the implementation for 'F:\workspace\_work\1\s\artifacts\bin\System.Text.Encodings.Web\netcoreapp3.0-Debug\System.Text.Encodings.Web.dll'. To address either fix errors in the reference assembly (referenced as implementation in compat errors for this reverse compat check), add the issues to the baseline file 'F:\workspace\_work\1\s\src\libraries\System.Text.Encodings.Web\src\MatchingRefApiCompatBaseline.txt' or disable this check by setting RunMatchingRefApiCompat=false in this project. [F:\workspace\_work\1\s\src\libraries\System.Text.Encodings.Web\src\System.Text.Encodings.Web.csproj]

@danmoseley
Copy link
Member Author

danmoseley commented Jul 16, 2020

@eiriktsarpalis these errors seem related to cd8759d ?

@MattGal
Copy link
Member

MattGal commented Jul 16, 2020

@MattGal was it retrying and I somehow interrupted it? or it retried a few times and still no luck?

By the time I looked that build was deleted from more commits being pushed, but this can succeed on retry.

@eiriktsarpalis
Copy link
Member

@danmosemsft the build error is tracked by this issue #39444

@jamshedd jamshedd added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jul 16, 2020
@jamshedd jamshedd added this to the 5.0 Preview 8 milestone Jul 16, 2020
@danmoseley danmoseley closed this Jul 16, 2020
@danmoseley danmoseley reopened this Jul 16, 2020
@danmoseley danmoseley requested a review from pgovind July 16, 2020 21:26
@danmoseley danmoseley removed the request for review from eerhardt July 16, 2020 22:02
@ghost
Copy link

ghost commented Jul 16, 2020

Hello @danmosemsft!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@danmoseley
Copy link
Member Author

@jaredpar one of the builds here did not complete in 60 minutes and timed out - OSX:

https://dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_apis/build/builds/734197/logs/22

@ghost ghost merged commit 082a5bf into dotnet:release/5.0-preview8 Jul 17, 2020
@danmoseley danmoseley deleted the portregexfix branch July 17, 2020 16:13
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants