-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Improve RegexInterpreter.Go code gen size #25096
Conversation
Changes look OK to me but we should find somebody in CoreFx to review. @stephentoub any suggestions? |
cc: @ViktorHofer |
Please give me a day or two to review the changes thoroughly. I need to finish something else before, which is timely. By quickly looking through it, the changes look good, thanks @benaadams! A good spot for optimizations as we are currently trying to improve our regex engine. |
@@ -465,6 +461,13 @@ protected override void Go() | |||
DumpState(); | |||
} | |||
#endif | |||
if (advance >= 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DEBUG code isn't critical, but technically this block should come before the above if block, right? Otherwise on iterations of the loop where calls to Advance(xyz) would have been made, the subsequent iteration would end up dumping the wrong state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
Ben do you have any data on the impact of this change? |
It should improve it a little; but it was mainly in reaction to the coreclr Jit change around inlining dotnet/coreclr#14850 which causes it to increase by 3.8kB of asm when compiled (for just this method). |
…icant digit if one exists. (dotnet#25096) * Updating Dragon4 to ensure the number buffer always provides a significant digit if one exists. * Changing System.Number.RoundNumber to not round up floating-point numbers. * Re-enabling the RealFormatterTestsBase CoreFX tests * Updating Number.RoundNumber to take a isCorrectlyRounded parameter and to use IEEE compliant rounding for floating-point numbers. * Change SinglePrecisionCustomFormat to 7, ensuring it matches the value used in netcoreapp2.1 Signed-off-by: dotnet-bot <[email protected]>
…icant digit if one exists. (#25096) * Updating Dragon4 to ensure the number buffer always provides a significant digit if one exists. * Changing System.Number.RoundNumber to not round up floating-point numbers. * Re-enabling the RealFormatterTestsBase CoreFX tests * Updating Number.RoundNumber to take a isCorrectlyRounded parameter and to use IEEE compliant rounding for floating-point numbers. * Change SinglePrecisionCustomFormat to 7, ensuring it matches the value used in netcoreapp2.1 Signed-off-by: dotnet-bot <[email protected]>
* Improve RegexInterpreter.Go code gen size * feedback Commit migrated from dotnet/corefx@047af1a
When the Advance call is inlined it causes considerable code gen size increase (as there are so many occurrences in a single method).
This can be reduced to a single call to resolve it.
From: dotnet/coreclr#14850 (comment)
@AndyAyersMS PTAL