Movatterモバイル変換

carlossanlop added this to the6.0.0 milestone

carlossanlop requested review fromadamsitnik,jeffhandley,jozkee andstephentoub

April 6, 2021 20:17

carlossanlop self-assigned this

@adamsitnik @jozkee @jeffhandley @stephentoub

Copy link

ghost commentedApr 6, 2021

Tagging subscribers to this area:@carlossanlop
See info inarea-owners.md if you want to be subscribed.

Issue Details

The behavior was largely left untouched, except for the following differences:

Net5CompatFileStreamStrategy.Windows will be the only strategy that consumes theFileStreamCompletionSource, so it is now a private nested class.
One single new type implements bothIValueTaskSource (used for write) andIValueTaskSource<int> (used for read).
The logic inside the callback and the cancellation token registration is the same, aside for the code that needs to report exceptions or return values.
We had interop errors redefined in multiple places.
The constants used byFileStreamCompletionSource were moved toFileStreamHelpers so they could be reused by the new type.

Unit tests

Benchmarks

Ran theFileStream performance benchmarks against .NET 5.0, then against the code of this PR's baseline commit, and then against this PR's new code. I compared the results, and there are subtle improvements in some benchmark tests:

.NET 5.0 vs .NET 6.0 before this PR

dotnet run --project D:\performance\src\tools\ResultsComparer\ResultsComparer.csproj -c release --base 'D:\fs50' --diff 'D:\fs60_before_pr' --threshold 0.0001%

summary:
better: 39, geomean: 2.801
worse: 22, geomean: 2.098
total diff: 61

Slower	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Perf_FileStream.CopyToFile(fileSize: 1048576, options: None)	4.93	882252.57	4348913.28
Perf_FileStream.CopyToFileAsync(fileSize: 1048576, options: None)	4.62	1003671.88	4633945.31	bimodal
Perf_FileStream.WriteByte(fileSize: 1024, options: None)	3.13	257218.23	805205.31	several?
Perf_FileStream.WriteByte(fileSize: 1024, options: Asynchronous)	3.13	286859.38	896524.31
Perf_FileStream.Write(fileSize: 1048576, userBufferSize: 4096, options: None)	3.06	1795459.38	5498006.25
Perf_FileStream.CopyToFileAsync(fileSize: 1048576, options: Asynchronous)	3.05	1868294.53	5689532.29
Perf_FileStream.Write(fileSize: 1024, userBufferSize: 512, options: None)	2.99	254148.29	758897.83
Perf_FileStream.WriteAsync(fileSize: 1024, userBufferSize: 512, options: Asynchronous)	2.97	283860.42	843052.43
Perf_FileStream.Write(fileSize: 1048576, userBufferSize: 512, options: None)	2.90	`1844293`.36	`5353701`.04	several?
Perf_FileStream.WriteAsync(fileSize: 1048576, userBufferSize: 4096, options: None)	2.89	2209384.38	`6395554`.17	bimodal
Perf_FileStream.WriteAsync(fileSize: 1024, userBufferSize: 512, options: None)	2.65	301619.83	798302.63
Perf_FileStream.CopyToFileAsync(fileSize: 1024, options: Asynchronous)	1.86	557062.05	1035575.21	bimodal
Perf_FileStream.CopyToFile(fileSize: 1024, options: None)	1.81	478821.50	868866.12
Perf_FileStream.FlushAsync(fileSize: 1024, options: None)	1.69	`3479778`.75	`5891160`.42	several?
Perf_FileStream.CopyToFileAsync(fileSize: 1024, options: None)	1.66	543512.61	903388.82	bimodal
Perf_FileStream.WriteAsync(fileSize: 104857600, userBufferSize: 4096, options: None)	1.40	168291900.00	235314600.00
Perf_FileStream.WriteAsync(fileSize: 1048576, userBufferSize: 512, options: Asynchronous)	1.24	`8197915`.63	10200393.75
Perf_FileStream.WriteAsync(fileSize: 1048576, userBufferSize: 512, options: None)	1.21	5313143.75	6444323.96
Perf_FileStream.Flush(fileSize: 1024, options: None)	1.18	3448111.25	4052132.81
Perf_FileStream.CopyToFile(fileSize: 104857600, options: None)	1.11	50055150.00	55473900.00
Perf_FileStream.CopyToFileAsync(fileSize: 104857600, options: None)	1.09	56033900.00	61052537.50
Perf_FileStream.Write(fileSize: 104857600, userBufferSize: 4096, options: None)	1.03	133329050.00	137489800.00

Faster	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Perf_FileStream.SeekBackward(fileSize: 1024, options: Asynchronous)	115.64	5489541.67	47470.79
Perf_FileStream.SeekForward(fileSize: 1024, options: Asynchronous)	64.90	2811281.25	43319.93
Perf_FileStream.SeekBackward(fileSize: 1024, options: None)	57.82	`2571736`.46	44479.08
Perf_FileStream.SeekForward(fileSize: 1024, options: None)	21.29	876923.26	41197.72
Perf_FileStream.WriteAsync(fileSize: 104857600, userBufferSize: 4096, options: Asynchronous)	5.06	2463834750.00	486578800.00
Perf_FileStream.OpenClose(fileSize: 1024, options: None)	4.94	152781.66	30909.09
Perf_FileStream.OpenClose(fileSize: 1024, options: Asynchronous)	4.72	153731.81	32593.67
Perf_FileStream.Read(fileSize: 1024, userBufferSize: 4096, options: None)	4.27	161781.84	37889.31	several?
Perf_FileStream.ReadByte(fileSize: 1024, options: None)	3.98	165696.68	41632.18
Perf_FileStream.Read(fileSize: 1024, userBufferSize: 512, options: None)	3.97	159526.58	40152.10
Perf_FileStream.ReadByte(fileSize: 1024, options: Asynchronous)	3.50	200423.36	57204.05
Perf_FileStream.LockUnlock(fileSize: 1024, options: None)	2.59	230670.38	89181.93
Perf_FileStream.LockUnlock(fileSize: 1024, options: Asynchronous)	2.55	238920.22	93543.27
Perf_FileStream.ReadAsync(fileSize: 1024, userBufferSize: 4096, options: None)	2.50	193629.08	77357.59
Perf_FileStream.ReadAsync(fileSize: 1048576, userBufferSize: 512, options: None)	2.44	3222202.50	1318286.88
Perf_FileStream.ReadAsync(fileSize: 1024, userBufferSize: 512, options: None)	2.39	194808.87	81591.76
Perf_FileStream.ReadAsync(fileSize: 1024, userBufferSize: 4096, options: Asynchronous)	2.36	216916.98	92096.58
Perf_FileStream.ReadAsync(fileSize: 104857600, userBufferSize: 512, options: None)	2.24	325116900.00	145245400.00
Perf_FileStream.ReadAsync(fileSize: 1048576, userBufferSize: 4096, options: Asynchronous)	2.15	`7025697`.92	`3271982`.50
Perf_FileStream.ReadAsync(fileSize: 1024, userBufferSize: 512, options: Asynchronous)	2.10	199647.89	95019.35
Perf_FileStream.ReadAsync(fileSize: 104857600, userBufferSize: 4096, options: Asynchronous)	2.04	686776300.00	336991400.00	bimodal
Perf_FileStream.WriteAsync(fileSize: 1048576, userBufferSize: 4096, options: Asynchronous)	1.95	19598723.08	10074393.75
Perf_FileStream.WriteAsync(fileSize: 104857600, userBufferSize: 512, options: None)	1.85	508393800.00	274360000.00
Perf_FileStream.WriteAsync(fileSize: 104857600, userBufferSize: 512, options: Asynchronous)	1.74	796591200.00	457934500.00	bimodal
Perf_FileStream.CopyToFileAsync(fileSize: 104857600, options: Asynchronous)	1.73	156593600.00	90313262.50
Perf_FileStream.Flush(fileSize: 1024, options: Asynchronous)	1.70	22492768.75	13248333.33
Perf_FileStream.ReadAsync(fileSize: 104857600, userBufferSize: 512, options: Asynchronous)	1.64	542213800.00	331399500.00
Perf_FileStream.ReadAsync(fileSize: 1048576, userBufferSize: 512, options: Asynchronous)	1.61	5221529.17	`3244615`.63
Perf_FileStream.FlushAsync(fileSize: 1024, options: Asynchronous)	1.50	21953590.91	14593046.88
Perf_FileStream.Read(fileSize: 1048576, userBufferSize: 4096, options: None)	1.33	987460.55	742610.27
Perf_FileStream.Read(fileSize: 1048576, userBufferSize: 512, options: None)	1.25	975146.09	777023.21
Perf_FileStream.ReadAsync(fileSize: 1048576, userBufferSize: 4096, options: None)	1.19	1301925.00	1096436.88
Perf_FileStream.WriteAsync(fileSize: 1024, userBufferSize: 4096, options: None)	1.12	142000.45	126451.42
Perf_FileStream.Write(fileSize: 104857600, userBufferSize: 512, options: None)	1.11	162435300.00	146191400.00
Perf_FileStream.WriteAsync(fileSize: 1024, userBufferSize: 4096, options: Asynchronous)	1.10	141476.75	128484.00
Perf_FileStream.Write(fileSize: 1024, userBufferSize: 4096, options: None)	1.10	142083.81	129048.19
Perf_FileStream.ReadAsync(fileSize: 104857600, userBufferSize: 4096, options: None)	1.03	116486775.00	113456750.00
Perf_FileStream.Read(fileSize: 104857600, userBufferSize: 4096, options: None)	1.02	88348175.00	86894587.50
Perf_FileStream.Read(fileSize: 104857600, userBufferSize: 512, options: None)	1.01	92483275.00	91193650.00

.NET 5.0 vs .NET 6.0 after this PR

Slower	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Perf_FileStream.CopyToFile(fileSize: 1048576, options: None)	4.92	882252.57	4341552.34
Perf_FileStream.CopyToFileAsync(fileSize: 1048576, options: None)	4.60	1003671.88	4615128.91	bimodal
Perf_FileStream.WriteAsync(fileSize: 1024, userBufferSize: 512, options: Asynchronous)	3.24	283860.42	919970.59
Perf_FileStream.Write(fileSize: 1024, userBufferSize: 512, options: None)	3.08	254148.29	783231.25
Perf_FileStream.CopyToFileAsync(fileSize: 1048576, options: Asynchronous)	3.06	1868294.53	5710579.17
Perf_FileStream.WriteByte(fileSize: 1024, options: Asynchronous)	3.05	286859.38	874430.38
Perf_FileStream.WriteByte(fileSize: 1024, options: None)	3.04	257218.23	780960.42
Perf_FileStream.Write(fileSize: 1048576, userBufferSize: 4096, options: None)	2.97	1795459.38	5329393.75
Perf_FileStream.Write(fileSize: 1048576, userBufferSize: 512, options: None)	2.95	`1844293`.36	5432295.83	several?
Perf_FileStream.WriteAsync(fileSize: 1048576, userBufferSize: 4096, options: None)	2.92	2209384.38	6445677.08
Perf_FileStream.WriteAsync(fileSize: 1024, userBufferSize: 512, options: None)	2.52	301619.83	758761.88
Perf_FileStream.CopyToFileAsync(fileSize: 1024, options: Asynchronous)	1.95	557062.05	1087235.83
Perf_FileStream.CopyToFileAsync(fileSize: 1024, options: None)	1.73	543512.61	937893.38	bimodal
Perf_FileStream.CopyToFile(fileSize: 1024, options: None)	1.66	478821.50	793440.46	bimodal
Perf_FileStream.FlushAsync(fileSize: 1024, options: None)	1.47	`3479778`.75	5113488.54	several?
Perf_FileStream.WriteAsync(fileSize: 104857600, userBufferSize: 4096, options: None)	1.32	168291900.00	221660000.00
Perf_FileStream.WriteAsync(fileSize: 1048576, userBufferSize: 512, options: None)	1.23	5313143.75	6512252.08
Perf_FileStream.WriteAsync(fileSize: 1048576, userBufferSize: 512, options: Asynchronous)	1.22	`8197915`.63	10012295.65
Perf_FileStream.Flush(fileSize: 1024, options: None)	1.21	3448111.25	4163660.16
Perf_FileStream.CopyToFileAsync(fileSize: 104857600, options: None)	1.13	56033900.00	63237650.00
Perf_FileStream.CopyToFile(fileSize: 104857600, options: None)	1.09	50055150.00	54423625.00
Perf_FileStream.Read(fileSize: 104857600, userBufferSize: 4096, options: None)	1.01	88348175.00	88983100.00

