Skip to content

Conversation

@GrabYourPitchforks
Copy link
Member

Getting rid of some low-hanging fruit.

The only place this helper is used is from the CultureInfo ctor, via:

/// <remarks>
/// This is ONLY used for caching names and shouldn't be used for anything else
/// </remarks>
internal static string AnsiToLower(string testString) => TextInfo.ToLowerAsciiInvariant(testString);

Representative call site (see line 1033):

public static CultureInfo GetCultureInfo(string name)
{
ArgumentNullException.ThrowIfNull(name);
name = CultureData.AnsiToLower(name);

This is: (a) not a hot code path; and (b) expected to contain very small strings, typically of the form aa-BB. Seems like it's an ideal candidate for simplifying the code.

Copy link
Contributor

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 successfully reduces unsafe code usage in TextInfo.cs by refactoring the ToLowerAsciiInvariant(string) method to use modern, safe C# patterns. The method is used internally for caching culture names (typically short strings like "aa-BB") and is not performance-critical.

Key Changes

  • Removed unsafe modifier from the method signature
  • Replaced pointer-based operations (fixed, FastAllocateString) with string.Create API
  • Simplified the implementation using span-based iteration with foreach (ref char c in span)

@tarekgh tarekgh added this to the 11.0.0 milestone Jan 21, 2026
Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

LGTM!

@MihaZupan
Copy link
Member

/ba-g AOT timeouts

@GrabYourPitchforks GrabYourPitchforks merged commit bb5ea9b into dotnet:main Jan 26, 2026
143 of 147 checks passed
jkotas added a commit that referenced this pull request Jan 27, 2026
@jkotas
Copy link
Member

jkotas commented Jan 27, 2026

/ba-g AOT timeouts

@MihaZupan NativeAOT failures were not all timeouts:

image

It is a real failure:

****************************************************
* Size test                                        *
* Size of the executable is   1,555 kB             *
****************************************************
BUG: File size is not in the expected range (1116160 to 1536000 bytes). Did a libraries change regress size of Hello World?
Expected: 100
Actual: 1

I suspect that this regression was introduced by adding a dependency on ValueTuple<T1,T2> that is quite heavy type. I am validating revert in #123692

@jkotas
Copy link
Member

jkotas commented Jan 28, 2026

MichalStrehovsky/rt-sz#203 confirmed this change introduced 200kB - 300kB regression in hello world NAOT binary size.

jkotas added a commit that referenced this pull request Jan 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants