C# backend generates unoptimized enumerator pattern

[Therzok]

If you look at the generated IEnumerable code, it generates a plain yield return enumerator. That means there's an allocation on every GetEnumerator call - which is normally avoided by providing a concrete StructEnumerator GetEnumerator() overload. The code seems to do a lot of redundant work (exception is checked on the indexer hot path, so indexer will never be inlined).

Would it make a bit more sense to return a ReadOnlySpan<T> view over the data? That would also give a bit of flexibility to the caller at no performance expense. That would also remove the inheritance information from the struct,s.

[ralfbiedert]

Thanks for the suggestion. Maybe I misunderstand, but would you want to return a ReadOnlySpan<T> from safe code? We already return one if you enable unsafe bindings via Config::use_unsafe.

About StructEnumerator, I haven't thought about it so much, but I'd be open to it. Could you sketch how exactly that would look like for one slice type? If this needs utility classes or structs (which StructEnumerator seems to be), I'd also prefer if they could be generic, or at least be nested in the parent type to avoid clutter.

[Therzok]

Thanks for the suggestion. Maybe I misunderstand, but would you want to return a ReadOnlySpan from safe code? We already return one if you enable unsafe bindings via Config::use_unsafe.

Thanks for the response! I was mostly shooting for use a ReadOnlySpan<T> implementation for the indexer internally. That would avoid having to use Marshalling machinery and written down bounds checks.

class SliceUseAsciiStringPattern : IEnumerable<UseAsciiStringPattern>
{
        public IEnumerator<UseAsciiStringPattern> GetEnumerator()
        {
            for (var i = 0; i < (int)len; ++i)
            {
                yield return this[i];
            }
        }
        IEnumerator IEnumerable.GetEnumerator()
        {
            return this.GetEnumerator();
        }
}

The above would be turned into something like the below, and it's likely not everything is needed, I just adapted the implementation of dotnet's List.cs.

class SliceUseAsciiStringPattern : IEnumerable<UseAsciiStringPattern>
{
        // Writing this, I wonder if it's easier to just `new ReadOnlySpan<T>(this.data.ToPointer(), (int)this.len).GetEnumerator()`
        public Enumerator GetEnumerator() =>
            new Enumerator(this);
        IEnumerator<UseAsciiStringPattern> IEnumerable<UseAsciiStringPattern>.GetEnumerator() =>
            new Enumerator(this);
        IEnumerator IEnumerable.GetEnumerator() =>
            new Enumerator(this);

        public struct Enumerator : IEnumerator<UseAsciiStringPattern>, IEnumerator
        {
            private readonly SliceUseAsciiStringPattern _list;
            private int _index;
            private UseAsciiStringPattern? _current;

            internal Enumerator(SliceUseAsciiStringPattern list)
            {
                _list = list;
                _index = 0;
                _current = default;
            }

            public void Dispose()
            {
            }

            public bool MoveNext()
            {
                var localList = _list;
                if ((uint)_index < (uint)localList.len)
                {
                    _current = localList[_index];
                    _index++;
                    return true;
                }

                _index = localList.len + 1;
                _current = default;
                return false;
            }

            public UseAsciiStringPattern Current => _current!;

            object? IEnumerator.Current => _current;

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

I also think the Enumerator implementation can be deduplicated if an IReadOnlyList<T> constraint is satisfied, as that gives us an indexer implementation.

[ralfbiedert]

The above would be turned into something like the below

Ah, yes, I'm generally open to that. I don't have so much time to test this right now, but I'd accept PRs.

The only question / caveat I have, is this really faster than the KISS version existing right now? You'd have to pay for the bounds check anyway as you just move that cost to MoveNext (and I'd unscientifically argue that most people only access elements once anyway when iterating). Then again, removing allocation sources alone might be worth it.

// Writing this, I wonder if it's easier to just new ReadOnlySpan<T>(this.data.ToPointer(), (int)this.len).GetEnumerator()

Doesn't this need unsafe?

In any case, checklist for PRs implementing this

• Implement code gen in C# backend
• Have some (out of project) benchmark showing this makes a difference (perf or mem), attach results here or in PR
• Add appropriate C# unit test in safe or unsafe backend