Faster	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Perf_FileStream.SeekBackward(fileSize: 1024, options: Asynchronous)	115.50	5489541.67	47526.62
Perf_FileStream.SeekForward(fileSize: 1024, options: Asynchronous)	64.94	2811281.25	43292.23
Perf_FileStream.SeekBackward(fileSize: 1024, options: None)	57.06	`2571736`.46	45070.47
Perf_FileStream.SeekForward(fileSize: 1024, options: None)	21.05	876923.26	41661.14
Perf_FileStream.WriteAsync(fileSize: 104857600, userBufferSize: 4096, options: Asynchronous)	5.36	2463834750.00	459292800.00
Perf_FileStream.OpenClose(fileSize: 1024, options: None)	4.95	152781.66	30850.74
Perf_FileStream.OpenClose(fileSize: 1024, options: Asynchronous)	4.75	153731.81	32349.21
Perf_FileStream.Read(fileSize: 1024, userBufferSize: 4096, options: None)	4.27	161781.84	37926.07	several?
Perf_FileStream.Read(fileSize: 1024, userBufferSize: 512, options: None)	3.98	159526.58	40096.31
Perf_FileStream.ReadByte(fileSize: 1024, options: None)	3.96	165696.68	41824.65
Perf_FileStream.LockUnlock(fileSize: 1024, options: None)	2.59	230670.38	89051.28
Perf_FileStream.LockUnlock(fileSize: 1024, options: Asynchronous)	2.56	238920.22	93276.60
Perf_FileStream.ReadAsync(fileSize: 1024, userBufferSize: 4096, options: None)	2.53	193629.08	76383.11
Perf_FileStream.ReadAsync(fileSize: 1024, userBufferSize: 512, options: None)	2.45	194808.87	79416.04
Perf_FileStream.ReadAsync(fileSize: 1048576, userBufferSize: 512, options: None)	2.34	3222202.50	`1376330`.63
Perf_FileStream.ReadAsync(fileSize: 1024, userBufferSize: 4096, options: Asynchronous)	2.31	216916.98	93711.06
Perf_FileStream.ReadAsync(fileSize: 104857600, userBufferSize: 512, options: None)	2.28	325116900.00	142880200.00
Perf_FileStream.ReadByte(fileSize: 1024, options: Asynchronous)	2.22	200423.36	90245.25
Perf_FileStream.ReadAsync(fileSize: 1048576, userBufferSize: 4096, options: Asynchronous)	2.16	`7025697`.92	3250746.88
Perf_FileStream.WriteAsync(fileSize: 104857600, userBufferSize: 512, options: None)	2.11	508393800.00	241032100.00
Perf_FileStream.ReadAsync(fileSize: 1024, userBufferSize: 512, options: Asynchronous)	2.11	199647.89	94675.64
Perf_FileStream.ReadAsync(fileSize: 104857600, userBufferSize: 4096, options: Asynchronous)	2.06	686776300.00	333108950.00	bimodal
Perf_FileStream.WriteAsync(fileSize: 1048576, userBufferSize: 4096, options: Asynchronous)	1.93	19598723.08	10171781.82
Perf_FileStream.CopyToFileAsync(fileSize: 104857600, options: Asynchronous)	1.74	156593600.00	90035450.00
Perf_FileStream.ReadAsync(fileSize: 104857600, userBufferSize: 512, options: Asynchronous)	1.65	542213800.00	329569650.00
Perf_FileStream.ReadAsync(fileSize: 1048576, userBufferSize: 512, options: Asynchronous)	1.64	5221529.17	3182177.50
Perf_FileStream.WriteAsync(fileSize: 104857600, userBufferSize: 512, options: Asynchronous)	1.64	796591200.00	486630000.00
Perf_FileStream.Flush(fileSize: 1024, options: Asynchronous)	1.52	22492768.75	14797453.85
Perf_FileStream.FlushAsync(fileSize: 1024, options: Asynchronous)	1.50	21953590.91	14625460.00
Perf_FileStream.Read(fileSize: 1048576, userBufferSize: 4096, options: None)	1.33	987460.55	742460.71
Perf_FileStream.Read(fileSize: 1048576, userBufferSize: 512, options: None)	1.25	975146.09	781337.50
Perf_FileStream.ReadAsync(fileSize: 1048576, userBufferSize: 4096, options: None)	1.20	1301925.00	1085495.19
Perf_FileStream.WriteAsync(fileSize: 1024, userBufferSize: 4096, options: Asynchronous)	1.11	141476.75	127158.59
Perf_FileStream.Write(fileSize: 1024, userBufferSize: 4096, options: None)	1.11	142083.81	128134.40
Perf_FileStream.WriteAsync(fileSize: 1024, userBufferSize: 4096, options: None)	1.10	142000.45	129068.15
Perf_FileStream.ReadAsync(fileSize: 104857600, userBufferSize: 4096, options: None)	1.03	116486775.00	112942550.00

Profiling

Ireused the same code we have in our benchmarks, but I executed it inside aconsole app thatforces the consumption of the compiled runtime code.

You'll notice the topTask allocation went away, and theFileStreamCompletionSource allocations were substituted byFileStreamValueTaskSource allocations.

`ReadAsync(bufferSize: 1, userBufferSize: FourKibibytes, FileOptions.Asynchronous)`

Allocations before this PR

Type	Allocations
-System.Threading.Tasks.Task<>	64,001
-System.IO.MemoryFileStreamCompletionSource	64,000
- System.Threading.ThreadPoolBoundHandleOverlapped	64,000
- System.Threading.OverlappedData	64,000
- System.SByte[]	2,797
- System.String	2,153
- System.Object[]	952
- System.Reflection.RuntimeParameterInfo	650
- System.Byte[]	539
- System.Diagnostics.Tracing.ScalarTypeInfo	489
- System.Func<,,>	485
- System.Reflection.ParameterInfo[]	281
- Microsoft.Win32.SafeHandles.SafeFileHandle	252
- System.IO.FileStream	252
- System.Runtime.CompilerServices.AsyncTaskMethodBuilder<>.AsyncStateMachineBox<>	251
- System.IO.AsyncWindowsFileStreamStrategy	250

Allocations after this PR

Type	Allocations
-System.IO.Strategies.AsyncWindowsFileStreamStrategy.MemoryFileStreamValueTaskSource	64,000
- System.Threading.ThreadPoolBoundHandleOverlapped	64,000
- System.Threading.OverlappedData	64,000
- System.SByte[]	2,803
- System.String	2,144
- System.Object[]	952
- System.Reflection.RuntimeParameterInfo	650
- System.Byte[]	539
- System.Reflection.ParameterInfo[]	281
- Microsoft.Win32.SafeHandles.SafeFileHandle	252
- System.IO.FileStream	252
- System.Runtime.CompilerServices.AsyncTaskMethodBuilder<>.AsyncStateMachineBox<>	251
- System.IO.Strategies.AsyncWindowsFileStreamStrategy	250
- System.Threading.ThreadPoolBoundHandle	250
- System.Reflection.RuntimeMethodInfo	241
- System.Signature	225

`ReadAsync(bufferSize: 1, userBufferSize: HalfKibibyte, FileOptions.Asynchronous)`

Allocations before this PR

Type	Allocations
-System.Threading.Tasks.Task<>	512,001
-System.IO.MemoryFileStreamCompletionSource	512,000
- System.Threading.ThreadPoolBoundHandleOverlapped	512,000
- System.Threading.OverlappedData	512,000
- System.SByte[]	2,806
- System.String	2,153
- System.Object[]	957
- System.Reflection.RuntimeParameterInfo	650
- System.Byte[]	539
- System.Diagnostics.Tracing.ScalarTypeInfo	489
- System.Func<,,>	485
- System.Reflection.ParameterInfo[]	281
- Microsoft.Win32.SafeHandles.SafeFileHandle	252
- System.IO.FileStream	252
- System.Runtime.CompilerServices.AsyncTaskMethodBuilder<>.AsyncStateMachineBox<>	251
- System.IO.AsyncWindowsFileStreamStrategy	250
- System.Threading.ThreadPoolBoundHandle	250

Allocations after this PR

Type	Allocations
-System.IO.Strategies.AsyncWindowsFileStreamStrategy.MemoryFileStreamValueTaskSource	512,000
- System.Threading.ThreadPoolBoundHandleOverlapped	512,000
- System.Threading.OverlappedData	512,000
- System.SByte[]	2,803
- System.String	2,144
- System.Object[]	964
- System.Reflection.RuntimeParameterInfo	650
- System.Byte[]	539
- System.Reflection.ParameterInfo[]	281
- Microsoft.Win32.SafeHandles.SafeFileHandle	252
- System.IO.FileStream	252
- System.Runtime.CompilerServices.AsyncTaskMethodBuilder<>.AsyncStateMachineBox<>	251
- System.IO.Strategies.AsyncWindowsFileStreamStrategy	250
- System.Threading.ThreadPoolBoundHandle	250
- System.Reflection.RuntimeMethodInfo	241
- System.Signature	225
- System.RuntimeType[]	178

`ReadAsync(bufferSize: FourKibibytes, userBufferSize: FourKibibytes, FileOptions.Asynchronous)`

Allocations before this PR

Type	Allocations
-System.Threading.Tasks.Task<>	64,013
-System.IO.MemoryFileStreamCompletionSource	64,000
- System.Threading.ThreadPoolBoundHandleOverlapped	64,000
- System.Threading.OverlappedData	64,000
- System.SByte[]	2,840
- System.String	2,153
- System.Object[]	955
- System.Reflection.RuntimeParameterInfo	650
- System.Byte[]	539
- System.Diagnostics.Tracing.ScalarTypeInfo	489
- System.Func<,,>	485
- System.Reflection.ParameterInfo[]	281
- System.IO.BufferedFileStreamStrategy	252
- Microsoft.Win32.SafeHandles.SafeFileHandle	252
- System.IO.FileStream	252
- System.Runtime.CompilerServices.AsyncTaskMethodBuilder<>.AsyncStateMachineBox<>	251
- System.IO.AsyncWindowsFileStreamStrategy	250
- System.Threading.SemaphoreSlim	250
- System.Threading.ThreadPoolBoundHandle	250

Allocations after this PR

