From 7d3a1320e00f0ce2ad68ac95e73fe03347633e39 Mon Sep 17 00:00:00 2001 From: stephentoub Date: Mon, 18 Jan 2016 10:44:35 -0500 Subject: [PATCH 1/4] Several Stream.Read/WriteAsync improvements Stream.XxAsync are currently implemented as a wrapper around Stream.Begin/EndXx. When a derived class overrides XxAsync to do its own async implementation, there's no issue. When a derived class overrides Begin/EndXx but not XxAsync, there's no issue (the base implementation does what it needs to do, wrapping Begin/EndXx). However, if the derived implementation doesn't override either XxAsync or Begin/EndXx, there are a few issues. First, there's unnecessary cost. The base Begin/EndXx methods queue a Task to call the corresponding Read/Write method. But then the XxAsync method create another Task to wrap the Begin/End method invocation. This means we're allocating and completing two tasks, when we only needed to do one. Second, task wait inlining is affected. Because Read/WriteAsync aren't returning the original queued delegate-backed task but rather a promise task, Wait()'ing on the returned task blocks until the operation completes on another thread. If the original delegate-backed task were returned, then Wait()'ing on the task could potentially "inline" its execution onto the current thread. Third, there's unnecessary blocking if there are other outstanding async operations on the instance. Since Begin/EndXx were introduced, they track whether an operation is in progress, and subsequent calls to BeginXx while an operation is in progress blocks synchronously. Since Read/WriteAsync just wrap the virtual Begin/End methods, they inherit this behavior. This commit addresses all three issues for CoreCLR. The Begin/EndXx methods aren't exposed from Stream in the new contracts, and as a result outside of mscorlib we don't need to be concerned about these methods being overridden. Thus, the commit adds an optimized path that simply returns the original delegate-backed task rather than wrapping it. This avoids the unnecessary task overheads and duplication, and it enables wait inlining if someone happens to wait on it. Further, since we're no longer subject to the behaviors of Begin/End, we can change the serialization to be done asynchronously rather than synchronously. --- src/mscorlib/src/System/IO/FileStream.cs | 3 + src/mscorlib/src/System/IO/Stream.cs | 111 +++++++++++++++++------ 2 files changed, 88 insertions(+), 26 deletions(-) diff --git a/src/mscorlib/src/System/IO/FileStream.cs b/src/mscorlib/src/System/IO/FileStream.cs index 3258940b5350..0952300dfb6f 100644 --- a/src/mscorlib/src/System/IO/FileStream.cs +++ b/src/mscorlib/src/System/IO/FileStream.cs @@ -1778,6 +1778,9 @@ private unsafe void WriteCore(byte[] buffer, int offset, int count) { return; } +#if FEATURE_CORECLR + internal override bool OverridesBeginEnd { get { return true; } } +#endif [System.Security.SecuritySafeCritical] // auto-generated [HostProtection(ExternalThreading = true)] diff --git a/src/mscorlib/src/System/IO/Stream.cs b/src/mscorlib/src/System/IO/Stream.cs index 64721cdf8066..9e7ae6dbb38e 100644 --- a/src/mscorlib/src/System/IO/Stream.cs +++ b/src/mscorlib/src/System/IO/Stream.cs @@ -285,15 +285,29 @@ protected virtual WaitHandle CreateWaitHandle() return new ManualResetEvent(false); } - [HostProtection(ExternalThreading=true)] + internal virtual bool OverridesBeginEnd + { + get + { +#if FEATURE_CORECLR + return false; // methods aren't exposed outside of mscorlib +#else + return true; // have to assume they are overridden as they're exposed +#endif + } + } + + [HostProtection(ExternalThreading=true)] public virtual IAsyncResult BeginRead(byte[] buffer, int offset, int count, AsyncCallback callback, Object state) { Contract.Ensures(Contract.Result() != null); - return BeginReadInternal(buffer, offset, count, callback, state, serializeAsynchronously: false); + return BeginReadInternal(buffer, offset, count, callback, state, serializeAsynchronously: false, apm: true); } [HostProtection(ExternalThreading = true)] - internal IAsyncResult BeginReadInternal(byte[] buffer, int offset, int count, AsyncCallback callback, Object state, bool serializeAsynchronously) + internal IAsyncResult BeginReadInternal( + byte[] buffer, int offset, int count, AsyncCallback callback, Object state, + bool serializeAsynchronously, bool apm) { Contract.Ensures(Contract.Result() != null); if (!CanRead) __Error.ReadNotSupported(); @@ -326,7 +340,7 @@ internal IAsyncResult BeginReadInternal(byte[] buffer, int offset, int count, As // Create the task to asynchronously do a Read. This task serves both // as the asynchronous work item and as the IAsyncResult returned to the user. - var asyncResult = new ReadWriteTask(true /*isRead*/, delegate + var asyncResult = new ReadWriteTask(true /*isRead*/, apm, delegate { // The ReadWriteTask stores all of the parameters to pass to Read. // As we're currently inside of it, we can get the current task @@ -334,10 +348,23 @@ internal IAsyncResult BeginReadInternal(byte[] buffer, int offset, int count, As var thisTask = Task.InternalCurrent as ReadWriteTask; Contract.Assert(thisTask != null, "Inside ReadWriteTask, InternalCurrent should be the ReadWriteTask"); - // Do the Read and return the number of bytes read - var bytesRead = thisTask._stream.Read(thisTask._buffer, thisTask._offset, thisTask._count); - thisTask.ClearBeginState(); // just to help alleviate some memory pressure - return bytesRead; + try + { + // Do the Read and return the number of bytes read + return thisTask._stream.Read(thisTask._buffer, thisTask._offset, thisTask._count); + } + finally + { + // If this implementation is part of Begin/EndXx, then the EndXx method will handle + // finishing the async operation. However, if this is part of XxAsync, then there won't + // be an end method, and this task is responsible for cleaning up. + if (!thisTask._apm) + { + thisTask._stream.FinishTrackingAsyncOperation(); + } + + thisTask.ClearBeginState(); // just to help alleviate some memory pressure + } }, state, this, buffer, offset, count, callback); // Schedule it @@ -388,9 +415,7 @@ public virtual int EndRead(IAsyncResult asyncResult) } finally { - _activeReadWriteTask = null; - Contract.Assert(_asyncActiveSemaphore != null, "Must have been initialized in order to get here."); - _asyncActiveSemaphore.Release(); + FinishTrackingAsyncOperation(); } #endif } @@ -414,7 +439,12 @@ public virtual Task ReadAsync(Byte[] buffer, int offset, int count, Cancell } private Task BeginEndReadAsync(Byte[] buffer, Int32 offset, Int32 count) - { + { + if (!OverridesBeginEnd) + { + return (Task)BeginReadInternal(buffer, offset, count, null, null, serializeAsynchronously: true, apm: false); + } + return TaskFactory.FromAsyncTrim( this, new ReadWriteParameters { Buffer = buffer, Offset = offset, Count = count }, (stream, args, callback, state) => stream.BeginRead(args.Buffer, args.Offset, args.Count, callback, state), // cached by compiler @@ -434,11 +464,13 @@ private Task BeginEndReadAsync(Byte[] buffer, Int32 offset, Int32 count) public virtual IAsyncResult BeginWrite(byte[] buffer, int offset, int count, AsyncCallback callback, Object state) { Contract.Ensures(Contract.Result() != null); - return BeginWriteInternal(buffer, offset, count, callback, state, serializeAsynchronously: false); + return BeginWriteInternal(buffer, offset, count, callback, state, serializeAsynchronously: false, apm: true); } [HostProtection(ExternalThreading = true)] - internal IAsyncResult BeginWriteInternal(byte[] buffer, int offset, int count, AsyncCallback callback, Object state, bool serializeAsynchronously) + internal IAsyncResult BeginWriteInternal( + byte[] buffer, int offset, int count, AsyncCallback callback, Object state, + bool serializeAsynchronously, bool apm) { Contract.Ensures(Contract.Result() != null); if (!CanWrite) __Error.WriteNotSupported(); @@ -470,7 +502,7 @@ internal IAsyncResult BeginWriteInternal(byte[] buffer, int offset, int count, A // Create the task to asynchronously do a Write. This task serves both // as the asynchronous work item and as the IAsyncResult returned to the user. - var asyncResult = new ReadWriteTask(false /*isRead*/, delegate + var asyncResult = new ReadWriteTask(false /*isRead*/, apm, delegate { // The ReadWriteTask stores all of the parameters to pass to Write. // As we're currently inside of it, we can get the current task @@ -478,10 +510,24 @@ internal IAsyncResult BeginWriteInternal(byte[] buffer, int offset, int count, A var thisTask = Task.InternalCurrent as ReadWriteTask; Contract.Assert(thisTask != null, "Inside ReadWriteTask, InternalCurrent should be the ReadWriteTask"); - // Do the Write - thisTask._stream.Write(thisTask._buffer, thisTask._offset, thisTask._count); - thisTask.ClearBeginState(); // just to help alleviate some memory pressure - return 0; // not used, but signature requires a value be returned + try + { + // Do the Write + thisTask._stream.Write(thisTask._buffer, thisTask._offset, thisTask._count); + return 0; // not used, but signature requires a value be returned + } + finally + { + // If this implementation is part of Begin/EndXx, then the EndXx method will handle + // finishing the async operation. However, if this is part of XxAsync, then there won't + // be an end method, and this task is responsible for cleaning up. + if (!thisTask._apm) + { + thisTask._stream.FinishTrackingAsyncOperation(); + } + + thisTask.ClearBeginState(); // just to help alleviate some memory pressure + } }, state, this, buffer, offset, count, callback); // Schedule it @@ -534,6 +580,13 @@ private void RunReadWriteTask(ReadWriteTask readWriteTask) readWriteTask.m_taskScheduler = TaskScheduler.Default; readWriteTask.ScheduleAndStart(needsProtection: false); } + + private void FinishTrackingAsyncOperation() + { + _activeReadWriteTask = null; + Contract.Assert(_asyncActiveSemaphore != null, "Must have been initialized in order to get here."); + _asyncActiveSemaphore.Release(); + } #endif public virtual void EndWrite(IAsyncResult asyncResult) @@ -574,9 +627,7 @@ public virtual void EndWrite(IAsyncResult asyncResult) } finally { - _activeReadWriteTask = null; - Contract.Assert(_asyncActiveSemaphore != null, "Must have been initialized in order to get here."); - _asyncActiveSemaphore.Release(); + FinishTrackingAsyncOperation(); } #endif } @@ -600,7 +651,8 @@ public virtual void EndWrite(IAsyncResult asyncResult) // with a single allocation. private sealed class ReadWriteTask : Task, ITaskCompletionAction { - internal readonly bool _isRead; + internal readonly bool _isRead; + internal readonly bool _apm; // true if this is from Begin/EndXx; false if it's from XxAsync internal Stream _stream; internal byte [] _buffer; internal int _offset; @@ -618,6 +670,7 @@ private sealed class ReadWriteTask : Task, ITaskCompletionAction [MethodImpl(MethodImplOptions.NoInlining)] public ReadWriteTask( bool isRead, + bool apm, Func function, object state, Stream stream, byte[] buffer, int offset, int count, AsyncCallback callback) : base(function, state, CancellationToken.None, TaskCreationOptions.DenyChildAttach) @@ -631,6 +684,7 @@ public ReadWriteTask( // Store the arguments _isRead = isRead; + _apm = apm; _stream = stream; _buffer = buffer; _offset = offset; @@ -708,7 +762,12 @@ public virtual Task WriteAsync(Byte[] buffer, int offset, int count, Cancellatio private Task BeginEndWriteAsync(Byte[] buffer, Int32 offset, Int32 count) - { + { + if (!OverridesBeginEnd) + { + return (Task)BeginWriteInternal(buffer, offset, count, null, null, serializeAsynchronously: true, apm: false); + } + return TaskFactory.FromAsyncTrim( this, new ReadWriteParameters { Buffer=buffer, Offset=offset, Count=count }, (stream, args, callback, state) => stream.BeginWrite(args.Buffer, args.Offset, args.Count, callback, state), // cached by compiler @@ -1220,7 +1279,7 @@ public override IAsyncResult BeginRead(byte[] buffer, int offset, int count, Asy // _stream due to this call blocked while holding the lock. return _overridesBeginRead.Value ? _stream.BeginRead(buffer, offset, count, callback, state) : - _stream.BeginReadInternal(buffer, offset, count, callback, state, serializeAsynchronously: true); + _stream.BeginReadInternal(buffer, offset, count, callback, state, serializeAsynchronously: true, apm: true); } } @@ -1278,7 +1337,7 @@ public override IAsyncResult BeginWrite(byte[] buffer, int offset, int count, As // _stream due to this call blocked while holding the lock. return _overridesBeginWrite.Value ? _stream.BeginWrite(buffer, offset, count, callback, state) : - _stream.BeginWriteInternal(buffer, offset, count, callback, state, serializeAsynchronously: true); + _stream.BeginWriteInternal(buffer, offset, count, callback, state, serializeAsynchronously: true, apm: true); } } From 4d0beabe9d6bc03d6e387b060c0d624a6ca63d9d Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Tue, 19 Jan 2016 00:04:33 -0800 Subject: [PATCH 2/4] Add generic detection of Stream.{Begin|End}{Read|Write} overrides --- src/mscorlib/src/System/IO/FileStream.cs | 3 -- src/mscorlib/src/System/IO/Stream.cs | 27 ++++++------ src/vm/comutilnative.cpp | 54 ++++++++++++++++++++++++ src/vm/comutilnative.h | 6 +++ src/vm/ecalllist.h | 6 +++ src/vm/metasig.h | 4 ++ src/vm/mscorlib.h | 4 ++ 7 files changed, 86 insertions(+), 18 deletions(-) diff --git a/src/mscorlib/src/System/IO/FileStream.cs b/src/mscorlib/src/System/IO/FileStream.cs index 0952300dfb6f..3258940b5350 100644 --- a/src/mscorlib/src/System/IO/FileStream.cs +++ b/src/mscorlib/src/System/IO/FileStream.cs @@ -1778,9 +1778,6 @@ private unsafe void WriteCore(byte[] buffer, int offset, int count) { return; } -#if FEATURE_CORECLR - internal override bool OverridesBeginEnd { get { return true; } } -#endif [System.Security.SecuritySafeCritical] // auto-generated [HostProtection(ExternalThreading = true)] diff --git a/src/mscorlib/src/System/IO/Stream.cs b/src/mscorlib/src/System/IO/Stream.cs index 9e7ae6dbb38e..de692bdd0b48 100644 --- a/src/mscorlib/src/System/IO/Stream.cs +++ b/src/mscorlib/src/System/IO/Stream.cs @@ -285,19 +285,7 @@ protected virtual WaitHandle CreateWaitHandle() return new ManualResetEvent(false); } - internal virtual bool OverridesBeginEnd - { - get - { -#if FEATURE_CORECLR - return false; // methods aren't exposed outside of mscorlib -#else - return true; // have to assume they are overridden as they're exposed -#endif - } - } - - [HostProtection(ExternalThreading=true)] + [HostProtection(ExternalThreading=true)] public virtual IAsyncResult BeginRead(byte[] buffer, int offset, int count, AsyncCallback callback, Object state) { Contract.Ensures(Contract.Result() != null); @@ -438,9 +426,13 @@ public virtual Task ReadAsync(Byte[] buffer, int offset, int count, Cancell : BeginEndReadAsync(buffer, offset, count); } + [System.Security.SecuritySafeCritical] + [MethodImplAttribute(MethodImplOptions.InternalCall)] + private extern bool HasOverridenBeginEndRead(); + private Task BeginEndReadAsync(Byte[] buffer, Int32 offset, Int32 count) { - if (!OverridesBeginEnd) + if (!HasOverridenBeginEndRead()) { return (Task)BeginReadInternal(buffer, offset, count, null, null, serializeAsynchronously: true, apm: false); } @@ -749,6 +741,8 @@ public Task WriteAsync(Byte[] buffer, int offset, int count) return WriteAsync(buffer, offset, count, CancellationToken.None); } + + [HostProtection(ExternalThreading = true)] [ComVisible(false)] public virtual Task WriteAsync(Byte[] buffer, int offset, int count, CancellationToken cancellationToken) @@ -760,10 +754,13 @@ public virtual Task WriteAsync(Byte[] buffer, int offset, int count, Cancellatio : BeginEndWriteAsync(buffer, offset, count); } + [System.Security.SecuritySafeCritical] + [MethodImplAttribute(MethodImplOptions.InternalCall)] + private extern bool HasOverridenBeginEndWrite(); private Task BeginEndWriteAsync(Byte[] buffer, Int32 offset, Int32 count) { - if (!OverridesBeginEnd) + if (!HasOverridenBeginEndWrite()) { return (Task)BeginWriteInternal(buffer, offset, count, null, null, serializeAsynchronously: true, apm: false); } diff --git a/src/vm/comutilnative.cpp b/src/vm/comutilnative.cpp index 9664bf93257d..1b21f12cdadf 100644 --- a/src/vm/comutilnative.cpp +++ b/src/vm/comutilnative.cpp @@ -3159,3 +3159,57 @@ INT32 QCALLTYPE CoreFxGlobalization::HashSortKey(PCBYTE pSortKey, INT32 cbSortKe return retVal; } #endif //FEATURE_COREFX_GLOBALIZATION + +static MethodTable * g_pStreamMT; +static WORD g_slotBeginRead, g_slotEndRead; +static WORD g_slotBeginWrite, g_slotEndWrite; + +FCIMPL1(FC_BOOL_RET, StreamNative::HasOverridenBeginEndRead, Object *stream) +{ + FCALL_CONTRACT; + + if (stream == NULL) + FC_RETURN_BOOL(TRUE); + + if (g_pStreamMT == NULL || g_slotBeginRead == 0 || g_slotEndRead == 0) + { + HELPER_METHOD_FRAME_BEGIN_RET_1(stream); + g_pStreamMT = MscorlibBinder::GetClass(CLASS__STREAM); + g_slotBeginRead = MscorlibBinder::GetMethod(METHOD__STREAM__BEGIN_READ)->GetSlot(); + g_slotEndRead = MscorlibBinder::GetMethod(METHOD__STREAM__END_READ)->GetSlot(); + HELPER_METHOD_FRAME_END(); + } + + MethodTable * pMT = stream->GetMethodTable(); + + FC_RETURN_BOOL( + pMT->GetRestoredSlot(g_slotBeginRead) != g_pStreamMT->GetRestoredSlot(g_slotBeginRead) || + pMT->GetRestoredSlot(g_slotEndRead) != g_pStreamMT->GetRestoredSlot(g_slotEndRead) + ); +} +FCIMPLEND + +FCIMPL1(FC_BOOL_RET, StreamNative::HasOverridenBeginEndWrite, Object *stream) +{ + FCALL_CONTRACT; + + if (stream == NULL) + FC_RETURN_BOOL(TRUE); + + if (g_pStreamMT == NULL || g_slotBeginWrite == 0 || g_slotEndWrite == 0) + { + HELPER_METHOD_FRAME_BEGIN_RET_1(stream); + g_pStreamMT = MscorlibBinder::GetClass(CLASS__STREAM); + g_slotBeginWrite = MscorlibBinder::GetMethod(METHOD__STREAM__BEGIN_WRITE)->GetSlot(); + g_slotEndWrite = MscorlibBinder::GetMethod(METHOD__STREAM__END_WRITE)->GetSlot(); + HELPER_METHOD_FRAME_END(); + } + + MethodTable * pMT = stream->GetMethodTable(); + + FC_RETURN_BOOL( + pMT->GetRestoredSlot(g_slotBeginWrite) != g_pStreamMT->GetRestoredSlot(g_slotBeginWrite) || + pMT->GetRestoredSlot(g_slotEndWrite) != g_pStreamMT->GetRestoredSlot(g_slotEndWrite) + ); +} +FCIMPLEND diff --git a/src/vm/comutilnative.h b/src/vm/comutilnative.h index 3a9b35a365cf..111619f25ca1 100644 --- a/src/vm/comutilnative.h +++ b/src/vm/comutilnative.h @@ -316,4 +316,10 @@ class CoreFxGlobalization { }; #endif // FEATURE_COREFX_GLOBALIZATION +class StreamNative { +public: + static FCDECL1(FC_BOOL_RET, HasOverridenBeginEndRead, Object *stream); + static FCDECL1(FC_BOOL_RET, HasOverridenBeginEndWrite, Object *stream); +}; + #endif // _COMUTILNATIVE_H_ diff --git a/src/vm/ecalllist.h b/src/vm/ecalllist.h index 504802b50d4d..7aa41efe418c 100644 --- a/src/vm/ecalllist.h +++ b/src/vm/ecalllist.h @@ -2069,6 +2069,11 @@ FCFuncStart(gVersioningHelperFuncs) FCFuncElement("GetRuntimeId", GetRuntimeId_Wrapper) FCFuncEnd() +FCFuncStart(gStreamFuncs) + FCFuncElement("HasOverridenBeginEndRead", StreamNative::HasOverridenBeginEndRead) + FCFuncElement("HasOverridenBeginEndWrite", StreamNative::HasOverridenBeginEndWrite) +FCFuncEnd() + #ifndef FEATURE_CORECLR FCFuncStart(gConsoleStreamFuncs) FCFuncElement("WaitForAvailableConsoleInput", ConsoleStreamHelper::WaitForAvailableConsoleInput) @@ -2420,6 +2425,7 @@ FCClassElement("SizedReference", "System", gSizedRefHandleFuncs) FCClassElement("StackBuilderSink", "System.Runtime.Remoting.Messaging", gStackBuilderSinkFuncs) #endif FCClassElement("StackTrace", "System.Diagnostics", gDiagnosticsStackTrace) +FCClassElement("Stream", "System.IO", gStreamFuncs) FCClassElement("String", "System", gStringFuncs) FCClassElement("StringBuilder", "System.Text", gStringBufferFuncs) FCClassElement("StringExpressionSet", "System.Security.Util", gCOMStringExpressionSetFuncs) diff --git a/src/vm/metasig.h b/src/vm/metasig.h index c64ab675e3bc..7836fbaa4770 100644 --- a/src/vm/metasig.h +++ b/src/vm/metasig.h @@ -679,6 +679,10 @@ DEFINE_METASIG_T(SM(RefCleanupWorkList_SafeHandle_RetIntPtr, r(C(CLEANUP_WORK_LI DEFINE_METASIG_T(IM(RuntimeTypeHandle_RefException_RetBool, g(RT_TYPE_HANDLE) r(C(EXCEPTION)), F)) DEFINE_METASIG_T(IM(RuntimeTypeHandle_RetRuntimeTypeHandle, g(RT_TYPE_HANDLE), g(RT_TYPE_HANDLE))) +DEFINE_METASIG_T(IM(ArrByte_Int_Int_AsyncCallback_Object_RetIAsyncResult, a(b) i i C(ASYNCCALLBACK) j, C(IASYNCRESULT))) +DEFINE_METASIG_T(IM(IAsyncResult_RetInt, C(IASYNCRESULT), i)) +DEFINE_METASIG_T(IM(IAsyncResult_RetVoid, C(IASYNCRESULT), v)) + // Undefine macros in case we include the file again in the compilation unit #undef DEFINE_METASIG diff --git a/src/vm/mscorlib.h b/src/vm/mscorlib.h index 8fb7853bb081..978b79d5b7b2 100644 --- a/src/vm/mscorlib.h +++ b/src/vm/mscorlib.h @@ -1495,6 +1495,10 @@ DEFINE_CLASS(STACK_TRACE, Diagnostics, StackTrace) DEFINE_METHOD(STACK_TRACE, GET_MANAGED_STACK_TRACE_HELPER, GetManagedStackTraceStringHelper, SM_Bool_RetStr) DEFINE_CLASS(STREAM, IO, Stream) +DEFINE_METHOD(STREAM, BEGIN_READ, BeginRead, IM_ArrByte_Int_Int_AsyncCallback_Object_RetIAsyncResult) +DEFINE_METHOD(STREAM, END_READ, EndRead, IM_IAsyncResult_RetInt) +DEFINE_METHOD(STREAM, BEGIN_WRITE, BeginWrite, IM_ArrByte_Int_Int_AsyncCallback_Object_RetIAsyncResult) +DEFINE_METHOD(STREAM, END_WRITE, EndWrite, IM_IAsyncResult_RetVoid) // Defined as element type alias // DEFINE_CLASS(INTPTR, System, IntPtr) From 0d3df25e0931c7e037cc9aa6feb35153a194cc59 Mon Sep 17 00:00:00 2001 From: stephentoub Date: Tue, 19 Jan 2016 08:14:42 -0500 Subject: [PATCH 3/4] Tweak a few more aspects of Stream.Read/WriteAsync perf improvements - Remove older reflection-based detection of overrides from SyncStream - Remove Tuple allocation from RunReadWriteTaskWhenReady - Fix spelling of Overridden - Add a few comments --- src/mscorlib/src/System/IO/Stream.cs | 77 ++++++++-------------------- src/vm/comutilnative.cpp | 4 +- src/vm/comutilnative.h | 4 +- src/vm/ecalllist.h | 4 +- 4 files changed, 28 insertions(+), 61 deletions(-) diff --git a/src/mscorlib/src/System/IO/Stream.cs b/src/mscorlib/src/System/IO/Stream.cs index de692bdd0b48..7d4f211196e4 100644 --- a/src/mscorlib/src/System/IO/Stream.cs +++ b/src/mscorlib/src/System/IO/Stream.cs @@ -428,15 +428,18 @@ public virtual Task ReadAsync(Byte[] buffer, int offset, int count, Cancell [System.Security.SecuritySafeCritical] [MethodImplAttribute(MethodImplOptions.InternalCall)] - private extern bool HasOverridenBeginEndRead(); + private extern bool HasOverriddenBeginEndRead(); private Task BeginEndReadAsync(Byte[] buffer, Int32 offset, Int32 count) { - if (!HasOverridenBeginEndRead()) + if (!HasOverriddenBeginEndRead()) { + // If the Stream does not override Begin/EndRead, then we can take an optimized path + // that skips an extra layer of tasks / IAsyncResults. return (Task)BeginReadInternal(buffer, offset, count, null, null, serializeAsynchronously: true, apm: false); } + // Otherwise, we need to wrap calls to Begin/EndWrite to ensure we use the derived type's functionality. return TaskFactory.FromAsyncTrim( this, new ReadWriteParameters { Buffer = buffer, Offset = offset, Count = count }, (stream, args, callback, state) => stream.BeginRead(args.Buffer, args.Offset, args.Count, callback, state), // cached by compiler @@ -539,7 +542,7 @@ private void RunReadWriteTaskWhenReady(Task asyncWaiter, ReadWriteTask readWrite // preconditions in async methods that await. Contract.Assert(asyncWaiter != null); // Ditto - // If the wait has already complete, run the task. + // If the wait has already completed, run the task. if (asyncWaiter.IsCompleted) { Contract.Assert(asyncWaiter.IsRanToCompletion, "The semaphore wait should always complete successfully."); @@ -547,15 +550,11 @@ private void RunReadWriteTaskWhenReady(Task asyncWaiter, ReadWriteTask readWrite } else // Otherwise, wait for our turn, and then run the task. { - asyncWaiter.ContinueWith((t, state) => - { - Contract.Assert(t.IsRanToCompletion, "The semaphore wait should always complete successfully."); - var tuple = (Tuple)state; - tuple.Item1.RunReadWriteTask(tuple.Item2); // RunReadWriteTask(readWriteTask); - }, Tuple.Create(this, readWriteTask), - default(CancellationToken), - TaskContinuationOptions.ExecuteSynchronously, - TaskScheduler.Default); + asyncWaiter.ContinueWith((t, state) => { + Contract.Assert(t.IsRanToCompletion, "The semaphore wait should always complete successfully."); + var rwt = (ReadWriteTask)state; + rwt._stream.RunReadWriteTask(rwt); // RunReadWriteTask(readWriteTask); + }, readWriteTask, default(CancellationToken), TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default); } } @@ -647,8 +646,8 @@ private sealed class ReadWriteTask : Task, ITaskCompletionAction internal readonly bool _apm; // true if this is from Begin/EndXx; false if it's from XxAsync internal Stream _stream; internal byte [] _buffer; - internal int _offset; - internal int _count; + internal readonly int _offset; + internal readonly int _count; private AsyncCallback _callback; private ExecutionContext _context; @@ -756,15 +755,18 @@ public virtual Task WriteAsync(Byte[] buffer, int offset, int count, Cancellatio [System.Security.SecuritySafeCritical] [MethodImplAttribute(MethodImplOptions.InternalCall)] - private extern bool HasOverridenBeginEndWrite(); + private extern bool HasOverriddenBeginEndWrite(); private Task BeginEndWriteAsync(Byte[] buffer, Int32 offset, Int32 count) { - if (!HasOverridenBeginEndWrite()) + if (!HasOverriddenBeginEndWrite()) { + // If the Stream does not override Begin/EndWrite, then we can take an optimized path + // that skips an extra layer of tasks / IAsyncResults. return (Task)BeginWriteInternal(buffer, offset, count, null, null, serializeAsynchronously: true, apm: false); } + // Otherwise, we need to wrap calls to Begin/EndWrite to ensure we use the derived type's functionality. return TaskFactory.FromAsyncTrim( this, new ReadWriteParameters { Buffer=buffer, Offset=offset, Count=count }, (stream, args, callback, state) => stream.BeginWrite(args.Buffer, args.Offset, args.Count, callback, state), // cached by compiler @@ -1111,10 +1113,6 @@ internal static void EndWrite(IAsyncResult asyncResult) { internal sealed class SyncStream : Stream, IDisposable { private Stream _stream; - [NonSerialized] - private bool? _overridesBeginRead; - [NonSerialized] - private bool? _overridesBeginWrite; internal SyncStream(Stream stream) { @@ -1233,38 +1231,11 @@ public override int ReadByte() lock(_stream) return _stream.ReadByte(); } - - private static bool OverridesBeginMethod(Stream stream, string methodName) - { - Contract.Requires(stream != null, "Expected a non-null stream."); - Contract.Requires(methodName == "BeginRead" || methodName == "BeginWrite", - "Expected BeginRead or BeginWrite as the method name to check."); - - // Get all of the methods on the underlying stream - var methods = stream.GetType().GetMethods(BindingFlags.Public | BindingFlags.Instance); - - // If any of the methods have the desired name and are defined on the base Stream - // Type, then the method was not overridden. If none of them were defined on the - // base Stream, then it must have been overridden. - foreach (var method in methods) - { - if (method.DeclaringType == typeof(Stream) && - method.Name == methodName) - { - return false; - } - } - return true; - } [HostProtection(ExternalThreading=true)] public override IAsyncResult BeginRead(byte[] buffer, int offset, int count, AsyncCallback callback, Object state) { - // Lazily-initialize whether the wrapped stream overrides BeginRead - if (_overridesBeginRead == null) - { - _overridesBeginRead = OverridesBeginMethod(_stream, "BeginRead"); - } + bool overridesBeginRead = _stream.HasOverriddenBeginEndRead(); lock (_stream) { @@ -1274,7 +1245,7 @@ public override IAsyncResult BeginRead(byte[] buffer, int offset, int count, Asy // than a synchronous wait. A synchronous wait will result in a deadlock condition, because // the EndXx method for the outstanding async operation won't be able to acquire the lock on // _stream due to this call blocked while holding the lock. - return _overridesBeginRead.Value ? + return overridesBeginRead ? _stream.BeginRead(buffer, offset, count, callback, state) : _stream.BeginReadInternal(buffer, offset, count, callback, state, serializeAsynchronously: true, apm: true); } @@ -1318,11 +1289,7 @@ public override void WriteByte(byte b) [HostProtection(ExternalThreading=true)] public override IAsyncResult BeginWrite(byte[] buffer, int offset, int count, AsyncCallback callback, Object state) { - // Lazily-initialize whether the wrapped stream overrides BeginWrite - if (_overridesBeginWrite == null) - { - _overridesBeginWrite = OverridesBeginMethod(_stream, "BeginWrite"); - } + bool overridesBeginWrite = _stream.HasOverriddenBeginEndWrite(); lock (_stream) { @@ -1332,7 +1299,7 @@ public override IAsyncResult BeginWrite(byte[] buffer, int offset, int count, As // than a synchronous wait. A synchronous wait will result in a deadlock condition, because // the EndXx method for the outstanding async operation won't be able to acquire the lock on // _stream due to this call blocked while holding the lock. - return _overridesBeginWrite.Value ? + return overridesBeginWrite ? _stream.BeginWrite(buffer, offset, count, callback, state) : _stream.BeginWriteInternal(buffer, offset, count, callback, state, serializeAsynchronously: true, apm: true); } diff --git a/src/vm/comutilnative.cpp b/src/vm/comutilnative.cpp index 1b21f12cdadf..84ab62cbc340 100644 --- a/src/vm/comutilnative.cpp +++ b/src/vm/comutilnative.cpp @@ -3164,7 +3164,7 @@ static MethodTable * g_pStreamMT; static WORD g_slotBeginRead, g_slotEndRead; static WORD g_slotBeginWrite, g_slotEndWrite; -FCIMPL1(FC_BOOL_RET, StreamNative::HasOverridenBeginEndRead, Object *stream) +FCIMPL1(FC_BOOL_RET, StreamNative::HasOverriddenBeginEndRead, Object *stream) { FCALL_CONTRACT; @@ -3189,7 +3189,7 @@ FCIMPL1(FC_BOOL_RET, StreamNative::HasOverridenBeginEndRead, Object *stream) } FCIMPLEND -FCIMPL1(FC_BOOL_RET, StreamNative::HasOverridenBeginEndWrite, Object *stream) +FCIMPL1(FC_BOOL_RET, StreamNative::HasOverriddenBeginEndWrite, Object *stream) { FCALL_CONTRACT; diff --git a/src/vm/comutilnative.h b/src/vm/comutilnative.h index 111619f25ca1..21d7b91823b8 100644 --- a/src/vm/comutilnative.h +++ b/src/vm/comutilnative.h @@ -318,8 +318,8 @@ class CoreFxGlobalization { class StreamNative { public: - static FCDECL1(FC_BOOL_RET, HasOverridenBeginEndRead, Object *stream); - static FCDECL1(FC_BOOL_RET, HasOverridenBeginEndWrite, Object *stream); + static FCDECL1(FC_BOOL_RET, HasOverriddenBeginEndRead, Object *stream); + static FCDECL1(FC_BOOL_RET, HasOverriddenBeginEndWrite, Object *stream); }; #endif // _COMUTILNATIVE_H_ diff --git a/src/vm/ecalllist.h b/src/vm/ecalllist.h index 7aa41efe418c..9461deff138e 100644 --- a/src/vm/ecalllist.h +++ b/src/vm/ecalllist.h @@ -2070,8 +2070,8 @@ FCFuncStart(gVersioningHelperFuncs) FCFuncEnd() FCFuncStart(gStreamFuncs) - FCFuncElement("HasOverridenBeginEndRead", StreamNative::HasOverridenBeginEndRead) - FCFuncElement("HasOverridenBeginEndWrite", StreamNative::HasOverridenBeginEndWrite) + FCFuncElement("HasOverriddenBeginEndRead", StreamNative::HasOverriddenBeginEndRead) + FCFuncElement("HasOverriddenBeginEndWrite", StreamNative::HasOverriddenBeginEndWrite) FCFuncEnd() #ifndef FEATURE_CORECLR From d88d221c1f2ba0a7c12229e09ddefd4cbf64bb4a Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Tue, 19 Jan 2016 07:41:19 -0800 Subject: [PATCH 4/4] Fix override detection to work reliably for JITed mscorlib --- src/vm/comutilnative.cpp | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/src/vm/comutilnative.cpp b/src/vm/comutilnative.cpp index 84ab62cbc340..b3f267028215 100644 --- a/src/vm/comutilnative.cpp +++ b/src/vm/comutilnative.cpp @@ -3164,6 +3164,31 @@ static MethodTable * g_pStreamMT; static WORD g_slotBeginRead, g_slotEndRead; static WORD g_slotBeginWrite, g_slotEndWrite; +static bool HasOverriddenStreamMethod(MethodTable * pMT, WORD slot) +{ + CONTRACTL{ + NOTHROW; + GC_NOTRIGGER; + MODE_ANY; + SO_TOLERANT; + } CONTRACTL_END; + + PCODE actual = pMT->GetRestoredSlot(slot); + PCODE base = g_pStreamMT->GetRestoredSlot(slot); + if (actual == base) + return false; + + if (!g_pStreamMT->IsZapped()) + { + // If mscorlib is JITed, the slots can be patched and thus we need to compare the actual MethodDescs + // to detect match reliably + if (MethodTable::GetMethodDescForSlotAddress(actual) == MethodTable::GetMethodDescForSlotAddress(base)) + return false; + } + + return true; +} + FCIMPL1(FC_BOOL_RET, StreamNative::HasOverriddenBeginEndRead, Object *stream) { FCALL_CONTRACT; @@ -3182,10 +3207,7 @@ FCIMPL1(FC_BOOL_RET, StreamNative::HasOverriddenBeginEndRead, Object *stream) MethodTable * pMT = stream->GetMethodTable(); - FC_RETURN_BOOL( - pMT->GetRestoredSlot(g_slotBeginRead) != g_pStreamMT->GetRestoredSlot(g_slotBeginRead) || - pMT->GetRestoredSlot(g_slotEndRead) != g_pStreamMT->GetRestoredSlot(g_slotEndRead) - ); + FC_RETURN_BOOL(HasOverriddenStreamMethod(pMT, g_slotBeginRead) || HasOverriddenStreamMethod(pMT, g_slotEndRead)); } FCIMPLEND @@ -3207,9 +3229,6 @@ FCIMPL1(FC_BOOL_RET, StreamNative::HasOverriddenBeginEndWrite, Object *stream) MethodTable * pMT = stream->GetMethodTable(); - FC_RETURN_BOOL( - pMT->GetRestoredSlot(g_slotBeginWrite) != g_pStreamMT->GetRestoredSlot(g_slotBeginWrite) || - pMT->GetRestoredSlot(g_slotEndWrite) != g_pStreamMT->GetRestoredSlot(g_slotEndWrite) - ); + FC_RETURN_BOOL(HasOverriddenStreamMethod(pMT, g_slotBeginWrite) || HasOverriddenStreamMethod(pMT, g_slotEndWrite)); } FCIMPLEND