Avoid OverflowException in Boolean.TryFormat

[xtqqczze]

MemoryMarshal.AsBytes would throw unnecessarily when destination.Length exceeds 0x3FFFFFFF.

[xtqqczze]

@MihuBot

[xtqqczze]

@MihuBot

[huoyaoyuan]

This may get superseded by #108575

[EgorBo]

This may get superseded by #108575

I was just wondering about perf overhead from a more idiomatic code (since we already do the same for UTF8 formatter for bools, see here)

Surprisingly, the idiomatic way seems to be cheaper? 😐

[xtqqczze]

@EgorBot -intel -arm64 -profiler

using System.Runtime.CompilerServices;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

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

public class Benchmarks
{
    static bool[] _values = Enumerable.Range(0, 256).Select(i => i % 2 == 0).ToArray();
    static char[] _output = new char[4096];

    [Benchmark]
    public void WriteBools()
    {
        Span<char> output = _output;
        foreach (var value in _values)
        {
            if (value.TryFormat(output, out int written))
                output = output.Slice(written);
            else
                throw new InvalidOperationException();
        }

        Consume(output);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    void Consume(Span<char> _){}
}

[EgorBo]

@xtqqczze feel free to just incorporate my change in your PR, but we may need to keep your version for Mono since it can't unroll Memmove.

[jkotas]

just incorporate #108575 in your PR, but we may need to keep your version for Mono since it can't unroll Memmove.

bool formatting is very unlikely to be on a hot path in scenarios targeted by Mono. I think it would be acceptable to take regression for Mono to keep the code simple and safe.

[xtqqczze]

Related: #77398

[xtqqczze]

@EgorBot -mono -intel -arm64

using System.Runtime.CompilerServices;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

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

public class Benchmarks
{
    static bool[] _values = Enumerable.Range(0, 256).Select(i => i % 2 == 0).ToArray();
    static char[] _output = new char[4096];

    [Benchmark]
    public void WriteBools()
    {
        Span<char> output = _output;
        foreach (var value in _values)
        {
            if (value.TryFormat(output, out int written))
                output = output.Slice(written);
            else
                throw new InvalidOperationException();
        }

        Consume(output);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    void Consume(Span<char> _){}
}

[EgorBo]

bool formatting is very unlikely to be on a hot path in scenarios targeted by Mono. I think it would be acceptable to take regression for Mono to keep the code simple and safe.

Agree. In fact it improves Mono-jit (mini), but slightly regresses Interpreter.

[xtqqczze]

@EgorBot -mono -intel -arm64 --envvars MONO_ENV_OPTIONS:--interpreter

using System.Runtime.CompilerServices;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

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

public class Benchmarks
{
    static bool[] _values = Enumerable.Range(0, 256).Select(i => i % 2 == 0).ToArray();
    static char[] _output = new char[4096];

    [Benchmark]
    public void WriteBools()
    {
        Span<char> output = _output;
        foreach (var value in _values)
        {
            if (value.TryFormat(output, out int written))
                output = output.Slice(written);
            else
                throw new InvalidOperationException();
        }

        Consume(output);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    void Consume(Span<char> _){}
}

[xtqqczze]

bool formatting is very unlikely to be on a hot path in scenarios targeted by Mono. I think it would be acceptable to take regression for Mono to keep the code simple and safe.

@jkotas For monointerpreter, the ratio between #108572 and #108575 ratio is 0.60.

Benchmark results on AzureAmpere

BenchmarkDotNet v0.14.0, Ubuntu 22.04.5 LTS (Jammy Jellyfish)
AzureAmpere
  Job-QUXZYN : .NET 10.0.0 (42.42.42.42424) using MonoVM, Arm64 AOT
  Job-CRJMBB : .NET 10.0.0 (42.42.42.42424) using MonoVM, Arm64 AOT
EnvironmentVariables=MONO_ENV_OPTIONS=--interpreter

Method	Toolchain	Mean	Error	Ratio
WriteBools	Main	22.75 μs	0.266 μs	1.00
WriteBools	#108572	16.58 μs	0.329 μs	0.73
WriteBools	Main	22.00 μs	0.440 μs	1.00
WriteBools	#108575	26.90 μs	0.529 μs	1.22

[EgorBo]

For monointerpreter, the ratio between #108572 and #108575 ratio is 0.60.

I think 20% regression is fine here, even the best case is still a lot slower than the JIT (16μs vs ~200ns) , so it can be handled via an intrinsic in the interpreter if it really becomes a bottle-neck. A safer/easier-to-read code worth it.

[xtqqczze]

@EgorBo Do you expect to merge #108576 soon?

[EgorBo]

@EgorBo Do you expect to merge #108576 soon?

You don't have to wait for it, it improves a tiny bit

[xtqqczze]

@EgorBot -intel -arm64 -profiler

using System.Runtime.CompilerServices;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

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

public class Benchmarks
{
    static bool[] _values = Enumerable.Range(0, 256).Select(i => i % 2 == 0).ToArray();
    static char[] _output = new char[4096];

    [Benchmark]
    public void WriteBools()
    {
        Span<char> output = _output;
        foreach (var value in _values)
        {
            if (value.TryFormat(output, out int written))
                output = output.Slice(written);
            else
                throw new InvalidOperationException();
        }

        Consume(output);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    void Consume(Span<char> _){}
}

[EgorBo]

LGTM, I think it's fine to leave constants instead of .Length to avoid making mono even more slower, but I don't have a strong opinion. I think it should be a good idea to fold ldstr+length in Mono too (I once tried to do so in Mono via mono/mono#16898 but didn't finish)

[xtqqczze]

LGTM, I think it's fine to leave constants instead of .Length to avoid making mono even more slower, but I don't have a strong opinion. I think it should be a good idea to fold ldstr+length in Mono too (I once tried to do so in Mono via mono/mono#16898 but didn't finish)

Issue opened: #108857

[jkotas]

LGTM