Type	Allocations
-System.IO.Strategies.AsyncWindowsFileStreamStrategy.MemoryFileStreamValueTaskSource	64,000
- System.Threading.ThreadPoolBoundHandleOverlapped	64,000
- System.Threading.OverlappedData	64,000
- System.SByte[]	2,836
- System.String	2,144
- System.Object[]	957
- System.Reflection.RuntimeParameterInfo	650
- System.Byte[]	539
- System.Reflection.ParameterInfo[]	281
- System.IO.Strategies.BufferedFileStreamStrategy	252
- Microsoft.Win32.SafeHandles.SafeFileHandle	252
- System.IO.FileStream	252
- System.Runtime.CompilerServices.AsyncTaskMethodBuilder<>.AsyncStateMachineBox<>	251
- System.IO.Strategies.AsyncWindowsFileStreamStrategy	250
- System.Threading.SemaphoreSlim	250
- System.Threading.ThreadPoolBoundHandle	250
- System.Runtime.CompilerServices.StrongBox<>	250
- System.Reflection.RuntimeMethodInfo	241
- System.Signature	225

`ReadAsync(bufferSize: FourKibibytes, userBufferSize: HalfKibibyte, FileOptions.Asynchronous)`

Allocations before this PR

Type	Allocations
- System.Runtime.CompilerServices.AsyncTaskMethodBuilder<>.AsyncStateMachineBox<>	64,235
-System.Threading.Tasks.Task<>	64,015
-System.IO.FileStreamCompletionSource	64,000
- System.SByte[]	2,916
- System.String	2,155
- System.Object[]	956
- System.Byte[]	789
- System.Reflection.RuntimeParameterInfo	650
- System.Diagnostics.Tracing.ScalarTypeInfo	489
- System.Func<,,>	485
- System.Reflection.ParameterInfo[]	281
- System.IO.BufferedFileStreamStrategy	252
- Microsoft.Win32.SafeHandles.SafeFileHandle	252
- System.IO.FileStream	252
- System.IO.AsyncWindowsFileStreamStrategy	250
- System.Threading.ThreadPoolBoundHandleOverlapped	250
- System.Threading.OverlappedData	250
- System.Threading.SemaphoreSlim	250
- System.Threading.ThreadPoolBoundHandle	250
- System.Threading.PreAllocatedOverlapped	250

Allocations after this PR

Type	Allocations
-System.IO.Strategies.AsyncWindowsFileStreamStrategy.FileStreamValueTaskSource	64,000
- System.Runtime.CompilerServices.AsyncTaskMethodBuilder<>.AsyncStateMachineBox<>	63,812
- System.SByte[]	2,909
- System.String	2,144
- System.Object[]	964
- System.Byte[]	789
- System.Reflection.RuntimeParameterInfo	650
- System.Reflection.ParameterInfo[]	281
- System.IO.Strategies.BufferedFileStreamStrategy	252
- Microsoft.Win32.SafeHandles.SafeFileHandle	252
- System.IO.FileStream	252
- System.IO.Strategies.AsyncWindowsFileStreamStrategy	250
- System.Threading.ThreadPoolBoundHandleOverlapped	250
- System.Threading.OverlappedData	250
- System.Threading.SemaphoreSlim	250
- System.Threading.ThreadPoolBoundHandle	250
- System.Threading.PreAllocatedOverlapped	250
- System.Runtime.CompilerServices.StrongBox<>	250
- System.Reflection.RuntimeMethodInfo	241
- System.Signature	225

`WriteAsync(bufferSize: 1, userBufferSize: FourKibibytes, FileOptions.Asynchronous)`

Allocations before this PR

Type	Allocations
-System.Threading.Tasks.Task<>	64,001
-System.IO.MemoryFileStreamCompletionSource	64,000
- System.Threading.ThreadPoolBoundHandleOverlapped	64,000
- System.Threading.OverlappedData	64,000
- System.SByte[]	2,798
- System.String	2,153
- System.Object[]	954
- System.Reflection.RuntimeParameterInfo	650
- System.Byte[]	539
- System.Diagnostics.Tracing.ScalarTypeInfo	489
- System.Func<,,>	485
- System.Reflection.ParameterInfo[]	281
- Microsoft.Win32.SafeHandles.SafeFileHandle	252
- System.IO.FileStream	252
- System.Runtime.CompilerServices.AsyncTaskMethodBuilder<>.AsyncStateMachineBox<>	251
- System.IO.AsyncWindowsFileStreamStrategy	250
- System.Threading.ThreadPoolBoundHandle	250

Allocations after this PR

Type	Allocations
-System.IO.Strategies.AsyncWindowsFileStreamStrategy.MemoryFileStreamValueTaskSource	64,000
- System.Threading.ThreadPoolBoundHandleOverlapped	64,000
- System.Threading.OverlappedData	64,000
- System.SByte[]	2,790
- System.String	2,144
- System.Object[]	959
- System.Reflection.RuntimeParameterInfo	650
- System.Byte[]	539
- System.Reflection.ParameterInfo[]	281
- Microsoft.Win32.SafeHandles.SafeFileHandle	252
- System.IO.FileStream	252
- System.Runtime.CompilerServices.AsyncTaskMethodBuilder<>.AsyncStateMachineBox<>	251
- System.IO.Strategies.AsyncWindowsFileStreamStrategy	250
- System.Threading.ThreadPoolBoundHandle	250
- System.Reflection.RuntimeMethodInfo	241
- System.Signature	225
- System.RuntimeType[]	178

`WriteAsync(bufferSize: 1, userBufferSize: HalfKibibyte, FileOptions.Asynchronous)`

Allocations before this PR

Type	Allocations
-System.Threading.Tasks.Task<>	512,001
-System.IO.MemoryFileStreamCompletionSource	512,000
- System.Threading.ThreadPoolBoundHandleOverlapped	512,000
- System.Threading.OverlappedData	512,000
- System.SByte[]	2,812
- System.String	2,153
- System.Object[]	959
- System.Reflection.RuntimeParameterInfo	650
- System.Byte[]	539
- System.Diagnostics.Tracing.ScalarTypeInfo	489
- System.Func<,,>	485
- System.Reflection.ParameterInfo[]	281
- Microsoft.Win32.SafeHandles.SafeFileHandle	252
- System.IO.FileStream	252
- System.Runtime.CompilerServices.AsyncTaskMethodBuilder<>.AsyncStateMachineBox<>	251
- System.IO.AsyncWindowsFileStreamStrategy	250
- System.Threading.ThreadPoolBoundHandle	250

Allocations after this PR

Type	Allocations
-System.IO.Strategies.AsyncWindowsFileStreamStrategy.MemoryFileStreamValueTaskSource	512,000
- System.Threading.ThreadPoolBoundHandleOverlapped	512,000
- System.Threading.OverlappedData	512,000
- System.SByte[]	2,790
- System.String	2,144
- System.Object[]	958
- System.Reflection.RuntimeParameterInfo	650
- System.Byte[]	539
- System.Reflection.ParameterInfo[]	281
- Microsoft.Win32.SafeHandles.SafeFileHandle	252
- System.IO.FileStream	252
- System.Runtime.CompilerServices.AsyncTaskMethodBuilder<>.AsyncStateMachineBox<>	251
- System.IO.Strategies.AsyncWindowsFileStreamStrategy	250
- System.Threading.ThreadPoolBoundHandle	250
- System.Reflection.RuntimeMethodInfo	241
- System.Signature	225
- System.RuntimeType[]	178

`WriteAsync(bufferSize: FourKibibytes, userBufferSize: FourKibibytes, FileOptions.Asynchronous)`

Allocations before this PR

Type	Allocations
-System.Threading.Tasks.Task<>	64,014
- System.Threading.ThreadPoolBoundHandleOverlapped	32,250
- System.Threading.OverlappedData	32,250
- System.Runtime.CompilerServices.AsyncTaskMethodBuilder<>.AsyncStateMachineBox<>	32,249
-System.IO.MemoryFileStreamCompletionSource	32,000
-System.IO.FileStreamCompletionSource	32,000
- System.SByte[]	2,915
- System.String	2,155
- System.Object[]	952
- System.Byte[]	789
- System.Reflection.RuntimeParameterInfo	650
- System.Diagnostics.Tracing.ScalarTypeInfo	489
- System.Func<,,>	485
- System.Reflection.ParameterInfo[]	281
- System.IO.BufferedFileStreamStrategy	252
- Microsoft.Win32.SafeHandles.SafeFileHandle	252
- System.IO.FileStream	252
- System.IO.AsyncWindowsFileStreamStrategy	250
- System.Threading.SemaphoreSlim	250
- System.Threading.ThreadPoolBoundHandle	250
- System.Threading.PreAllocatedOverlapped	250

Allocations after this PR

Type	Allocations
- System.Threading.ThreadPoolBoundHandleOverlapped	32,250
- System.Threading.OverlappedData	32,250
- System.Runtime.CompilerServices.AsyncTaskMethodBuilder<>.AsyncStateMachineBox<>	32,246
-System.IO.Strategies.AsyncWindowsFileStreamStrategy.MemoryFileStreamValueTaskSource	32,000
-System.IO.Strategies.AsyncWindowsFileStreamStrategy.FileStreamValueTaskSource	32,000
- System.SByte[]	2,893
- System.String	2,144
- System.Object[]	954
- System.Byte[]	789
- System.Reflection.RuntimeParameterInfo	650
- System.Reflection.ParameterInfo[]	281
- System.IO.Strategies.BufferedFileStreamStrategy	252
- Microsoft.Win32.SafeHandles.SafeFileHandle	252
- System.IO.FileStream	252
- System.IO.Strategies.AsyncWindowsFileStreamStrategy	250
- System.Threading.SemaphoreSlim	250
- System.Threading.ThreadPoolBoundHandle	250
- System.Threading.PreAllocatedOverlapped	250
- System.Runtime.CompilerServices.StrongBox<>	250
- System.Reflection.RuntimeMethodInfo	241
- System.Signature	225

`WriteAsync(bufferSize: FourKibibytes, userBufferSize: HalfKibibyte, FileOptions.Asynchronous)`

Allocations before this PR

Type	Allocations
-System.Threading.Tasks.Task<>	64,014
-System.IO.FileStreamCompletionSource	64,000
- System.Runtime.CompilerServices.AsyncTaskMethodBuilder<>.AsyncStateMachineBox<>	63,963
- System.SByte[]	2,915
- System.String	2,155
- System.Object[]	956
- System.Byte[]	789
- System.Reflection.RuntimeParameterInfo	650
- System.Diagnostics.Tracing.ScalarTypeInfo	489
- System.Func<,,>	485
- System.Reflection.ParameterInfo[]	281
- System.IO.BufferedFileStreamStrategy	252
- Microsoft.Win32.SafeHandles.SafeFileHandle	252
- System.IO.FileStream	252
- System.IO.AsyncWindowsFileStreamStrategy	250
- System.Threading.ThreadPoolBoundHandleOverlapped	250
- System.Threading.OverlappedData	250
- System.Threading.SemaphoreSlim	250
- System.Threading.ThreadPoolBoundHandle	250
- System.Threading.PreAllocatedOverlapped	250

Allocations after this PR

Type	Allocations
-System.IO.Strategies.AsyncWindowsFileStreamStrategy.FileStreamValueTaskSource	64,000
- System.Runtime.CompilerServices.AsyncTaskMethodBuilder<>.AsyncStateMachineBox<>	63,830
- System.SByte[]	2,912
- System.String	2,144
- System.Object[]	966
- System.Byte[]	789
- System.Reflection.RuntimeParameterInfo	650
- System.Reflection.ParameterInfo[]	281
- System.IO.Strategies.BufferedFileStreamStrategy	252
- Microsoft.Win32.SafeHandles.SafeFileHandle	252
- System.IO.FileStream	252
- System.IO.Strategies.AsyncWindowsFileStreamStrategy	250
- System.Threading.ThreadPoolBoundHandleOverlapped	250
- System.Threading.OverlappedData	250
- System.Threading.SemaphoreSlim	250
- System.Threading.ThreadPoolBoundHandle	250
- System.Threading.PreAllocatedOverlapped	250
- System.Runtime.CompilerServices.StrongBox<>	250
- System.Threading.Tasks.ValueTask.ValueTaskSourceAsTask	249
- System.Threading.QueueUserWorkItemCallbackDefaultContext<>	249

