Skip to content

Commit 91423fa

Browse files
adamsitnikadamsitnik
committed
track the file offset in memory, don't use expensive sys calls to synchronize it
1 parent d06f175 commit 91423fa

File tree

7 files changed

+106
-55
lines changed

7 files changed

+106
-55
lines changed

‎src/libraries/Common/src/Interop/Windows/Kernel32/Interop.ReadFile_SafeHandle_NativeOverlapped.cs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,5 +16,13 @@ internal static extern unsafe int ReadFile(
1616
int numBytesToRead,
1717
IntPtr numBytesRead_mustBeZero,
1818
NativeOverlapped* overlapped);
19+
20+
[DllImport(Libraries.Kernel32, SetLastError = true)]
21+
internal static extern unsafe int ReadFile(
22+
SafeHandle handle,
23+
byte* bytes,
24+
int numBytesToRead,
25+
out int numBytesRead,
26+
NativeOverlapped* overlapped);
1927
}
2028
}

‎src/libraries/Common/src/Interop/Windows/Kernel32/Interop.WriteFile_SafeHandle_NativeOverlapped.cs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,5 +17,8 @@ internal static partial class Kernel32
1717
// and pass in an address for the numBytesRead parameter.
1818
[DllImport(Libraries.Kernel32, SetLastError = true)]
1919
internal static extern unsafe int WriteFile(SafeHandle handle, byte* bytes, int numBytesToWrite, IntPtr numBytesWritten_mustBeZero, NativeOverlapped* lpOverlapped);
20+
21+
[DllImport(Libraries.Kernel32, SetLastError = true)]
22+
internal static extern unsafe int WriteFile(SafeHandle handle, byte* bytes, int numBytesToWrite, out int numBytesWritten, NativeOverlapped* lpOverlapped);
2023
}
2124
}

‎src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs‎

