Use StartsWith in regex compiler / source gen for shorter strings #65222
Conversation
The RegexCompiler and source generator currently special-case strings < 64 chars in length and unroll the loop, using a series of ulong and uint comparisons where possible. While efficient, this makes the generated code harder to read, and the source generator also has endianness issues when the compiled binary is then used on a machine with different endianness. The JIT is going to start doing such unrolling as part of StartsWith, so we can leave the optimization up to it; it'll be able to do it better, anyway, with its optimization applying to more uses, using vectors where applicable, etc.
stephentoub
added
NO-MERGE
|
Tagging subscribers to this area: @dotnet/area-system-text-regularexpressions Issue DetailsThe RegexCompiler and source generator currently special-case strings < 64 chars in length and unroll the loop, using a series of ulong and uint comparisons where possible. While efficient, this makes the generated code harder to read, and the source generator also has endianness issues when the compiled binary is then used on a machine with different endianness. The JIT is going to start doing such unrolling as part of StartsWith, so we can leave the optimization up to it; it'll be able to do it better, anyway, with its optimization applying to more uses, using vectors where applicable, etc.
|
stephentoub
removed
the
NO-MERGE
src/libraries/System.Text.RegularExpressions/gen/RegexGenerator.Emitter.cs
Show resolved
Hide resolved
It had been, but then removed it and decided we can live with some temporary regressions. As it stands, the current code is incorrect with big endian in mind, so we'd need to do something here regardless... may as well delete :) |
|
Regression: dotnet/perf-autofiling-issues#3560 and dotnet/perf-autofiling-issues#3571 (supposed to be fixed via #65288) |
ghost
locked as resolved and limited conversation to collaborators
|
@EgorBo it seems that the regression has not been solved yet:
Please let me know if this is by design or not (I'll open a new issue) |
|
@adamsitnik thanks, I'll take a look later today why |
|
Some other regex benchmarks have regressed as well (cc @stephentoub):
|
Thanks. That's the same case... this is the code we generate now at the heart of the matching for that pattern: // Match with 4 alternative expressions, atomically.
{
if (slice.IsEmpty)
{
goto CharLoopBacktrack;
}
switch (slice[0])
{
case 'T':
// Match the string "om".
if (!slice.Slice(1).StartsWith("om"))
{
goto CharLoopBacktrack;
}
pos += 3;
slice = inputSpan.Slice(pos);
break;
case 'S':
// Match the string "awyer".
if (!slice.Slice(1).StartsWith("awyer"))
{
goto CharLoopBacktrack;
}
pos += 6;
slice = inputSpan.Slice(pos);
break;
case 'H':
// Match the string "uckleberry".
if (!slice.Slice(1).StartsWith("uckleberry"))
{
goto CharLoopBacktrack;
}
pos += 11;
slice = inputSpan.Slice(pos);
break;
case 'F':
// Match the string "inn".
if (!slice.Slice(1).StartsWith("inn"))
{
goto CharLoopBacktrack;
}
pos += 4;
slice = inputSpan.Slice(pos);
break;
default:
goto CharLoopBacktrack;
}
}Prior to this change, we would have manually unrolled all of those StartsWith in the generated code. |
|
@stephentoub @adamsitnik ok I've just figured out why this regression wasn't addressed - it's because of this VM call - runtime/src/coreclr/vm/jitinterface.cpp Lines 681 to 684 in 316797b for Dynamic Context this API only returns string literal's length and doesn't return actual string content so my optimization just gives up. I'll file a fix |
|
Interesting. Even before your fix, we should be able to confirm that's the issue by trying the same benchmark with the source generator instead of RegexOptions.Compiled, since the source generator doesn't use reflection emit / dynamic methods. |
|
Yes, I don't see a reason why it'd not work with source-gen based Regexes (except too long string fed to StartsWith, currently it's limited with 32 chars on x64 and 16 on arm64) |
You're planning to increase that via use of wider vector operations, right? Regardless, those limits should be plenty wide for these benchmarks. We also previously only unrolled up to 64 chars. I'll try today with the source generator. |
Yes, I already have a branch for it EgorBo@e28e5cc but wanted to do some general clean up as part of it |
|
Just as confirmation, I tried the perf test that regressed above, comparing RegexOptions.Compiled with the source generator, and the source generated version is indeed much faster:
so hopefully your dynamic fix will address the problem. |
The RegexCompiler and source generator currently special-case strings < 64 chars in length and unroll the loop, using a series of ulong and uint comparisons where possible. While efficient, this makes the generated code harder to read, and the source generator also has endianness issues when the compiled binary is then used on a machine with different endianness. The JIT is going to start doing such unrolling as part of StartsWith (#65288), so we can leave the optimization up to it; it'll be able to do it better, anyway, with its optimization applying to more uses, using vectors where applicable, etc.
cc: @EgorBo, @joperezr