Also: Don't forget to ignore whitespace changes so it's easier to review the nested type's changes 😃

Author:	carlossanlop
Assignees:	carlossanlop
Labels:	`area-System.IO`
Milestone:	6.0.0

Copy link

ContributorAuthor

carlossanlop commentedApr 6, 2021

Tagging subscribers to this area:@carlossanlop

Yes, thank you bot... 😆

jozkee reviewed

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs OutdatedShow resolvedHide resolved

jozkee reviewed

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs

Copy link

Member

jozkee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Otherwise; LGTM, will approve if/when comments are resolved, most of them are nits and/or questions.

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs Outdated

Comment on lines 17 to 22

		internalconstlongNoResult=0;
		internalconstlongResultSuccess=(long)1<<32;
		internalconstlongResultError=(long)2<<32;
		internalconstlongRegisteringCancellation=(long)4<<32;
		internalconstlongCompletedCallback=(long)8<<32;
		internalconstulongResultMask=((ulong)uint.MaxValue)<<32;

Copy link

Member

jozkeeApr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Consider prefixing this constants withAsyncCompletionSource_ or something alike since these names are too ambiguous or keep them in their respective classes regardless of them being duplicated.

Copy link

Member

adamsitnikApr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

or put them into a nested static class calledReturnCodes:

