Fix RegexCompiler regression on 32-bit for some set matching #68655
Conversation
stephentoub
added
area-System.Text.RegularExpressions
tenet-performance
|
Tagging subscribers to this area: @dotnet/area-system-text-regularexpressions Issue DetailsWe added an optimization to regex where for sets containing values all within 64 characters of each other (e.g. all hex digits), we use a ulong to represent a bitmap and can implement the check in an entirely branchless manner. This results in a measurable win on 64-bit, e.g. upwards of 20% for some patterns. Unfortunately, it also results in a measurable regression on 32-bit. This PR fixes that for RegexCompiler by special-casing the optimization to only apply when IntPtr.Size == 8. For the source generator, we don't have the same luxury of knowing that the code is emitted and used on the same bitness, so since it would result in very complicated code to emit multiple implementations and since we generally prefer optimizing for 64-bit, I've left it in for the source generator. Fixes #68565
|
ghost
assigned
stephentoub
requested review from
GrabYourPitchforks
and removed request for
GrabYourPitchforks
|
@GrabYourPitchforks, please let me know if you have any better ideas for the source generator :) One thing I contemplated was adding a 32-bit path for values within 32 of each other, so that we'd only have the 32-bit regression for sets between 33 and 64, but the most impactful ones (e.g. hex digits) fall in that range. |
We added an optimization to regex where for sets containing values all within 64 characters of each other (e.g. all hex digits), we use a ulong to represent a bitmap and can implement the check in an entirely branchless manner. This results in a measurable win on 64-bit, e.g. upwards of 20% for some patterns. Unfortunately, it also results in a measurable regression on 32-bit. This PR fixes that for RegexCompiler by special-casing the optimization to only apply when IntPtr.Size == 8. For the source generator, we don't have the same luxury of knowing that the code is emitted and used on the same bitness, so since it would result in very complicated code to emit multiple implementations and since we generally prefer optimizing for 64-bit, I've left it in for the source generator.
stephentoub
force-pushed
the
fix32bitregression
branch
from
51342ec to
f8d2f2a
Compare
| if (analysis.OnlyRanges && (analysis.UpperBoundExclusiveIfOnlyRanges - analysis.LowerBoundInclusiveIfOnlyRanges) <= 64) | ||
| // We skip this on 32-bit, as otherwise using 64-bit numbers in this manner is a deoptimization | ||
| // when compared to the subsequent fallbacks. | ||
| if (IntPtr.Size == 8 && analysis.OnlyRanges && (analysis.UpperBoundExclusiveIfOnlyRanges - analysis.LowerBoundInclusiveIfOnlyRanges) <= 64) |
There was a problem hiding this comment.
Environment.Is64BitProcess is a little more descriptive.
There was a problem hiding this comment.
Can address in a follow-up as necessary
We added an optimization to regex where for sets containing values all within 64 characters of each other (e.g. all hex digits), we use a ulong to represent a bitmap and can implement the check in an entirely branchless manner. This results in a measurable win on 64-bit, e.g. upwards of 20% for some patterns. Unfortunately, it also results in a measurable regression on 32-bit. This PR fixes that for RegexCompiler by special-casing the optimization to only apply when IntPtr.Size == 8. For the source generator, we don't have the same luxury of knowing that the code is emitted and used on the same bitness, so since it would result in very complicated code to emit multiple implementations and since we generally prefer optimizing for 64-bit, I've left it in for the source generator.
Fixes #68565