Replace signed comparisons with unsigned comparisons in bounds checking ImmutableArray<T>.Builder

[prozolic]

This PR replaces signed comparisons with unsigned comparisons in bounds checking for ImmutableArray<T>.Builder indexer and ItemRef method. Change bounds check from if (index >= this.Count) to if ((uint)index >= (uint)this.Count).

[EgorBo]

Any specific pattern you're trying to improve? can you provide before/after asm for it?

Although, your change kind of makes sense to me, It looks like it's mostly a regression, see MihuBot/runtime-utils#1584 (comment)

[xtqqczze]

What is the motivation for the change, is it a correctness issue?

[prozolic]

@EgorBo Thanks for the feedback.

With the current implementation, negative indices pass the bounds check (-1 >= this.Count is false), and the exception is only thrown later by the array access itself. While the result is the same IndexOutOfRangeException, I think that this seems inconsistent with explicit bounds checking semantics.

However, I understand that this change leads to a significant performance regression as shown in MihuBot/runtime-utils#1584. Considering performance regression, would you prefer that I close this PR?

[EgorBo]

I think that this seems inconsistent with explicit bounds checking semantics.

Ah, I see. Looks like in both cases exactly the same exception is thrown - IndexOutOfRangeException with the same text, so not sure it's actually an issue? stack-trace might be slightly different, thought:

too big index:

Unhandled exception. System.IndexOutOfRangeException: Index was outside the bounds of the array.
   at System.Collections.Immutable.ImmutableArray`1.Builder.ThrowIndexOutOfRangeException()
   at System.Collections.Immutable.ImmutableArray`1.Builder.ItemRef(Int32 index)

negative index:

Unhandled exception. System.IndexOutOfRangeException: Index was outside the bounds of the array.
   at System.Collections.Immutable.ImmutableArray`1.Builder.ItemRef(Int32 index)

[xtqqczze]

stack-trace might be slightly different, thought:

The stack trace differs only because ThrowIndexOutOfRangeException doesn’t have the [StackTraceHidden] attribute that most throw helpers have.

[EgorBo]

stack-trace might be slightly different, thought:

The stack trace differs only because ThrowIndexOutOfRangeException doesn’t have the [StackTraceHidden] attribute that most throw helpers have.

I don't think we have a rule to slap that attribute on the throw helpers. in Release, stack-traces get cut due to inlining and tail-calls heavily, so we'd better not hide frames which may help identify the actual source of the exception

[xtqqczze]

I don't think we have a rule to slap that attribute on the throw helpers. in Release, stack-traces get cut due to inlining and tail-calls heavily, so we'd better not hide frames which may help identify the actual source of the exception

runtime/src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs

Lines 53 to 54 in 768bb6d

	[StackTraceHidden]
	internal static class ThrowHelper

[EgorBo]

I don't think we have a rule to slap that attribute on the throw helpers. in Release, stack-traces get cut due to inlining and tail-calls heavily, so we'd better not hide frames which may help identify the actual source of the exception

runtime/src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs

Lines 53 to 54 in 768bb6d

	[StackTraceHidden]
	internal static class ThrowHelper

Yes, I'm aware that we already have it

[prozolic]

Upon closer inspection, the regressions shown in MihuBot/runtime-utils#1584 (comment) appear to be caused by the indexer used in ImmutableArray<T>.Builder.GetEnumerator()

This is a bit different from the main proposal, but what if we made ImmutableArray<T>.Builder.GetEnumerator() return a dedicated enumerator struct like the following? Do you think that could help improve this?

• before

public IEnumerator<T> GetEnumerator()
{
    for (int i = 0; i < this.Count; i++)
    {
        yield return this[i];
    }
}

• after

public IEnumerator<T> GetEnumerator()
{
    return new Enumerator(this);
}

private struct Enumerator : IEnumerator<T>, IEnumerator
{
    private readonly Builder _builder;

    private int _index;
    private T? _current;

    internal Enumerator(Builder builder)
    {
        _builder = builder;
    }

    public void Dispose()
    { }

    public bool MoveNext()
    {
        Builder builder = _builder;

        if ((uint)_index < (uint)builder._count)
        {
            _current = builder._elements[_index];
            _index++;
            return true;
        }

        _current = default;
        _index = -1;
        return false;
    }

    public T Current => _current!;

    object? IEnumerator.Current => this.Current;

    void IEnumerator.Reset()
    {
        _index = 0;
        _current = default;
    }
}