internalstaticpartialclassFileStreamHelpers{internalstaticclassReturnCodes{internalconstlongNoResult=0;internalconstlongResultSuccess=(long)1<<32;internalconstlongResultError=(long)2<<32;internalconstlongRegisteringCancellation=(long)4<<32;internalconstlongCompletedCallback=(long)8<<32;internalconstulongResultMask=((ulong)uint.MaxValue)<<32;}

but it's a nit (not a blocker)

...ibraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.csShow resolvedHide resolved

Comment on lines 66 to 67

		_strategy.CompareExchangeCurrentOverlappedOwner(this,null)==null?
		_strategy._fileHandle.ThreadPoolBinding!.AllocateNativeOverlapped(preallocatedOverlapped!):// allocated when buffer was created, and buffer is non-null

Copy link

Member

jozkeeApr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change

	_strategy.CompareExchangeCurrentOverlappedOwner(this,null)==null?
	_strategy._fileHandle.ThreadPoolBinding!.AllocateNativeOverlapped(preallocatedOverlapped!):// allocated when buffer was created, and buffer is non-null
	strategy.CompareExchangeCurrentOverlappedOwner(this,null)==null?
	strategy._fileHandle.ThreadPoolBinding!.AllocateNativeOverlapped(preallocatedOverlapped!):// allocated when buffer was created, and buffer is non-null

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs OutdatedShow resolvedHide resolved

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs Outdated

		// be directly the AwaitableProvider that's completing (in the case where the preallocated
		// overlapped was already in use by another operation).
		object?state=ThreadPoolBoundHandle.GetNativeOverlappedState(pOverlapped);
		Debug.Assert(stateis(AsyncWindowsFileStreamStrategy orFileStreamValueTaskSource));

Copy link

Member

jozkeeApr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change

	Debug.Assert(stateis(AsyncWindowsFileStreamStrategy orFileStreamValueTaskSource));
	Debug.Assert(stateisAsyncWindowsFileStreamStrategy orFileStreamValueTaskSource);

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs Outdated

		interrorCode=unchecked((int)(packedResult&uint.MaxValue));
		if(errorCode==Interop.Errors.ERROR_OPERATION_ABORTED)
		{
		_source.SetException(ExceptionDispatchInfo.SetCurrentStackTrace(newOperationCanceledException(cancellationToken.IsCancellationRequested?cancellationToken:newCancellationToken(canceled:true))));

Copy link

Member

jozkeeApr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change

	_source.SetException(ExceptionDispatchInfo.SetCurrentStackTrace(newOperationCanceledException(cancellationToken.IsCancellationRequested?cancellationToken:newCancellationToken(canceled:true))));
	Exceptione=newOperationCanceledException(cancellationToken.IsCancellationRequested?cancellationToken:newCancellationToken(canceled:true));
	e.SetCurrentStackTrace();
	_source.SetException(e);

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs

		}

		privatevoidCancel(CancellationTokentoken)
		{

Copy link

Member

jozkeeApr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Missing comment and asserts:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs

Lines 211 to 215 in57bbdee

	// WARNING: This may potentially be called under a lock (during cancellation registration)

	Debug.Assert(stateisFileStreamCompletionSource,"Unknown state passed to cancellation");
	FileStreamCompletionSourcecompletionSource=(FileStreamCompletionSource)state;
	Debug.Assert(completionSource._overlapped!=null&&!completionSource.Task.IsCompleted,"IO should not have completed yet");

Copy link

ContributorAuthor

carlossanlopApr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The first assert is not needed because the method argument is not anobject? state.

I'll add the assert that checks for overlapped an completed status.

Copy link

Member

jozkeeApr 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thestate param could be added to this method and allows you to conserve the first Debug.Assert.

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs Outdated

		longpackedResult=Interlocked.CompareExchange(ref_result,FileStreamHelpers.RegisteringCancellation,FileStreamHelpers.NoResult);
		if(packedResult==FileStreamHelpers.NoResult)
		{
		_cancellationRegistration=cancellationToken.UnsafeRegister(static(s,token)=>((FileStreamValueTaskSource)s!).Cancel(token),this);

Copy link

Member

jozkeeApr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Could we pass the callback on a similar fashion to FileStreamCompletionSource

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs

Line 92 in57bbdee

_cancellationRegistration=cancellationToken.UnsafeRegister(cancelCallback,this);

Or why did you opted for this way instead?

Copy link

ContributorAuthor

carlossanlopApr 8, 2021•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It made more sense to passthis as the second argument ofUnsafeRegister, which ensures it's aValueTaskSource instance, and saves us from the check of theobject? state parameter like in the old code. The old codealso allocates anAction<object?> instance to save a reference to theCancel method.

We have asimilar example in System.Threading.Channels.

Copy link

Member

jozkeeApr 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It made more sense to pass this as the second argument of UnsafeRegister

FileStreamCompletionSource was also passingthis as 2nd argument.

The old code also allocates an Action<object?> instance to save a reference to the Cancel method.

Yes, seems that it was caching the method group to avoid allocating everytime it was passed toUnsafeRegister.
However I think you can avoid the allocation without caching yourself by doing this:

_cancellationRegistration=cancellationToken.UnsafeRegister((s,token)=>Cancel(s,token),this);

Just a suggestion, not something that you should block on.

...ries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.Windows.csShow resolvedHide resolved

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs OutdatedShow resolvedHide resolved

Copy link

Member

davidfowl commentedApr 7, 2021

This is the change I was waiting for 👏🏾👏🏾

Copy link

Member

davidfowl commentedApr 7, 2021

I haven't looked but I assume we'll reuse the operation objects if there's no concurrency? This is what sockets do today so in the 95% case of no overlapping writes or reads, there'll be a single reusable allocation on the FileStream

Copy link

Member

stephentoub commentedApr 7, 2021

This is the change I was waiting for

It's not, actually. You want the next one where the IValueTaskSource instance is cached.

stephentoub reviewed

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs

...ibraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.csShow resolvedHide resolved

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs OutdatedShow resolvedHide resolved

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs OutdatedShow resolvedHide resolved

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.csShow resolvedHide resolved

		}
		// else: Some other thread stole the result, so now it is responsible to finish the callback
		}
		// else: Some other thread is registering a cancellation, so it must finish the callback

Copy link

Member

stephentoubApr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

All this logic looks more complicated than it needs to be. I realize you're just following the same flow as what was already there, so nothing needs to be changed in this PR. But we should revisit all of this synchronization later; I suspect we can do it more succinctly and efficiently.

Copy link

ContributorAuthor

carlossanlopApr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I opened an issue to follow up on this and on the caching of the IValueTaskSource.
#50972

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs OutdatedShow resolvedHide resolved

adamsitnik reviewed

Copy link

Member

adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Overall looks good to me, but before we merge I would like to see the following benchmark results:

.NET 6 with new strategy enabled: before and after your change. (not .NET 5 vs 6 as there are too many moving targets)

You should be able to achieve that by running the following command in the perf repo;

git clone https://github.com/dotnet/performance.gitcd performance\src\benchmarks\microdotnet run -c Release -f net6.0 --filter *Perf_FileStream* --coreRun $pathToCoreRunWithoutYourChanges $pathToCoreRunWithYourChanges --envVars"DOTNET_SYSTEM_IO_USENET5COMPATFILESTREAM:0"

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs OutdatedShow resolvedHide resolved

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.csShow resolvedHide resolved

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.csShow resolvedHide resolved

...ries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.Windows.csShow resolvedHide resolved

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs Outdated

		/// <see cref="MemoryHandle"/> when the operation has completed. This should only be used
		/// when memory doesn't wrap a byte[].
		/// </summary>
		privatesealedclassMemoryAwaitableProvider:FileStreamValueTaskSource

Copy link

Member

adamsitnikApr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This should only be used when memory doesn't wrap a byte[].

I just realized that we don't have benchmarks for this scenario asMemory used in our micro-benchmarks always wraps a byte array. Could you please add such benchmarks tohttps://github.com/dotnet/performance/blob/main/src/benchmarks/micro/libraries/System.IO.FileSystem/Perf.FileStream.cs ? One test case should be enough as this should be rare.

Copy link

ContributorAuthor

carlossanlopApr 7, 2021•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Some of the benchmarks do consume this type. If you look at some of the profiling tests I executed, you'll see thatMemoryValueTaskSource shows up a few times. The profiling tests reuse the code we had in our benchmarks.

I don't mind adding an explicit one though. Dou you want me to add one anyway?

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs Outdated

Comment on lines 17 to 22

		internalconstlongNoResult=0;
		internalconstlongResultSuccess=(long)1<<32;
		internalconstlongResultError=(long)2<<32;
		internalconstlongRegisteringCancellation=(long)4<<32;
		internalconstlongCompletedCallback=(long)8<<32;
		internalconstulongResultMask=((ulong)uint.MaxValue)<<32;

Copy link

Member

adamsitnikApr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

or put them into a nested static class calledReturnCodes:

internalstaticpartialclassFileStreamHelpers{internalstaticclassReturnCodes{internalconstlongNoResult=0;internalconstlongResultSuccess=(long)1<<32;internalconstlongResultError=(long)2<<32;internalconstlongRegisteringCancellation=(long)4<<32;internalconstlongCompletedCallback=(long)8<<32;internalconstulongResultMask=((ulong)uint.MaxValue)<<32;}

but it's a nit (not a blocker)

carlossanlop commented

src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems OutdatedShow resolvedHide resolved

runfoappbot mentioned this pull request

Microsoft.Extensions.Hosting.Tests.HostTests.CreateDefaultBuilder_SecretsDoesReload fails on Linux#50751

Closed

carlossanlop mentioned this pull request

Apr 8, 2021

Cache IValueTaskSource in AsyncWindowsFileStreamStrategy#50972

Closed

jozkee reviewed

Apr 9, 2021

...m.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.ValueTaskSource.cs OutdatedShow resolvedHide resolved

...m.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.ValueTaskSource.cs Outdated

Comment on lines 62 to 63

Copy link

Member

jozkeeApr 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Did you run the perf. benchmarks after doing this change and did you notice any impact?

Copy link

ContributorAuthor

carlossanlopApr 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Pending.

Copy link

ContributorAuthor

carlossanlopApr 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Confirmed - There is a slight improvement.

I made sure to compare apples to apples: I used my latest commit, and built it settingRunContinuationsAsynchronously totrue, then tofalse. I compared it both times against our code in .NET 5.0.

The improvement is clear when setting it tofalse (or not setting it at all).

RunContinuationsAsynchronously = true

❯ dotnet run -c release --base "D:\fs50" --diff "D:\fs60_NEW_ON_IVTS_asynctrue" --threshold 0.0001%
summary:
better: 24, geomean: 3.402
worse: 19, geomean: 2.244
total diff: 43

Slower	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Perf_FileStream.CopyToFile(fileSize: 1048576, options: None)	5.49	882252.57	4840275.00
Perf_FileStream.CopyToFileAsync(fileSize: 1048576, options: None)	5.12	1003671.88	`5136350`.00	bimodal
Perf_FileStream.WriteByte(fileSize: 1024, options: None)	4.44	257218.23	1141827.01
Perf_FileStream.WriteByte(fileSize: 1024, options: Asynchronous)	4.08	286859.38	1171600.96
Perf_FileStream.Write(fileSize: 1048576, userBufferSize: 4096, options: None)	3.25	1795459.38	5832233.33
Perf_FileStream.Write(fileSize: 1048576, userBufferSize: 512, options: None)	3.11	`1844293`.36	5741847.92	several?
Perf_FileStream.CopyToFileAsync(fileSize: 1048576, options: Asynchronous)	3.11	1868294.53	5807825.00
Perf_FileStream.WriteAsync(fileSize: 1048576, userBufferSize: 4096, options: None)	3.08	2209384.38	6796443.75
Perf_FileStream.CopyToFileAsync(fileSize: 1024, options: Asynchronous)	2.46	557062.05	`1371566`.15
Perf_FileStream.CopyToFile(fileSize: 1024, options: None)	2.33	478821.50	1116033.48
Perf_FileStream.CopyToFileAsync(fileSize: 1024, options: None)	2.28	543512.61	1239956.25	bimodal
Perf_FileStream.FlushAsync(fileSize: 1024, options: None)	1.61	`3479778`.75	5612725.00
Perf_FileStream.WriteAsync(fileSize: 1048576, userBufferSize: 512, options: Asynchronous)	1.44	`8197915`.63	11814575.00
Perf_FileStream.WriteAsync(fileSize: 104857600, userBufferSize: 4096, options: None)	1.33	168291900.00	224355000.00
Perf_FileStream.WriteAsync(fileSize: 1048576, userBufferSize: 512, options: None)	1.33	5313143.75	7063218.75
Perf_FileStream.Flush(fileSize: 1024, options: None)	1.25	3448111.25	4313832.81
Perf_FileStream.CopyToFile(fileSize: 104857600, options: None)	1.12	50055150.00	56309025.00
Perf_FileStream.CopyToFileAsync(fileSize: 104857600, options: None)	1.12	56033900.00	62803900.00
Perf_FileStream.Write(fileSize: 104857600, userBufferSize: 4096, options: None)	1.12	133329050.00	148833000.00	bimodal

Faster	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Perf_FileStream.SeekBackward(fileSize: 1024, options: Asynchronous)	117.07	5489541.67	46889.31
Perf_FileStream.SeekForward(fileSize: 1024, options: Asynchronous)	65.93	2811281.25	42642.74
Perf_FileStream.SeekBackward(fileSize: 1024, options: None)	57.84	`2571736`.46	44462.45
Perf_FileStream.SeekForward(fileSize: 1024, options: None)	21.09	876923.26	41586.04
Perf_FileStream.OpenClose(fileSize: 1024, options: None)	5.00	152781.66	30548.62
Perf_FileStream.OpenClose(fileSize: 1024, options: Asynchronous)	4.76	153731.81	32314.04
Perf_FileStream.WriteAsync(fileSize: 104857600, userBufferSize: 4096, options: Asynchronous)	4.03	2463834750.00	611183400.00
Perf_FileStream.ReadByte(fileSize: 1024, options: None)	3.93	165696.68	42208.34
Perf_FileStream.ReadByte(fileSize: 1024, options: Asynchronous)	3.52	200423.36	56865.52
Perf_FileStream.LockUnlock(fileSize: 1024, options: None)	2.62	230670.38	87990.73
Perf_FileStream.LockUnlock(fileSize: 1024, options: Asynchronous)	2.58	238920.22	92478.58
Perf_FileStream.ReadAsync(fileSize: 1048576, userBufferSize: 512, options: None)	2.34	3222202.50	1377939.84
Perf_FileStream.CopyToFileAsync(fileSize: 104857600, options: Asynchronous)	1.87	156593600.00	83681962.50
Perf_FileStream.WriteAsync(fileSize: 1048576, userBufferSize: 4096, options: Asynchronous)	1.66	19598723.08	11803641.67
Perf_FileStream.Flush(fileSize: 1024, options: Asynchronous)	1.63	22492768.75	13818900.00
Perf_FileStream.ReadAsync(fileSize: 1048576, userBufferSize: 4096, options: Asynchronous)	1.57	`7025697`.92	4487121.88
Perf_FileStream.ReadAsync(fileSize: 104857600, userBufferSize: 4096, options: Asynchronous)	1.47	686776300.00	466936500.00	bimodal
Perf_FileStream.Read(fileSize: 1048576, userBufferSize: 4096, options: None)	1.33	987460.55	743825.45
Perf_FileStream.Read(fileSize: 1048576, userBufferSize: 512, options: None)	1.24	975146.09	784156.88
Perf_FileStream.ReadAsync(fileSize: 1048576, userBufferSize: 512, options: Asynchronous)	1.19	5221529.17	`4387085`.94
Perf_FileStream.ReadAsync(fileSize: 1048576, userBufferSize: 4096, options: None)	1.18	1301925.00	1103803.79
Perf_FileStream.ReadAsync(fileSize: 104857600, userBufferSize: 4096, options: None)	1.04	116486775.00	112407200.00
Perf_FileStream.FlushAsync(fileSize: 1024, options: Asynchronous)	1.03	21953590.91	21241122.73
Perf_FileStream.Read(fileSize: 104857600, userBufferSize: 4096, options: None)	1.01	88348175.00	87467100.00

RunContinuationsAsynchronously = false

dotnet run -c release --base "D:\fs50" --diff "D:\fs60_NEW_ON_IVTS_asyncfalse" --threshold 0.0001%
summary:
better: 23, geomean: 3.592
worse: 20, geomean: 2.135
total diff: 43

Slower	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Perf_FileStream.CopyToFile(fileSize: 1048576, options: None)	5.36	882252.57	`4730875`.00
Perf_FileStream.CopyToFileAsync(fileSize: 1048576, options: None)	5.05	1003671.88	`5071912`.50	bimodal
Perf_FileStream.WriteByte(fileSize: 1024, options: None)	4.25	257218.23	1094164.38	several?
Perf_FileStream.WriteByte(fileSize: 1024, options: Asynchronous)	4.05	286859.38	1161160.27
Perf_FileStream.Write(fileSize: 1048576, userBufferSize: 4096, options: None)	3.23	1795459.38	5791481.25
Perf_FileStream.Write(fileSize: 1048576, userBufferSize: 512, options: None)	3.10	`1844293`.36	5709820.83	several?
Perf_FileStream.WriteAsync(fileSize: 1048576, userBufferSize: 4096, options: None)	3.09	2209384.38	6836504.17
Perf_FileStream.CopyToFileAsync(fileSize: 1048576, options: Asynchronous)	3.07	1868294.53	5733883.33
Perf_FileStream.CopyToFileAsync(fileSize: 1024, options: Asynchronous)	2.46	557062.05	1368034.90
Perf_FileStream.CopyToFile(fileSize: 1024, options: None)	2.31	478821.50	1103696.25
Perf_FileStream.CopyToFileAsync(fileSize: 1024, options: None)	2.24	543512.61	1218234.38	bimodal
Perf_FileStream.FlushAsync(fileSize: 1024, options: None)	1.60	`3479778`.75	5572682.29
Perf_FileStream.WriteAsync(fileSize: 1048576, userBufferSize: 512, options: Asynchronous)	1.42	`8197915`.63	11634850.00
Perf_FileStream.WriteAsync(fileSize: 104857600, userBufferSize: 4096, options: None)	1.33	168291900.00	223141800.00	bimodal
Perf_FileStream.WriteAsync(fileSize: 1048576, userBufferSize: 512, options: None)	1.31	5313143.75	6949831.25
Perf_FileStream.Flush(fileSize: 1024, options: None)	1.26	3448111.25	4332915.63
Perf_FileStream.Write(fileSize: 104857600, userBufferSize: 4096, options: None)	1.13	133329050.00	150223800.00	bimodal
Perf_FileStream.CopyToFile(fileSize: 104857600, options: None)	1.11	50055150.00	55699050.00
Perf_FileStream.CopyToFileAsync(fileSize: 104857600, options: None)	1.10	56033900.00	61710550.00
Perf_FileStream.Read(fileSize: 104857600, userBufferSize: 4096, options: None)	1.00	88348175.00	88759625.00

Faster	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Perf_FileStream.SeekBackward(fileSize: 1024, options: Asynchronous)	116.97	5489541.67	46929.42
Perf_FileStream.SeekForward(fileSize: 1024, options: Asynchronous)	65.76	2811281.25	42753.29
Perf_FileStream.SeekBackward(fileSize: 1024, options: None)	57.56	`2571736`.46	44676.15
Perf_FileStream.SeekForward(fileSize: 1024, options: None)	21.52	876923.26	40756.47
Perf_FileStream.OpenClose(fileSize: 1024, options: None)	5.06	152781.66	30216.63
Perf_FileStream.OpenClose(fileSize: 1024, options: Asynchronous)	4.81	153731.81	31975.57
Perf_FileStream.WriteAsync(fileSize: 104857600, userBufferSize: 4096, options: Asynchronous)	4.05	2463834750.00	608864800.00
Perf_FileStream.ReadByte(fileSize: 1024, options: None)	3.99	165696.68	41526.73
Perf_FileStream.ReadByte(fileSize: 1024, options: Asynchronous)	3.54	200423.36	56610.08
Perf_FileStream.LockUnlock(fileSize: 1024, options: None)	2.62	230670.38	87877.10
Perf_FileStream.LockUnlock(fileSize: 1024, options: Asynchronous)	2.59	238920.22	92291.75
Perf_FileStream.ReadAsync(fileSize: 1048576, userBufferSize: 512, options: None)	2.40	3222202.50	`1345116`.25
Perf_FileStream.CopyToFileAsync(fileSize: 104857600, options: Asynchronous)	1.86	156593600.00	83969875.00
Perf_FileStream.WriteAsync(fileSize: 1048576, userBufferSize: 4096, options: Asynchronous)	1.64	19598723.08	11925173.68
Perf_FileStream.Flush(fileSize: 1024, options: Asynchronous)	1.62	22492768.75	13859764.71
Perf_FileStream.ReadAsync(fileSize: 1048576, userBufferSize: 4096, options: Asynchronous)	1.56	`7025697`.92	4493881.25
Perf_FileStream.ReadAsync(fileSize: 104857600, userBufferSize: 4096, options: Asynchronous)	1.46	686776300.00	470158300.00	bimodal
Perf_FileStream.Read(fileSize: 1048576, userBufferSize: 4096, options: None)	1.32	987460.55	750294.64
Perf_FileStream.Read(fileSize: 1048576, userBufferSize: 512, options: None)	1.24	975146.09	784056.56
Perf_FileStream.ReadAsync(fileSize: 1048576, userBufferSize: 4096, options: None)	1.17	1301925.00	`1108880`.36
Perf_FileStream.ReadAsync(fileSize: 1048576, userBufferSize: 512, options: Asynchronous)	1.17	5221529.17	4470673.44
Perf_FileStream.FlushAsync(fileSize: 1024, options: Asynchronous)	1.07	21953590.91	20580725.00
Perf_FileStream.ReadAsync(fileSize: 104857600, userBufferSize: 4096, options: None)	1.01	116486775.00

Copy link

ContributorAuthor

carlossanlopApr 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Here is the result comparison if we compare the benchmark results of setting it totrue vsfalse:

base: RunContinuationsAsynchronously=true, diff: RunContinuationsAsynchronously=false

dotnet run -c release --base "D:\fs60_NEW_ON_IVTS_asynctrue" --diff "D:\fs60_NEW_ON_IVTS_asyncfalse" --threshold 0.0001%
summary:
better: 14, geomean: 1.018
worse: 11, geomean: 1.017
total diff: 25

Slower	diff/base	Base Median (ns)	Diff Median (ns)
Perf_FileStream.WriteAsync_NoBuffering(fileSize: 104857600, userBufferSize: 16384, options: None)	1.05	67143550.00	70259187.50
Perf_FileStream.ReadAsync(fileSize: 104857600, userBufferSize: 4096, options: None)	1.02	112407200.00	115107550.00
Perf_FileStream.Read_NoBuffering(fileSize: 1048576, userBufferSize: 16384, options: None)	1.02	240016.67	244921.93
Perf_FileStream.ReadAsync(fileSize: 1048576, userBufferSize: 512, options: Asynchronous)	1.02	`4387085`.94	4470673.44
Perf_FileStream.Read(fileSize: 104857600, userBufferSize: 4096, options: None)	1.01	87467100.00	88759625.00
Perf_FileStream.WriteAsync_NoBuffering(fileSize: 1048576, userBufferSize: 16384, options: None)	1.01	21684354.55	21982809.09
Perf_FileStream.ReadAsync_NoBuffering(fileSize: 1048576, userBufferSize: 16384, options: Asynchronous)	1.01	1236709.90	1253242.19
Perf_FileStream.Read(fileSize: 1024, userBufferSize: 1024, options: None)	1.01	39289.54	39730.73
Perf_FileStream.Write_NoBuffering(fileSize: 1048576, userBufferSize: 16384, options: None)	1.01	21445427.27	21646650.00
Perf_FileStream.Read(fileSize: 1048576, userBufferSize: 4096, options: None)	1.01	743825.45	750294.64
Perf_FileStream.SeekBackward(fileSize: 1024, options: None)	1.00	44462.45	44676.15

Faster	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Perf_FileStream.WriteByte(fileSize: 1024, options: None)	1.04	1141827.01	1094164.38	several?
Perf_FileStream.ReadAsync_NoBuffering(fileSize: 104857600, userBufferSize: 16384, options: None)	1.03	45688612.50	44474575.00
Perf_FileStream.ReadAsync(fileSize: 1048576, userBufferSize: 512, options: None)	1.02	1377939.84	`1345116`.25
Perf_FileStream.CopyToFile(fileSize: 1048576, options: None)	1.02	4840275.00	`4730875`.00
Perf_FileStream.SeekForward(fileSize: 1024, options: None)	1.02	41586.04	40756.47
Perf_FileStream.ReadAsync_NoBuffering(fileSize: 104857600, userBufferSize: 16384, options: Asynchronous)	1.02	130849050.00	128463775.00
Perf_FileStream.ReadByte(fileSize: 1024, options: None)	1.02	42208.34	41526.73
Perf_FileStream.WriteAsync(fileSize: 1048576, userBufferSize: 512, options: Asynchronous)	1.02	11814575.00	11634850.00
Perf_FileStream.CopyToFileAsync(fileSize: 1048576, options: Asynchronous)	1.01	5807825.00	5733883.33
Perf_FileStream.CopyToFileAsync(fileSize: 1048576, options: None)	1.01	`5136350`.00	`5071912`.50
Perf_FileStream.CopyToFile(fileSize: 104857600, options: None)	1.01	56309025.00	55699050.00
Perf_FileStream.Read_NoBuffering(fileSize: 104857600, userBufferSize: 16384, options: None)	1.01	33747814.29	`3338345`.14
Perf_FileStream.OpenClose(fileSize: 1024, options: Asynchronous)	1.01	32314.04	31975.57
Perf_FileStream.ReadAsync(fileSize: 1024, userBufferSize: 1024, options: Asynchronous)	1.00	81988.09	81641.78

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs Outdated

		longpackedResult=Interlocked.CompareExchange(ref_result,FileStreamHelpers.RegisteringCancellation,FileStreamHelpers.NoResult);
		if(packedResult==FileStreamHelpers.NoResult)
		{
		_cancellationRegistration=cancellationToken.UnsafeRegister(static(s,token)=>((FileStreamValueTaskSource)s!).Cancel(token),this);

Copy link

Member

jozkeeApr 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It made more sense to pass this as the second argument of UnsafeRegister

FileStreamCompletionSource was also passingthis as 2nd argument.

The old code also allocates an Action<object?> instance to save a reference to the Cancel method.

Yes, seems that it was caching the method group to avoid allocating everytime it was passed toUnsafeRegister.
However I think you can avoid the allocation without caching yourself by doing this:

_cancellationRegistration=cancellationToken.UnsafeRegister((s,token)=>Cancel(s,token),this);

Just a suggestion, not something that you should block on.

jozkee reviewed

Apr 9, 2021

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs Outdated

Comment on lines 120 to 122

Copy link

Member

jozkeeApr 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Is it possible to apply this same pattern invoid Write?
https://github.com/dotnet/runtime/blob/9bebe1f4b5ab5ec63ff537140d89a166a44d1d7f/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs#L238-L239

Copy link

ContributorAuthor

carlossanlopApr 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I tried, but aValueTask does not have aResult property, like aValueTask<T> does, because it returns void. So I'm not sure if it's possible to apply an equivalent pattern.

Copy link

Member

stephentoubApr 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

publicoverridevoidWrite(byte[]buffer,intoffset,intcount){ValueTaskvt=WriteAsyncInternal(newReadOnlyMemory<byte>(buffer,offset,count),CancellationToken.None);if(vt.IsCompleted){vt.GetAwaiter().GetResult();}else{vt.AsTask().GetAwaiter().GetResult();}}

That said, it's not much of an issue for this case. The read side is an issue because AsTask() will almost certainly need to allocate a newTask<int> in order to hand back the result, but for the write side, AsTask() on an already completed ValueTask will just return Task.Completed, assuming the operation completed successfully. So the only time the existing code for the write side will allocate is when the operation was canceled or faulted, in which case we're about to throw an exception anyway, and the extra task allocation won't really matter. Up to you whether you want to change it.

jozkee approved these changes

Apr 10, 2021

Use ValueTask-returning async FileStream methods wherever possible#51050

Copy link

Member

jozkee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM; thanks Carlos!

adamsitnik mentioned this pull request

Apr 10, 2021

Closed

Copy link

Member

adamsitnik commentedApr 12, 2021

I compared it both times against our code in .NET 5.0.

Comparison against 5.0 is a comparison of all of the changes that were merged to main since we have snapped 5.0 with your change against 5.0. What we care about is how your change (and nothing else) is affecting the perf, so we need to isolate the runs to only your change.

To be able to do that you need to run the benchmarks using two CoreRuns:

as a baseline the corerun that was built from main when you have created a new branch, without any of your changes
your branch with your changes

My personal workflow:

When I create a new branch, I perform a full build inRelease:

build -c Release -subset clr+libs

I copy the folder that containsCoreRun.exe (runtime\artifacts\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0) to a separate folder. Typically I call it "before".
I apply some changes and perform an incremental Release build. In case ofSystem.Private.CoreLib.dll it's:

build -c Release -subset clr.corelib+clr.nativecorelib+libs.PreTest

I run the benchmarks using the old and newCoreRun.exe:

dotnet run -c Release -f net6.0 --filter *ABC* --corerun C:\Projects\runtime\artifacts\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\before\CoreRun.exe C:\Projects\runtime\artifacts\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe

Copy link

ContributorAuthor

carlossanlop commentedApr 13, 2021•
edited
Loading

as a baseline the corerun that was built from main when you have created a new branch, without any of your changes
your branch with your changes

@adamsitnik @jozkee here's the table comparing the before (original) and after of this PR. Most results had allocation improvements, but worse speed when FileOptions = Asynchronous:

Results

Command:

 dotnet run    -c Release    -f net6.0    --corerun        D:\runtime2\artifacts\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe        D:\runtime\artifacts\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe    --filter System.IO.Tests.Perf_FileStream.*Async*    --statisticalTest 3ms    --minIterationCount 100    --maxIterationCount 101    --artifacts "D:\finalresults"

Note: The runtime2 corerun contains the baseline commit before my changes, the runtime corerun contains this PR's changes.

Method	Toolchain	fileSize	userBufferSize	options	Mean	Error	StdDev	Median	Min	Max	Ratio	Allocated
ReadAsync	BEFORE	1024	1024	None	78.69 μs	0.255 μs	0.736 μs	78.53 μs	77.61 μs	80.66 μs	1.00	5 KB
ReadAsync	AFTER	1024	1024	None	79.10 μs	0.237 μs	0.673 μs	79.08 μs	76.96 μs	80.71 μs	1.01	5 KB

WriteAsync	BEFORE	1024	1024	None	316.97 μs	2.512 μs	6.919 μs	315.92 μs	305.11 μs	339.35 μs	1.00	4 KB
WriteAsync	AFTER	1024	1024	None	300.13 μs	5.643 μs	15.730 μs	297.59 μs	272.46 μs	348.15 μs	0.95	4 KB

ReadAsync	BEFORE	1024	1024	Asynchronous	94.13 μs	0.259 μs	0.731 μs	94.02 μs	92.27 μs	96.16 μs	1.00	5 KB
ReadAsync	AFTER	1024	1024	Asynchronous	85.48 μs	0.842 μs	2.291 μs	85.11 μs	82.58 μs	92.09 μs	0.91	5 KB

WriteAsync	BEFORE	1024	1024	Asynchronous	292.63 μs	5.659 μs	15.869 μs	290.59 μs	264.25 μs	338.98 μs	1.00	5 KB
WriteAsync	AFTER	1024	1024	Asynchronous	324.35 μs	13.906 μs	39.222 μs	311.27 μs	274.34 μs	428.64 μs	1.11	5 KB

FlushAsync	BEFORE	1024	?	None	4,606.81 μs	46.736 μs	133.341 μs	4,633.92 μs	4,374.04 μs	4,952.63 μs	1.00	269 KB
FlushAsync	AFTER	1024	?	None	4,587.88 μs	58.046 μs	168.403 μs	4,638.98 μs	4,322.79 μs	5,034.48 μs	1.00	269 KB

CopyToFileAsync	BEFORE	1024	?	None	474.57 μs	5.497 μs	15.141 μs	474.66 μs	440.31 μs	520.31 μs	1.00	5 KB
CopyToFileAsync	AFTER	1024	?	None	488.94 μs	3.631 μs	10.061 μs	488.62 μs	469.93 μs	518.76 μs	1.03	5 KB

FlushAsync	BEFORE	1024	?	Asynchronous	14,680.35 μs	150.967 μs	437.982 μs	14,637.76 μs	13,623.50 μs	15,645.01 μs	1.00	301 KB
FlushAsync	AFTER	1024	?	Asynchronous	20,333.22 μs	116.277 μs	339.185 μs	20,299.93 μs	19,724.25 μs	21,148.73 μs	1.39	261 KB

CopyToFileAsync	BEFORE	1024	?	Asynchronous	520.97 μs	5.982 μs	16.674 μs	519.35 μs	490.05 μs	572.29 μs	1.00	6 KB
CopyToFileAsync	AFTER	1024	?	Asynchronous	523.30 μs	6.294 μs	17.650 μs	520.91 μs	479.14 μs	583.85 μs	1.00	6 KB

ReadAsync	BEFORE	1048576	512	None	1,375.28 μs	5.930 μs	17.015 μs	1,376.60 μs	1,340.44 μs	1,413.73 μs	1.00	85 KB
ReadAsync	AFTER	1048576	512	None	1,347.16 μs	5.128 μs	14.958 μs	1,345.14 μs	1,323.57 μs	1,382.14 μs	0.98	85 KB

WriteAsync	BEFORE	1048576	512	None	3,530.03 μs	38.149 μs	108.222 μs	3,521.78 μs	3,299.91 μs	3,823.63 μs	1.00	76 KB
WriteAsync	AFTER	1048576	512	None	3,473.85 μs	47.943 μs	134.438 μs	3,469.00 μs	3,222.90 μs	3,832.17 μs	0.98	76 KB

ReadAsync	BEFORE	1048576	512	Asynchronous	3,278.98 μs	30.616 μs	87.844 μs	3,257.98 μs	3,151.46 μs	3,507.69 μs	1.00	93 KB
ReadAsync	AFTER	1048576	512	Asynchronous	4,399.34 μs	16.153 μs	47.119 μs	4,401.64 μs	4,294.71 μs	4,531.75 μs	1.34	83 KB

WriteAsync	BEFORE	1048576	512	Asynchronous	5,304.10 μs	73.935 μs	214.500 μs	5,309.27 μs	4,771.82 μs	5,852.43 μs	1.00	85 KB
WriteAsync	AFTER	1048576	512	Asynchronous	6,506.15 μs	94.189 μs	271.756 μs	6,505.61 μs	6,042.59 μs	7,132.02 μs	1.23	75 KB

ReadAsync	BEFORE	1048576	4096	None	1,090.98 μs	6.156 μs	18.152 μs	1,096.14 μs	1,039.20 μs	1,120.19 μs	1.00	29 KB
ReadAsync	AFTER	1048576	4096	None	1,084.03 μs	8.601 μs	25.225 μs	1,086.13 μs	1,042.07 μs	1,131.85 μs	0.99	29 KB

WriteAsync	BEFORE	1048576	4096	None	3,247.60 μs	43.259 μs	124.117 μs	3,243.69 μs	2,954.42 μs	3,556.50 μs	1.00	55 KB
WriteAsync	AFTER	1048576	4096	None	3,250.35 μs	36.664 μs	100.985 μs	3,234.77 μs	3,039.74 μs	3,505.95 μs	1.00	55 KB

ReadAsync	BEFORE	1048576	4096	Asynchronous	3,273.61 μs	17.591 μs	50.753 μs	3,264.72 μs	3,190.18 μs	3,410.97 μs	1.00	79 KB
ReadAsync	AFTER	1048576	4096	Asynchronous	4,547.38 μs	73.438 μs	216.535 μs	4,398.34 μs	4,327.10 μs	5,035.98 μs	1.39	69 KB

WriteAsync	BEFORE	1048576	4096	Asynchronous	5,384.46 μs	83.792 μs	240.415 μs	5,346.69 μs	4,897.35 μs	5,955.92 μs	1.00	84 KB
WriteAsync	AFTER	1048576	4096	Asynchronous	6,845.63 μs	88.794 μs	256.192 μs	6,883.75 μs	6,079.92 μs	7,286.73 μs	1.27	74 KB

ReadAsync_NoBuffering	BEFORE	1048576	16384	None	406.34 μs	2.413 μs	7.113 μs	403.28 μs	387.16 μs	419.08 μs	1.00	7 KB
ReadAsync_NoBuffering	AFTER	1048576	16384	None	380.35 μs	3.413 μs	10.063 μs	378.36 μs	363.73 μs	402.82 μs	0.94	7 KB

WriteAsync_NoBuffering	BEFORE	1048576	16384	None	2,545.17 μs	30.174 μs	83.613 μs	2,526.42 μs	2,397.67 μs	2,776.47 μs	1.00	7 KB
WriteAsync_NoBuffering	AFTER	1048576	16384	None	2,537.77 μs	27.489 μs	76.628 μs	2,528.02 μs	2,421.65 μs	2,750.68 μs	1.00	7 KB

ReadAsync_NoBuffering	BEFORE	1048576	16384	Asynchronous	949.54 μs	6.336 μs	18.077 μs	943.69 μs	926.29 μs	1,002.20 μs	1.00	20 KB
ReadAsync_NoBuffering	AFTER	1048576	16384	Asynchronous	1,195.21 μs	3.070 μs	8.507 μs	1,194.94 μs	1,179.45 μs	1,218.92 μs	1.26	17 KB

WriteAsync_NoBuffering	BEFORE	1048576	16384	Asynchronous	2,965.91 μs	40.616 μs	114.558 μs	2,952.10 μs	2,677.93 μs	3,244.55 μs	1.00	20 KB
WriteAsync_NoBuffering	AFTER	1048576	16384	Asynchronous	3,021.90 μs	40.697 μs	116.110 μs	3,000.74 μs	2,803.04 μs	3,343.84 μs	1.02	17 KB

CopyToFileAsync	BEFORE	1048576	?	None	2,440.43 μs	17.056 μs	49.754 μs	2,437.81 μs	2,343.18 μs	2,576.37 μs	1.00	3 KB
CopyToFileAsync	AFTER	1048576	?	None	2,421.23 μs	15.376 μs	43.869 μs	2,413.43 μs	2,350.26 μs	2,521.45 μs	0.99	3 KB

CopyToFileAsync	BEFORE	1048576	?	Asynchronous	2,878.14 μs	16.682 μs	47.863 μs	2,876.69 μs	2,771.42 μs	3,001.49 μs	1.00	4 KB
CopyToFileAsync	AFTER	1048576	?	Asynchronous	2,769.09 μs	15.649 μs	43.882 μs	2,760.85 μs	2,691.56 μs	2,885.67 μs	0.96	4 KB

ReadAsync	BEFORE	104857600	4096	None	115,657.20 μs	352.576 μs	1,039.577 μs	115,639.45 μs	113,670.15 μs	117,832.15 μs	1.00	2,801 KB
ReadAsync	AFTER	104857600	4096	None	116,612.88 μs	249.577 μs	731.966 μs	116,751.05 μs	114,615.00 μs	117,903.05 μs	1.01	2,801 KB

WriteAsync	BEFORE	104857600	4096	None	205,378.59 μs	7,785.258 μs	22,462.248 μs	194,584.50 μs	184,125.30 μs	268,874.60 μs	1.00	5,005 KB
WriteAsync	AFTER	104857600	4096	None	209,453.56 μs	10,355.532 μs	30,043.276 μs	194,064.20 μs	173,778.40 μs	293,593.30 μs	1.03	5,005 KB

ReadAsync	BEFORE	104857600	4096	Asynchronous	327,225.97 μs	1,532.011 μs	4,346.059 μs	326,564.00 μs	320,187.10 μs	339,508.20 μs	1.00	7,801 KB
ReadAsync	AFTER	104857600	4096	Asynchronous	457,654.89 μs	902.286 μs	2,559.634 μs	457,207.00 μs	451,652.60 μs	465,093.00 μs	1.40	6,801 KB

WriteAsync	BEFORE	104857600	4096	Asynchronous	454,994.00 μs	7,661.181 μs	21,857.780 μs	449,253.50 μs	421,512.80 μs	511,199.10 μs	1.00	7,905 KB
WriteAsync	AFTER	104857600	4096	Asynchronous	600,076.62 μs	10,088.501 μs	29,268.570 μs	592,035.20 μs	559,265.50 μs	677,403.80 μs	1.32	6,905 KB

ReadAsync_NoBuffering	BEFORE	104857600	16384	None	45,086.23 μs	156.169 μs	453.074 μs	45,005.65 μs	43,497.93 μs	46,041.93 μs	1.00	701 KB
ReadAsync_NoBuffering	AFTER	104857600	16384	None	42,133.53 μs	174.922 μs	457.740 μs	42,093.18 μs	41,187.75 μs	43,731.75 μs	0.93	701 KB

WriteAsync_NoBuffering	BEFORE	104857600	16384	None	61,814.53 μs	685.277 μs	1,887.453 μs	61,419.69 μs	59,079.38 μs	68,035.43 μs	1.00	701 KB
WriteAsync_NoBuffering	AFTER	104857600	16384	None	61,435.05 μs	720.962 μs	1,973.622 μs	61,127.18 μs	58,506.18 μs	67,827.25 μs	0.99	701 KB

ReadAsync_NoBuffering	BEFORE	104857600	16384	Asynchronous	97,220.19 μs	667.147 μs	1,881.701 μs	96,464.10 μs	94,386.65 μs	102,972.80 μs	1.00	1,951 KB
ReadAsync_NoBuffering	AFTER	104857600	16384	Asynchronous	128,169.40 μs	556.512 μs	1,587.760 μs	127,771.80 μs	125,455.30 μs	133,061.15 μs	1.32	1,701 KB

WriteAsync_NoBuffering	BEFORE	104857600	16384	Asynchronous	136,593.81 μs	1,249.681 μs	3,462.860 μs	136,822.00 μs	127,534.30 μs	146,281.60 μs	1.00	1,951 KB
WriteAsync_NoBuffering	AFTER	104857600	16384	Asynchronous	169,656.11 μs	2,245.877 μs	6,515.696 μs	168,284.30 μs	157,404.20 μs	189,342.10 μs	1.25	1,701 KB

CopyToFileAsync	BEFORE	104857600	?	None	56,537.38 μs	245.101 μs	695.312 μs	56,440.93 μs	55,567.65 μs	58,581.80 μs	1.00	177 KB
CopyToFileAsync	AFTER	104857600	?	None	56,184.32 μs	275.348 μs	785.585 μs	56,116.43 μs	54,790.00 μs	58,574.72 μs	0.99	177 KB

CopyToFileAsync	BEFORE	104857600	?	Asynchronous	87,668.39 μs	312.004 μs	905.181 μs	87,555.38 μs	85,324.65 μs	89,838.85 μs	1.00	246 KB
CopyToFileAsync	AFTER	104857600	?	Asynchronous	78,490.65 μs	501.805 μs	1,431.678 μs	78,350.84 μs	75,379.32 μs	82,799.10 μs	0.90	214 KB

carlossanlop added5 commits

April 13, 2021 12:55

Remove IFileStreamCompletionStrategy. This makes FileStreamCompletion…

bbc28e9

…Source only needed by Net5CompatFileStreamStrategy.

Use AwaitableProvider for AsyncWindowsFileStreamStrategy.

7449c79

No need for ManualResetValueTaskSource class. Remove it to get rid of…

4ee6d26

… one allocation and get the actual profiling improvement.Rename FileStreamAwaitableProvider.

Bring back ReadAsync/WriteAsync code to the strategy for simpler code…

c586540

… review. Shorter filename for awaitable provider.

FileStreamCompletionSource can now be nested in Net5CompatFileStreamS…

84b2d30

…trategy, and its _fileHandle can now be private.

carlossanlop added13 commits

April 13, 2021 12:56

Remove duplicate error definitions, use centralized Interop.Errors.

3f842c4

Move shared mask values to FileStreamHelpers to avoid duplication.

302e2fd

Use Interop.Errors also in SyncWindowsFileStreamStrategy.

f0af5d7

Move misplaced comment, move method location.

47cb553

Slightly better comment.

dc2fb7e

Rename FileStreamAwaitableProvider to FileStreamValueTaskSource to ke…

21fe9af

…ep in sync with the name of FileStreamCompletionSource.

Bring back the raw pointer intOverlapped.

877e28d

Rename MemoryAwaitableProvider to MemoryFileStreamValueTaskSource.

48a50b3

Remove numBufferedBytes and avoid unnecessary allocation in Read(byte…

e61732b

…[],int,int).

Rename files of nested types.

ba3e23e

Nested async result codes in static class.

6595f14

Address feedback.

32bdb7b

Address suggestions

b05dab9

Copy link

Member

adamsitnik commentedApr 14, 2021

Most results had allocation improvements, but worse speed when FileOptions = Asynchronous:

As discussed offline, such a regression needs to be solved before merging the PR.

I've even built your fork and run the benchmarks myself to be 100% sure, but unfortunately, I've confirmed the regression:

Method	Toolchain	fileSize	userBufferSize	options	Mean	Ratio	Allocated
ReadAsync_NoBuffering	\after\corerun.exe	1048576	16384	Asynchronous	838.1 us	1.12	17 KB
ReadAsync_NoBuffering	\before\corerun.exe	1048576	16384	Asynchronous	748.1 us	1.00	20 KB

ReadAsync_NoBuffering	\after\corerun.exe	104857600	16384	Asynchronous	93,214.6 us	1.12	1,700 KB
ReadAsync_NoBuffering	\before\corerun.exe	104857600	16384	Asynchronous	83,445.4 us	1.00	1,950 KB

If I can suggest something,ReadAsync_NoBuffering withfileSize=1048576 andoptions=Asynchronous should be the best benchmark for creating a repo app and profiling it. It's the simplest benchmark (buffering is not involved) andReadAsync has a smaller deviation thanWriteAsync. If you fail to find the regression with VS Profiler, you can usePerfview.

adamsitnik added the NO-MERGEThe PR is not ready for merge yet (see discussion for detailed reasons) label

Apr 14, 2021

Copy link

ContributorAuthor

carlossanlop commentedApr 14, 2021

I compared the CPU usage before and after, and discovered we spend a considerable amount of time insideOverlapped.PerformIOCompletionCallback.

The new code (left) consumes 62% of the time callingExecutionContext.RunInternal, while the old code (right) only took 3.82%.

Screenshot

The root cause seems to be this line, where we call_source.SetResult:

Screenshot

Which is adding up the time it takes to run the rest of theReadAsync calls in the loop, including the disposal of theFileStream object in the main method.

There's a comment stating thatExecutionContext.RunInternal (part of the callstack) warning that the code is a very hot path:

Screenshot

I wasn't expecting all the continuation code to be counted as part of the asynchronous calls. I wonder if this has anything to do with the removal ofRunContinuationsAsynchronously = true flag. I'll bring that flag back and compare the results.

cc@jozkee @adamsitnik @stephentoub

Copy link

Member

davidfowl commentedApr 14, 2021

We don't need to restore the ExecutionContext in the overlapped, that restore should be avoided in these APIs. The async state machine already does it. This is what you need#42549 😄

Copy link

Member

stephentoub commentedApr 14, 2021•
edited
Loading

The new code (left) consumes 62% of the time calling ExecutionContext.RunInternal, while the old code (right) only took 3.82%.

That's inclusive (what "Total" means in the VS view). The callback is actually running the continuation now (because of RunContinuationsAsynchronously=false), rather than queueing it to be run on a different thread, so the 62% is inclusive of the actual app code.

stephentoub reviewed

Apr 14, 2021

...m.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.ValueTaskSource.cs OutdatedShow resolvedHide resolved

stephentoub reviewed

Apr 14, 2021

...m.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.ValueTaskSource.cs Outdated

Copy link

Member

stephentoubApr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I know you're copying this logic from what was there before, but it seems to be unnecessarily complex, with lots of interlocked operations for transitioning from one state to another to another. If we're looking for places to reduce overheads, revisiting this whole implementation is likely a good place to start.

Copy link

Member

adamsitnik commentedApr 15, 2021

The callback is actually running the continuation now (because of RunContinuationsAsynchronously=false)

I've run all the async benchmarks for 3 configurations:

"before" (@carlossanlop fork main branch)
"false" (the current state of this PR asRunContinuationsAsynchronously is not set so it'sfalse)
"true" ( _source.RunContinuationsAsynchronously = true; inctor)

With the following settings:

100 instead of 20 iterations (--minIterationCount 100 --maxIterationCount 101)
no outliers removal (--outliers DontRemove)

and confirmed that with _source.RunContinuationsAsynchronously = true; we have no regression in themicrobenchmarks

@stephentoub should we:

setRunContinuationsAsynchronously = true; and include this PR in preview4
get some "real world" numbers from ASP.NET benchmarks (if there are none, write some and contribute tohttps://github.com/aspnet/benchmarks) and see howRunContinuationsAsynchronously setting affects non-micro benchmarks. This means preview5 for this PR.

Method	Toolchain	fileSize	userBufferSize	options	Mean	StdDev	Median	Min	Max	Ratio	Allocated
ReadAsync	\before\corerun.exe	1024	1024	Asynchronous	82.69 us	1.946 us	82.22 us	81.60 us	94.90 us	1.00	5 KB
ReadAsync	\false\corerun.exe	1024	1024	Asynchronous	78.76 us	0.718 us	78.56 us	78.04 us	82.13 us	0.95	5 KB
ReadAsync	\true\corerun.exe	1024	1024	Asynchronous	83.28 us	0.825 us	83.12 us	82.48 us	89.82 us	1.01	5 KB

WriteAsync	\before\corerun.exe	1024	1024	Asynchronous	404.05 us	22.574 us	400.81 us	379.20 us	494.53 us	1.00	5 KB
WriteAsync	\false\corerun.exe	1024	1024	Asynchronous	400.76 us	17.211 us	397.80 us	380.07 us	472.21 us	0.99	5 KB
WriteAsync	\true\corerun.exe	1024	1024	Asynchronous	405.20 us	25.469 us	401.47 us	383.59 us	510.74 us	1.01	5 KB

ReadAsync	\before\corerun.exe	1048576	512	Asynchronous	2,418.83 us	55.684 us	2,413.59 us	2,361.95 us	2,804.46 us	1.00	93 KB
ReadAsync	\false\corerun.exe	1048576	512	Asynchronous	2,648.89 us	73.366 us	2,626.45 us	2,550.29 us	2,959.92 us	1.10	83 KB
ReadAsync	\true\corerun.exe	1048576	512	Asynchronous	2,386.25 us	9.985 us	2,386.16 us	2,356.79 us	2,419.58 us	0.99	83 KB

WriteAsync	\before\corerun.exe	1048576	512	Asynchronous	4,254.70 us	277.778 us	4,213.63 us	3,967.55 us	6,203.71 us	1.00	85 KB
WriteAsync	\false\corerun.exe	1048576	512	Asynchronous	4,583.08 us	183.949 us	4,559.42 us	4,317.29 us	5,334.19 us	1.08	75 KB
WriteAsync	\true\corerun.exe	1048576	512	Asynchronous	4,207.77 us	179.340 us	4,173.53 us	3,896.65 us	4,865.02 us	0.99	75 KB

ReadAsync	\before\corerun.exe	1048576	4096	Asynchronous	2,343.06 us	35.339 us	2,355.62 us	2,225.73 us	2,379.72 us	1.00	79 KB
ReadAsync	\false\corerun.exe	1048576	4096	Asynchronous	2,603.91 us	40.916 us	2,603.11 us	2,513.55 us	2,715.90 us	1.11	69 KB
ReadAsync	\true\corerun.exe	1048576	4096	Asynchronous	2,324.68 us	38.225 us	2,337.29 us	2,226.14 us	2,374.04 us	0.99	69 KB

WriteAsync	\before\corerun.exe	1048576	4096	Asynchronous	4,110.48 us	265.902 us	4,050.53 us	3,844.25 us	6,007.74 us	1.00	84 KB
WriteAsync	\false\corerun.exe	1048576	4096	Asynchronous	4,572.16 us	320.658 us	4,542.09 us	4,256.30 us	7,440.25 us	1.12	74 KB
WriteAsync	\true\corerun.exe	1048576	4096	Asynchronous	4,048.01 us	184.553 us	4,015.01 us	3,739.28 us	4,754.61 us	0.99	74 KB

ReadAsync_NoBuffering	\before\corerun.exe	1048576	16384	Asynchronous	747.53 us	5.076 us	747.87 us	729.80 us	755.69 us	1.00	20 KB
ReadAsync_NoBuffering	\false\corerun.exe	1048576	16384	Asynchronous	822.87 us	6.877 us	822.90 us	809.94 us	838.18 us	1.10	17 KB
ReadAsync_NoBuffering	\true\corerun.exe	1048576	16384	Asynchronous	726.82 us	11.262 us	726.44 us	699.96 us	754.36 us	0.97	17 KB

WriteAsync_NoBuffering	\before\corerun.exe	1048576	16384	Asynchronous	2,726.54 us	136.755 us	2,712.48 us	2,534.40 us	3,234.12 us	1.00	20 KB
WriteAsync_NoBuffering	\false\corerun.exe	1048576	16384	Asynchronous	2,761.05 us	146.361 us	2,792.11 us	2,507.70 us	3,188.16 us	1.01	17 KB
WriteAsync_NoBuffering	\true\corerun.exe	1048576	16384	Asynchronous	2,736.46 us	194.495 us	2,705.98 us	2,415.11 us	4,022.13 us	1.01	17 KB

ReadAsync	\before\corerun.exe	104857600	4096	Asynchronous	250,490.85 us	2,604.707 us	250,256.45 us	244,814.30 us	256,607.90 us	1.00	7,801 KB
ReadAsync	\false\corerun.exe	104857600	4096	Asynchronous	278,260.89 us	4,931.050 us	278,009.45 us	269,882.60 us	297,338.40 us	1.11	6,803 KB
ReadAsync	\true\corerun.exe	104857600	4096	Asynchronous	252,682.05 us	19,763.442 us	250,702.50 us	243,911.20 us	447,534.10 us	1.01	6,801 KB

WriteAsync	\before\corerun.exe	104857600	4096	Asynchronous	370,893.59 us	6,590.799 us	369,781.05 us	352,834.20 us	390,366.40 us	1.00	7,905 KB
WriteAsync	\false\corerun.exe	104857600	4096	Asynchronous	397,619.03 us	8,020.098 us	397,052.95 us	384,609.00 us	446,559.70 us	1.07	6,905 KB
WriteAsync	\true\corerun.exe	104857600	4096	Asynchronous	358,128.05 us	6,536.171 us	357,755.45 us	346,707.00 us	382,735.70 us	0.97	6,905 KB

Bring back RunContinuationAsynchronously=true

d5344be

adamsitnik removed the NO-MERGEThe PR is not ready for merge yet (see discussion for detailed reasons) label

Apr 15, 2021

adamsitnik approved these changes

Apr 15, 2021