Console.OpenStandard* few times slower on Linux

[adamsitnik]

Console.OpenStandard* methods are few times slower on Linux.

Slower	diff/base	Base Median (ns)	Diff Median (ns)	Modality
System.ConsoleTests.Perf_Console.OpenStandardInput	10.50	175.08	1838.35
System.ConsoleTests.Perf_Console.OpenStandardError	4.25	434.18	1846.05
System.ConsoleTests.Perf_Console.OpenStandardOutput	4.21	437.84	1843.03

How to run the benchmarks:

git clone https://github.com/dotnet/performance.git
python3 ./performance/scripts/benchmarks_ci.py -f netcoreapp5.0 --filter 'System.ConsoleTests.Perf_Console.OpenStandard*'

Recommended profilers are PerfCollect and VTune.

[adamsitnik]

Quick look at the profile:

private static void Main(string[] args)
{
    for (int i = 0; i < 10_000_000; i++)
    {
        using (var output = Console.OpenStandardOutput())
        {
        };    
    }
}

20% of time is spent in call to fstat:

https://github.com/dotnet/corefx/blob/ee038925bde85c1c45c52f235ab09ea03b9c126c/src/System.Console/src/System/ConsolePal.Unix.cs#L1427-L1430

Which sets _handleType which looks like is not used anymore (it might be a low hanging fruit)

[tmds]

Which sets _handleType which looks like is not used anymore (it might be a low hanging fruit)

@adamsitnik this issue can be closed. _handleType was removed in dotnet/corefx#42400.

[adamsitnik]

@tmds When I created this issue I was hoping that somebody is going to perform a full investigation and bring the performance on par with Windows. We have solved only the low hanging fruit (20%) and the difference is still very big. That's why I would perfer to keep it open.

[tmds]

Digging a bit deeper.

The highest cost comes from the syscalls involved:

Windows makes 1 for open, and Dispose doesn't make any:

runtime/src/libraries/System.Console/src/System/ConsolePal.Windows.cs

Lines 1104 to 1113 in 886e442

	protected override void Dispose(bool disposing)
	{
	// We're probably better off not closing the OS handle here. First,
	// we allow a program to get multiple instances of ConsoleStreams
	// around the same OS handle, so closing one handle would invalidate
	// them all. Additionally, we want a second AppDomain to be able to
	// write to stdout if a second AppDomain quits.
	_handle = IntPtr.Zero;
	base.Dispose(disposing);
	}

Unix makes 1 for open, and Dispose makes 2:

runtime/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs

Lines 110 to 126 in 886e442

	protected override bool ReleaseHandle()
	{
	// When the SafeFileHandle was opened, we likely issued an flock on the created descriptor in order to add
	// an advisory lock. This lock should be removed via closing the file descriptor, but close can be
	// interrupted, and we don't retry closes. As such, we could end up leaving the file locked,
	// which could prevent subsequent usage of the file until this process dies. To avoid that, we proactively
	// try to release the lock before we close the handle. (If it's not locked, there's no behavioral
	// problem trying to unlock it.)
	Interop.Sys.FLock(handle, Interop.Sys.LockOperations.LOCK_UN); // ignore any errors
	// Close the descriptor. Although close is documented to potentially fail with EINTR, we never want
	// to retry, as the descriptor could actually have been closed, been subsequently reassigned, and
	// be in use elsewhere in the process. Instead, we simply check whether the call was successful.
	int result = Interop.Sys.Close(handle);
	Debug.Assert(result == 0, $"Close failed with result {result} and error {Interop.Sys.GetLastErrorInfo()}");
	return result == 0;
	}

The flock could be eliminated. It would require specializing the Handle type. I don't think it is worth it because I expect this won't be called on perf critical paths. For best performance, the method should be called once, and the returned Stream should be kept around.

[tmds]

@adamsitnik is the above analysis good enough to close this issue?

[adamsitnik]

is the above analysis good enough to close this issue?

It is! Thank you @tmds!