Lines changed: 20 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -140,15 +140,16 @@ private unsafe Task<int> ReadAsyncInternal(Memory<byte> destination, Cancellatio
140140
NativeOverlapped* intOverlapped = completionSource.Overlapped;
141141

142142
// Calculate position in the file we should be at after the read is done
143+
long positionBefore = _filePosition;
143144
if (CanSeek)
144145
{
145146
long len = Length;
146147

147-
if (_filePosition + destination.Length > len)
148+
if (positionBefore + destination.Length > len)
148149
{
149-
if (_filePosition <= len)
150+
if (positionBefore <= len)
150151
{
151-
destination = destination.Slice(0, (int)(len - _filePosition));
152+
destination = destination.Slice(0, (int)(len - positionBefore));
152153
}
153154
else
154155
{
@@ -158,23 +159,17 @@ private unsafe Task<int> ReadAsyncInternal(Memory<byte> destination, Cancellatio
158159

159160
// Now set the position to read from in the NativeOverlapped struct
160161
// For pipes, we should leave the offset fields set to 0.
161-
intOverlapped->OffsetLow = unchecked((int)_filePosition);
162-
intOverlapped->OffsetHigh = (int)(_filePosition >> 32);
162+
intOverlapped->OffsetLow = unchecked((int)positionBefore);
163+
intOverlapped->OffsetHigh = (int)(positionBefore >> 32);
163164

164165
// When using overlapped IO, the OS is not supposed to
165166
// touch the file pointer location at all. We will adjust it
166-
// ourselves. This isn't threadsafe.
167-
168-
// WriteFile should not update the file pointer when writing
169-
// in overlapped mode, according to MSDN. But it does update
170-
// the file pointer when writing to a UNC path!
171-
// So changed the code below to seek to an absolute
172-
// location, not a relative one. ReadFile seems consistent though.
173-
SeekCore(_fileHandle, destination.Length, SeekOrigin.Current);
167+
// ourselves, but only in memory. This isn't threadsafe.
168+
_filePosition += destination.Length;
174169
}
175170

176171
// queue an async ReadFile operation and pass in a packed overlapped
177-
int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination.Span, intOverlapped, out int errorCode);
172+
int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination.Span, false, intOverlapped, out int errorCode);
178173

179174
// ReadFile, the OS version, will return 0 on failure. But
180175
// my ReadFileNative wrapper returns -1. My wrapper will return
@@ -203,7 +198,7 @@ private unsafe Task<int> ReadAsyncInternal(Memory<byte> destination, Cancellatio
203198
{
204199
if (!_fileHandle.IsClosed && CanSeek) // Update Position - It could be anywhere.
205200
{
206-
SeekCore(_fileHandle, 0, SeekOrigin.Current);
201+
_filePosition = positionBefore;
207202
}
208203

209204
completionSource.ReleaseNativeResource();
@@ -264,29 +259,30 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory<byte> source, Cancella
264259
FileStreamCompletionSource completionSource = FileStreamCompletionSource.Create(this, _preallocatedOverlapped, 0, source);
265260
NativeOverlapped* intOverlapped = completionSource.Overlapped;
266261

262+
long positionBefore = _filePosition;
267263
if (CanSeek)
268264
{
269265
// Make sure we set the length of the file appropriately.
270266
long len = Length;
271267

272-
if (_filePosition + source.Length > len)
268+
if (positionBefore + source.Length > len)
273269
{
274-
SetLengthCore(_filePosition + source.Length);
270+
SetLengthCore(positionBefore + source.Length);
275271
}
276272

277273
// Now set the position to read from in the NativeOverlapped struct
278274
// For pipes, we should leave the offset fields set to 0.
279-
intOverlapped->OffsetLow = (int)_filePosition;
280-
intOverlapped->OffsetHigh = (int)(_filePosition >> 32);
275+
intOverlapped->OffsetLow = (int)positionBefore;
276+
intOverlapped->OffsetHigh = (int)(positionBefore >> 32);
281277

282278
// When using overlapped IO, the OS is not supposed to
283279
// touch the file pointer location at all. We will adjust it
284-
// ourselves. This isn't threadsafe.
285-
SeekCore(_fileHandle, source.Length, SeekOrigin.Current);
280+
// ourselves, but only in memory. This isn't threadsafe.
281+
_filePosition += source.Length;
286282
}
287283

288284
// queue an async WriteFile operation and pass in a packed overlapped
289-
int r = FileStreamHelpers.WriteFileNative(_fileHandle, source.Span, intOverlapped, out int errorCode);
285+
int r = FileStreamHelpers.WriteFileNative(_fileHandle, source.Span, false, intOverlapped, out int errorCode);
290286

291287
// WriteFile, the OS version, will return 0 on failure. But
292288
// my WriteFileNative wrapper returns -1. My wrapper will return
@@ -312,7 +308,7 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory<byte> source, Cancella
312308
{
313309
if (!_fileHandle.IsClosed && CanSeek) // Update Position - It could be anywhere.
314310
{
315-
SeekCore(_fileHandle, 0, SeekOrigin.Current);
311+
_filePosition = positionBefore;
316312
}
317313

318314
completionSource.ReleaseNativeResource();
@@ -385,7 +381,7 @@ private async Task AsyncModeCopyToAsync(Stream destination, int bufferSize, Canc
385381
// Make sure the stream's current position reflects where we ended up
386382
if (!_fileHandle.IsClosed && CanSeek)
387383
{
388-
SeekCore(_fileHandle, 0, SeekOrigin.End);
384+
_filePosition = Length;
389385
}
390386
}
391387
}

‎src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs‎

Lines changed: 29 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -337,7 +337,7 @@ internal static unsafe void SetLength(SafeFileHandle handle, string? path, long
337337
}
338338

339339
// __ConsoleStream also uses this code.
340-
internal static unsafe int ReadFileNative(SafeFileHandle handle, Span<byte> bytes, NativeOverlapped* overlapped, out int errorCode)
340+
internal static unsafe int ReadFileNative(SafeFileHandle handle, Span<byte> bytes, bool syncUsingOverlapped, NativeOverlapped* overlapped, out int errorCode)
341341
{
342342
Debug.Assert(handle != null, "handle != null");
343343

@@ -347,13 +347,24 @@ internal static unsafe int ReadFileNative(SafeFileHandle handle, Span<byte> byte
347347
fixed (byte* p = &MemoryMarshal.GetReference(bytes))
348348
{
349349
r = overlapped != null ?
350-
Interop.Kernel32.ReadFile(handle, p, bytes.Length, IntPtr.Zero, overlapped) :
351-
Interop.Kernel32.ReadFile(handle, p, bytes.Length, out numBytesRead, IntPtr.Zero);
350+
(syncUsingOverlapped
351+
? Interop.Kernel32.ReadFile(handle, p, bytes.Length, out numBytesRead, overlapped)
352+
: Interop.Kernel32.ReadFile(handle, p, bytes.Length, IntPtr.Zero, overlapped))
353+
: Interop.Kernel32.ReadFile(handle, p, bytes.Length, out numBytesRead, IntPtr.Zero);
352354
}
353355

354356
if (r == 0)
355357
{
356358
errorCode = GetLastWin32ErrorAndDisposeHandleIfInvalid(handle);
359+
360+
if (syncUsingOverlapped && errorCode == Interop.Errors.ERROR_HANDLE_EOF)
361+
{
362+
// https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-readfile#synchronization-and-file-position :
363+
// "If lpOverlapped is not NULL, then when a synchronous read operation reaches the end of a file,
364+
// ReadFile returns FALSE and GetLastError returns ERROR_HANDLE_EOF"
365+
return numBytesRead;
366+
}
367+
357368
return -1;
358369
}
359370
else
@@ -363,7 +374,7 @@ internal static unsafe int ReadFileNative(SafeFileHandle handle, Span<byte> byte
363374
}
364375
}
365376

366-
internal static unsafe int WriteFileNative(SafeFileHandle handle, ReadOnlySpan<byte> buffer, NativeOverlapped* overlapped, out int errorCode)
377+
internal static unsafe int WriteFileNative(SafeFileHandle handle, ReadOnlySpan<byte> buffer, bool syncUsingOverlapped, NativeOverlapped* overlapped, out int errorCode)
367378
{
368379
Debug.Assert(handle != null, "handle != null");
369380

@@ -373,13 +384,24 @@ internal static unsafe int WriteFileNative(SafeFileHandle handle, ReadOnlySpan<b
373384
fixed (byte* p = &MemoryMarshal.GetReference(buffer))
374385
{
375386
r = overlapped != null ?
376-
Interop.Kernel32.WriteFile(handle, p, buffer.Length, IntPtr.Zero, overlapped) :
377-
Interop.Kernel32.WriteFile(handle, p, buffer.Length, out numBytesWritten, IntPtr.Zero);
387+
(syncUsingOverlapped
388+
? Interop.Kernel32.WriteFile(handle, p, buffer.Length, out numBytesWritten, overlapped)
389+
: Interop.Kernel32.WriteFile(handle, p, buffer.Length, IntPtr.Zero, overlapped))
390+
: Interop.Kernel32.WriteFile(handle, p, buffer.Length, out numBytesWritten, IntPtr.Zero);
378391
}
379392

380393
if (r == 0)
381394
{
382395
errorCode = GetLastWin32ErrorAndDisposeHandleIfInvalid(handle);
396+
397+
if (syncUsingOverlapped && errorCode == Interop.Errors.ERROR_HANDLE_EOF)
398+
{
399+
// https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-readfile#synchronization-and-file-position :
400+
// "If lpOverlapped is not NULL, then when a synchronous read operation reaches the end of a file,
401+
// ReadFile returns FALSE and GetLastError returns ERROR_HANDLE_EOF"
402+
return numBytesWritten;
403+
}
404+
383405
return -1;
384406
}
385407
else
@@ -464,7 +486,7 @@ internal static async Task AsyncModeCopyToAsync(SafeFileHandle handle, string? p
464486
}
465487

466488
// Kick off the read.
467-
synchronousSuccess = ReadFileNative(handle, copyBuffer, readAwaitable._nativeOverlapped, out errorCode) >= 0;
489+
synchronousSuccess = ReadFileNative(handle, copyBuffer, false, readAwaitable._nativeOverlapped, out errorCode) >= 0;
468490
}
469491

470492
// If the operation did not synchronously succeed, it either failed or initiated the asynchronous operation.

‎src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.Windows.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1079,14 +1079,14 @@ private unsafe int ReadFileNative(SafeFileHandle handle, Span<byte> bytes, Nativ
10791079
{
10801080
Debug.Assert((_useAsyncIO && overlapped != null) || (!_useAsyncIO && overlapped == null), "Async IO and overlapped parameters inconsistent in call to ReadFileNative.");
10811081

1082-
return FileStreamHelpers.ReadFileNative(handle, bytes, overlapped, out errorCode);
1082+
return FileStreamHelpers.ReadFileNative(handle, bytes, false, overlapped, out errorCode);
10831083
}
10841084

10851085
private unsafe int WriteFileNative(SafeFileHandle handle, ReadOnlySpan<byte> buffer, NativeOverlapped* overlapped, out int errorCode)
10861086
{
10871087
Debug.Assert((_useAsyncIO && overlapped != null) || (!_useAsyncIO && overlapped == null), "Async IO and overlapped parameters inconsistent in call to WriteFileNative.");
10881088

1089-
return FileStreamHelpers.WriteFileNative(handle, buffer, overlapped, out errorCode);
1089+
return FileStreamHelpers.WriteFileNative(handle, buffer, false, overlapped, out errorCode);
10901090
}
10911091

10921092
public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken)

‎src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,8 @@ private unsafe int ReadSpan(Span<byte> destination)
104104

105105
Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed");
106106

107-
int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination, null, out int errorCode);
107+
NativeOverlapped nativeOverlapped = GetNativeOverlappedForCurrentPosition();
108+
int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination, true, &nativeOverlapped, out int errorCode);
108109

109110
if (r == -1)
110111
{
@@ -136,7 +137,8 @@ private unsafe void WriteSpan(ReadOnlySpan<byte> source)
136137

137138
Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed");
138139

139-
int r = FileStreamHelpers.WriteFileNative(_fileHandle, source, null, out int errorCode);
140+
NativeOverlapped nativeOverlapped = GetNativeOverlappedForCurrentPosition();
141+
int r = FileStreamHelpers.WriteFileNative(_fileHandle, source, true, &nativeOverlapped, out int errorCode);
140142

141143
if (r == -1)
142144
{
@@ -159,5 +161,15 @@ private unsafe void WriteSpan(ReadOnlySpan<byte> source)
159161
_filePosition += r;
160162
return;
161163
}
164+
165+
private NativeOverlapped GetNativeOverlappedForCurrentPosition()
166+
{
167+
NativeOverlapped nativeOverlapped = default;
168+
// For pipes the offsets are ignored by the OS
169+
nativeOverlapped.OffsetLow = unchecked((int)_filePosition);
170+
nativeOverlapped.OffsetHigh = (int)(_filePosition >> 32);
171+
172+
return nativeOverlapped;
173+
}
162174
}
163175
}

‎src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs‎

Lines changed: 30 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -83,14 +83,16 @@ internal WindowsFileStreamStrategy(string path, FileMode mode, FileAccess access
8383
public override long Position
8484
{
8585
get => _filePosition;
86-
set => Seek(value, SeekOrigin.Begin);
86+
set => _filePosition = value;
8787
}
8888

8989
internal sealed override string Name => _path ?? SR.IO_UnknownFileName;
9090

9191
internal sealed override bool IsClosed => _fileHandle.IsClosed;
9292

9393
// Flushing is the responsibility of BufferedFileStreamStrategy
94+
// TODO: we might consider calling FileStreamHelpers.Seek(handle, _path, _filePosition, SeekOrigin.Begin);
95+
// here to set the actual file offset
9496
internal sealed override SafeFileHandle SafeFileHandle => _fileHandle;
9597

9698
// ReadByte and WriteByte methods are used only when the user has disabled buffering on purpose
@@ -152,29 +154,33 @@ public sealed override long Seek(long offset, SeekOrigin origin)
152154
if (!CanSeek) throw Error.GetSeekNotSupported();
153155

154156
long oldPos = _filePosition;
155-
long pos = SeekCore(_fileHandle, offset, origin);
157+
long pos = origin switch
158+
{
159+
SeekOrigin.Begin => offset,
160+
SeekOrigin.End => FileStreamHelpers.GetFileLength(_fileHandle, _path) + offset,
161+
_ => _filePosition + offset // SeekOrigin.Current
162+
};
163+
164+
if (pos >= 0)
165+
{
166+
_filePosition = pos;
167+
}
168+
else
169+
{
170+
// keep throwing the same exception we did when seek was causing actual offset change
171+
throw Win32Marshal.GetExceptionForWin32Error(Interop.Errors.ERROR_INVALID_PARAMETER);
172+
}
156173

157-
// Prevent users from overwriting data in a file that was opened in
158-
// append mode.
174+
// Prevent users from overwriting data in a file that was opened in append mode.
159175
if (_appendStart != -1 && pos < _appendStart)
160176
{
161-
SeekCore(_fileHandle, oldPos, SeekOrigin.Begin);
177+
_filePosition = oldPos;
162178
throw new IOException(SR.IO_SeekAppendOverwrite);
163179
}
164180

165181
return pos;
166182
}
167183

168-
// This doesn't do argument checking. Necessary for SetLength, which must
169-
// set the file pointer beyond the end of the file. This will update the
170-
// internal position
171-
protected long SeekCore(SafeFileHandle fileHandle, long offset, SeekOrigin origin, bool closeInvalidHandle = false)
172-
{
173-
Debug.Assert(!fileHandle.IsClosed && _canSeek, "!fileHandle.IsClosed && _canSeek");
174-
175-
return _filePosition = FileStreamHelpers.Seek(fileHandle, _path, offset, origin, closeInvalidHandle);
176-
}
177-
178184
internal sealed override void Lock(long position, long length) => FileStreamHelpers.Lock(_fileHandle, _path, position, length);
179185

180186
internal sealed override void Unlock(long position, long length) => FileStreamHelpers.Unlock(_fileHandle, _path, position, length);
@@ -192,7 +198,7 @@ private void Init(FileMode mode, string originalPath)
192198
// For Append mode...
193199
if (mode == FileMode.Append)
194200
{
195-
_appendStart = SeekCore(_fileHandle, 0, SeekOrigin.End);
201+
_appendStart = _filePosition = Length;
196202
}
197203
else
198204
{
@@ -226,9 +232,15 @@ private void InitFromHandleImpl(SafeFileHandle handle, out bool canSeek, out boo
226232
OnInitFromHandle(handle);
227233

228234
if (_canSeek)
229-
SeekCore(handle, 0, SeekOrigin.Current);
235+
{
236+
// given strategy was created out of existing handle, so we have to perform
237+
// a syscall to get the current handle offset
238+
_filePosition = FileStreamHelpers.Seek(handle, _path, 0, SeekOrigin.Current);
239+
}
230240
else
241+
{
231242
_filePosition = 0;
243+
}
232244
}
233245

234246
public sealed override void SetLength(long value)
@@ -239,8 +251,6 @@ public sealed override void SetLength(long value)
239251
SetLengthCore(value);
240252
}
241253

242-
// We absolutely need this method broken out so that WriteInternalCoreAsync can call
243-
// a method without having to go through buffering code that might call FlushWrite.
244254
protected unsafe void SetLengthCore(long value)
245255
{
246256
Debug.Assert(value >= 0, "value >= 0");
@@ -249,7 +259,7 @@ protected unsafe void SetLengthCore(long value)
249259

250260
if (_filePosition > value)
251261
{
252-
SeekCore(_fileHandle, 0, SeekOrigin.End);
262+
_filePosition = value;
253263
}
254264
}
255265
}

0 commit comments

Comments
 (0)