Skip to content

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Aug 7, 2025

Some changes in #110955 PR regressed Mono runtime significantly (>150 benchmarks) given number formatting is often a perf sensitive area I'm reverting that change for now, we'll figure out it later in 11.

@Copilot Copilot AI review requested due to automatic review settings August 7, 2025 21:08
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR partially reverts performance regression introduced in PR #110955 that affected Mono runtime significantly. The changes revert optimizations in number formatting that used ReadOnlySpan<byte> properties back to static readonly byte arrays to restore performance.

  • Reverts TwoDigitsCharsAsBytes and TwoDigitsBytes from ReadOnlySpan<byte> properties back to static readonly byte[] fields
  • Updates references from MemoryMarshal.GetReference to MemoryMarshal.GetArrayDataReference to match the reverted field types
  • Adds .ToArray() calls to convert the concatenated UTF-8 string literals back to byte arrays

@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 7, 2025
@EgorBo
Copy link
Member Author

EgorBo commented Aug 7, 2025

PTAL @stephentoub @lewing

@jkotas
Copy link
Member

jkotas commented Aug 11, 2025

the new/old code is still unsafe so it will require some rewrite anyway.

I expect that the rewrite would have to be ifdefed out for Mono as well, to avoid introducing even worse perf regression.

Copy link
Member

@lewing lewing left a comment

Choose a reason for hiding this comment

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

approving for wasm/mono since this we see this in key scenarios, I'm fine with an ifdef if that is preferred.

@lewing lewing added this to the 10.0.0 milestone Aug 14, 2025
@EgorBo
Copy link
Member Author

EgorBo commented Aug 14, 2025

Added if-defs, PTAL @jkotas

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM. I assume that this is zero-diff change for CoreCLR and NAOT.

@lewing lewing enabled auto-merge (squash) August 15, 2025 16:15
@lewing
Copy link
Member

lewing commented Aug 15, 2025

/ba-g coreclr android timeouts are known and resolved on the tip of main

@lewing lewing merged commit f514fa5 into dotnet:main Aug 15, 2025
140 of 144 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 15, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants