`Stream.ReadByte` should use `stackalloc byte[1]` instead allocate one byte array

[DrkWzrd]

Description

In advance sorry for my english and sorry if this "is not a bug".

Right now in System.IO.Stream.ReadByte the code allocates an 1-length array for return the only byte read done in Read(byte[], int, int)

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs

Lines 697 to 702 in c9d8f80

	public virtual int ReadByte()
	{
	var oneByteArray = new byte[1];
	int r = Read(oneByteArray, 0, 1);
	return r == 0 ? -1 : oneByteArray[0];
	}

Reproduction Steps

Just that.

Expected behavior

public virtual int ReadByte()
{
    var oneByteArray = (stackalloc byte[1]);
    int r = Read(oneByteArray);
    return r == 0 ? -1 : oneByteArray[0];
}

Actual behavior

Actually a call to ReadByte can produce unnecesary GCs (and some child classes and developers that are unaware of this use it a lot).

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

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

Issue Details

---

Description

In advance sorry for my english and sorry if this "is not a bug".

Right now in System.IO.Stream.ReadByte the code allocates an 1-length array for return the only byte read done in Read(byte[], int, int)

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs

Lines 697 to 702 in c9d8f80

	public virtual int ReadByte()
	{
	var oneByteArray = new byte[1];
	int r = Read(oneByteArray, 0, 1);
	return r == 0 ? -1 : oneByteArray[0];
	}

Reproduction Steps

Just that.

Expected behavior

public virtual int ReadByte()
        {
            var oneByteArray = (stackalloc byte[1]);
            int r = Read(oneByteArray);
            return r == 0 ? -1 : oneByteArray[0];
        }

Actual behavior

Actually a call to ReadByte can produce unnecesary GCs (and some child classes and developers that are unaware of this use it a lot).

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

Author:	DrkWzrd
Assignees:	-
Labels:	area-System.IO, untriaged
Milestone:	-

[alexrp]

This has been brought up a number of times before. This would be a breaking change because we'd now be calling the Span<byte> overload instead of the byte[] overload. This matters for derived classes that may not be expecting this.

I don't know if there's any appetite for taking that breaking change considering the fix is relatively easy for derived classes that care about the allocation.

[DrkWzrd]

@alexrp

I think this should be revised, the performance impact is measurable, And this could be solved with something like.

 public virtual int ReadByte() 
 { 
     var oneByteArray = (stackalloc byte[1]); 
     int r = ReadByteSpan(oneByteArray); 
     return r == 0 ? -1 : oneByteArray[0]; 
 } 

private int ReadByteSpan(Span<byte> buffer)
{
    byte[] sharedBuffer = ArrayPool<byte>.Shared.Rent(buffer.Length);
    try
    {
        int numRead = Read(sharedBuffer, 0, buffer.Length);
        if ((uint)numRead > (uint)buffer.Length)
        {
            throw new IOException(SR.IO_StreamTooLong);
        }

        new ReadOnlySpan<byte>(sharedBuffer, 0, numRead).CopyTo(buffer);
        return numRead;
    }
    finally
    {
        ArrayPool<byte>.Shared.Return(sharedBuffer);
    }
}

I know this is "too much" but the performance in this way should be measured.

[teo-tsirpanis]

performance impact is measurable

Stream.ReadByte should not be used by high-performance code. Regardless of optimizations in its implementation, using it means calling a virtual method for every byte. It is recommended to use a Read overload and read many bytes at once (I've done something similar in my code and saw a significant performance improvement). And keep in mind that this is the default implementation of Stream.ReadByte. Derived classes can override it to make it allocation-free, and in fact most if not all streams in the BCL do that.

[stephentoub]

And this could be solved with something like

The overhead of renting/returning a 1-byte buffer as compared to allocating a byte[1] is sizeable:

Method	Mean
RentReturn	15.503 ns
Alloc	2.511 ns

[Benchmark]
public byte[] RentReturn()
{
    byte[] buffer = ArrayPool<byte>.Shared.Rent(1);
    ArrayPool<byte>.Shared.Return(buffer);
    return buffer; // just to mirror the same benchmark signature as for Alloc()
}

[Benchmark]
public byte[] Alloc()
{
    byte[] buffer = new byte[1];
    return buffer;
}

As @teo-tsirpanis mentioned, this has been discussed and rejected multiple times in the past.

[jozkee]

Duplicate of #60587.