Skip to content

Rewrite ConvertToDecimal in XslNumber.cs to use safe code with string.Create and for loop#124794

Open
Copilot wants to merge 3 commits intomainfrom
copilot/refactor-convert-to-decimal
Open

Rewrite ConvertToDecimal in XslNumber.cs to use safe code with string.Create and for loop#124794
Copilot wants to merge 3 commits intomainfrom
copilot/refactor-convert-to-decimal

Conversation

Copy link
Contributor

Copilot AI commented Feb 24, 2026

Replaces the unsafe block in NumberFormatter.ConvertToDecimal (XslNumber.cs) — which used stackalloc, raw pointer arithmetic, and a while (true) loop — with safe code using string.Create and a bounds-check-friendly for loop.

Changes

  • XslNumber.cs: ConvertToDecimal rewritten. Key details:

    • string.Create(newLen, (str, shift, zero, separator, groupSize), static (result, state) => ...) writes directly into the result string, eliminating the intermediate stackalloc buffer copy
    • for (int newPos = result.Length - 1; newPos >= 0; newPos--) replaces the while (true) + pointer-walk; a cnt counter drives grouping-separator insertion identically to the original algorithm
    • groupSize != 0 && cnt == 0 guard correctly handles both the grouping path and the pure-shift path (groupSize == 0, shift != 0) without special-casing
    • All fast-path early returns (return str, return str.PadLeft(...)) are preserved
  • XslCompilerTests.cs: Added XslNumberDecimalFormatting [Theory] with 12 cases covering all code paths through ConvertToDecimal: no-op fast path, padding-only fast path, grouping separator, padding + grouping, and non-ASCII digit system (Arabic-Indic, shift != 0)


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@github-actions github-actions bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 24, 2026
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
Copilot AI changed the title [WIP] Refactor ConvertToDecimal in XslNumber.cs for safety and performance Rewrite ConvertToDecimal in XslNumber.cs to use safe code with string.Create and for loop Feb 24, 2026
@EgorBo
Copy link
Member

EgorBo commented Feb 24, 2026

@EgorBot -linux_amd -osx_arm64

using System.IO;
using System.Xml;
using System.Xml.Xsl;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

BenchmarkSwitcher.FromAssembly(typeof(Bench).Assembly).Run(args);

[MemoryDiagnoser]
public class Bench
{
    private XslCompiledTransform _xslNoGrouping = default!;
    private XslCompiledTransform _xslWithGrouping = default!;
    private XslCompiledTransform _xslWithPaddingAndGrouping = default!;
    private XslCompiledTransform _xslNonAsciiDigits = default!;
    private string _xml = "<root/>";

    [GlobalSetup]
    public void Setup()
    {
        _xslNoGrouping = Compile("""
            <xsl:stylesheet version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform">
              <xsl:output method="text" />
              <xsl:template match="/"><xsl:number value="1234567" format="1"/></xsl:template>
            </xsl:stylesheet>
            """);

        _xslWithGrouping = Compile("""
            <xsl:stylesheet version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform">
              <xsl:output method="text" />
              <xsl:template match="/"><xsl:number value="1234567" format="1" grouping-separator="," grouping-size="3"/></xsl:template>
            </xsl:stylesheet>
            """);

        _xslWithPaddingAndGrouping = Compile("""
            <xsl:stylesheet version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform">
              <xsl:output method="text" />
              <xsl:template match="/"><xsl:number value="42" format="00001" grouping-separator="," grouping-size="3"/></xsl:template>
            </xsl:stylesheet>
            """);

        _xslNonAsciiDigits = Compile("""
            <xsl:stylesheet version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform">
              <xsl:output method="text" />
              <xsl:template match="/"><xsl:number value="1234" format="&#x0661;"/></xsl:template>
            </xsl:stylesheet>
            """);
    }

    private static XslCompiledTransform Compile(string xsl)
    {
        var t = new XslCompiledTransform();
        using var r = XmlReader.Create(new StringReader(xsl));
        t.Load(r);
        return t;
    }

    [Benchmark]
    public string NoGrouping()
    {
        var sw = new StringWriter();
        using var xr = XmlReader.Create(new StringReader(_xml));
        _xslNoGrouping.Transform(xr, null, sw);
        return sw.ToString();
    }

    [Benchmark]
    public string WithGroupingSeparator()
    {
        var sw = new StringWriter();
        using var xr = XmlReader.Create(new StringReader(_xml));
        _xslWithGrouping.Transform(xr, null, sw);
        return sw.ToString();
    }

    [Benchmark]
    public string WithPaddingAndGrouping()
    {
        var sw = new StringWriter();
        using var xr = XmlReader.Create(new StringReader(_xml));
        _xslWithPaddingAndGrouping.Transform(xr, null, sw);
        return sw.ToString();
    }

    [Benchmark]
    public string NonAsciiDigits()
    {
        var sw = new StringWriter();
        using var xr = XmlReader.Create(new StringReader(_xml));
        _xslNonAsciiDigits.Transform(xr, null, sw);
        return sw.ToString();
    }
}

@EgorBo EgorBo marked this pull request as ready for review February 24, 2026 15:54
Copilot AI review requested due to automatic review settings February 24, 2026 15:54
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 modernizes the ConvertToDecimal method in XslNumber.cs by replacing unsafe pointer arithmetic with safe managed code using string.Create. The refactoring eliminates the unsafe block, stackalloc, and while(true) loop while preserving the exact same behavior and performance characteristics.

Changes:

  • Rewrote ConvertToDecimal to use string.Create with a static lambda and state tuple, replacing unsafe pointer-based string construction
  • Added comprehensive test coverage with 12 test cases covering all code paths (fast paths, grouping separators, padding, and non-ASCII digit systems)

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
XslNumber.cs Replaced unsafe pointer arithmetic with safe string.Create pattern; changed while(true) loop to bounds-check-friendly for loop; preserved all fast-path optimizations
XslCompilerTests.cs Added XslNumberDecimalFormatting theory test with 12 cases covering no-op, padding-only, grouping separators, combined padding+grouping, and Arabic-Indic digits

@EgorBo
Copy link
Member

EgorBo commented Feb 24, 2026

PTAL @stephentoub

It seems like this function had an unbound stackalloc with a size that could come from user input + general unsafe code removal. JIT removes bounds check for result, but has to keep it for str[oldPos--] but I wasn't able to detect a perf regression from that.

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

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI reduce-unsafe

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants