Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Improve RegexInterpreter.Go code gen size #25096

Merged
merged 2 commits into from
Nov 9, 2017

Conversation

benaadams
Copy link
Member

@benaadams benaadams commented Nov 7, 2017

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

@AndyAyersMS
Copy link
Member

Changes look OK to me but we should find somebody in CoreFx to review.

@stephentoub any suggestions?

@stephentoub
Copy link
Member

cc: @ViktorHofer

@ViktorHofer
Copy link
Member

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)
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@ViktorHofer
Copy link
Member

Ben do you have any data on the impact of this change?

@benaadams
Copy link
Member Author

benaadams commented Nov 9, 2017

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).

@stephentoub stephentoub merged commit 047af1a into dotnet:master Nov 9, 2017
@karelz karelz added this to the 2.1.0 milestone Nov 18, 2017
@benaadams benaadams deleted the regex-go branch January 30, 2018 15:19
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corefx that referenced this pull request Jun 26, 2019
…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]>
Anipik pushed a commit that referenced this pull request Jun 26, 2019
…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]>
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* Improve RegexInterpreter.Go code gen size

* feedback


Commit migrated from dotnet/corefx@047af1a
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants