Skip to content

Simplify and optimize utf8.c - #136915

Merged
jkotas merged 4 commits into
dotnet:mainfrom
am11:patch-61
Nov 4, 2026
Merged

jkotas merged 4 commits into
dotnet:mainfrom
am11:patch-61

Conversation

@am11

@am11 am11 commented Nov 3, 2026

Copy link
Copy Markdown
Member

Reviving #102424 with optimizations.

Benchmark (median of 5 repetitions, ns) Win32 main #102424 new new/Win32 new/main
get_length_utf16_to_utf8 / ASCII_Short 42.2 41.9 43.3 42.5 1.01× 1.01×
get_length_utf16_to_utf8 / NonASCII_Short 52.0 49.3 44.2 49.5 0.95× 1.00×
get_length_utf16_to_utf8 / ASCII_Long 51.5 53.7 84.2 45.5 0.88× 0.85×
get_length_utf16_to_utf8 / NonASCII_Long 142.5 137.3 127.6 91.8 0.64× 0.67×
convert_utf16_to_utf8 / ASCII_Short 83.7 85.7 92.5 77.7 0.93× 0.91×
convert_utf16_to_utf8 / NonASCII_Short 114.0 110.2 94.4 97.8 0.86× 0.89×
convert_utf16_to_utf8 / ASCII_Long 119.0 128.9 229.1 97.1 0.82× 0.75×
convert_utf16_to_utf8 / NonASCII_Long 370.6 412.0 325.8 261.5 0.71× 0.63×
get_length_utf8_to_utf16 / ASCII_Short 42.1 42.3 43.2 41.1 0.98× 0.97×
get_length_utf8_to_utf16 / NonASCII_Short 55.1 55.4 71.0 50.5 0.92× 0.91×
get_length_utf8_to_utf16 / ASCII_Long 45.7 45.3 79.5 43.5 0.95× 0.96×
get_length_utf8_to_utf16 / NonASCII_Long 107.1 114.3 144.7 84.3 0.79× 0.74×
convert_utf8_to_utf16 / ASCII_Short 82.3 82.7 81.2 76.9 0.93× 0.93×
convert_utf8_to_utf16 / NonASCII_Short 124.8 125.3 127.8 106.1 0.85× 0.85×
convert_utf8_to_utf16 / ASCII_Long 102.6 105.6 170.1 94.9 0.92× 0.90×
convert_utf8_to_utf16 / NonASCII_Long 244.9 259.6 269.0 190.5 0.78× 0.73×

@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Nov 3, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

@am11

am11 commented Nov 3, 2026

Copy link
Copy Markdown
Member Author

This is a drop-in replacement for the existing API, and well-formed input converts exactly as before, as does ill-formed input mid-string (same U+FFFD replacement as main). It does change four edge cases where main misbehaved:

  1. An incomplete sequence at the end of explicit-length UTF-8 input now becomes U+FFFD. Main counted a replacement character for it but didn't write one, so the length query and the conversion disagreed: "A\xF0\x90" counted 2 and converted 1, and "\xC2" alone returned 0 without setting an error.
  2. A replacement character that doesn't fit in the destination now fails with ERROR_INSUFFICIENT_BUFFER. In both directions, main returned a shorter count with no error. For example, "ab\x80" into 2 UTF-16 units returned 2, and { 0x41, 0xDC00 } into 3 bytes returned 1.
  3. MB_ERR_INVALID_CHARS now fails with ERROR_NO_UNICODE_TRANSLATION for an incomplete sequence at the end of the input too. Main either dropped those bytes and succeeded ("A\xC2" returned 1), or failed with ERROR_INSUFFICIENT_BUFFER when nothing had been written yet ("\xC2"), which Mono's g_utf8_to_utf16 reported as "Allocation failed".
  4. MINIPAL_TREAT_AS_LITTLE_ENDIAN on big-endian platforms now byte-swaps every UTF-16 code unit. Main left some non-ASCII, surrogate and replacement-character code units unswapped, and its UTF-16→UTF-8 length query could disagree with the conversion. This mode is used by Mono's EventPipe on s390x.

After this (and #136821), I intend to rewrite unicode related PAL tests somewhere under src/tests, before deleting PAL's unicode.cpp wrapper layer and deleting unicode PAL tests.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The WideCharToMultiByte tests lack coverage of the new 4096-block accumulation/reset boundary, leaving byte-count errors that could cause undersized buffer allocations unchecked.

1 open issue
What changed in this PR

This PR simplifies minipal’s UTF-8/UTF-16 conversion implementation, reviving #102424 with additional fast paths for length calculation and conversion.

Changes:

  • Replaces stateful fallback machinery with focused decoding and encoding helpers.
  • Adds word-sized ASCII processing and specialized loops for multibyte sequences and surrogate pairs.
  • Centralizes malformed-input handling, byte swapping, and destination-capacity checks.
File Description
src/​native/​minipal/​utf8.c Rewrites UTF conversion and length-calculation routines with simpler helpers and optimized fast paths.

Review effort: Balanced

Comment thread src/native/minipal/utf8.c Resolved
Comment thread src/native/minipal/utf8.c Resolved
am11 and others added 2 commits November 3, 2026 22:21
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Comment thread src/native/minipal/utf8.c Resolved
Comment thread src/native/minipal/utf8.c Resolved
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@jkotas

jkotas commented Nov 3, 2026

Copy link
Copy Markdown
Member

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@jkotas jkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you

@jkotas

jkotas commented Nov 4, 2026

Copy link
Copy Markdown
Member

/ba-g known failures

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-PAL-coreclr only for closed issues community-contribution Indicates that the PR has been added by a community member tenet-performance Performance related issue

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants