From edb58d134d65b71527fdf96b2666477a74f223a3 Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Wed, 6 Jun 2018 10:15:19 -0700 Subject: [PATCH] Moves FastResourceComparer and TaskAwaiter to shared (dotnet/coreclr#18283) * Reduces diff in BinaryReader * Moves FastResourceComparer to shared * Reduces diff in FileBasedResourceGroveler * Minor comment diff in ManifestBasedResourceGroveler * Reducing diff by removing extra blank lines in ResourceReader * Reduces diff by renaming private fields in AssemblyName * String to string and Object to object in AssemblyName * Reduces diff in TaskAwaiter * Moves TaskAwaiter to shared * Apply code review feedback * Fixes failure in corert * Revert Renaming fields in AssemblyName Commit migrated from https://github.com/dotnet/coreclr/commit/39da0af098b86b55e44bd1e42fefb2b2b6a8ab67 --- .../System.Private.CoreLib.csproj | 2 - .../src/System/IO/BinaryReader.cs | 114 +++++++++++++++++---- .../src/System/Reflection/AssemblyName.cs | 36 +++---- .../System/Resources/FileBasedResourceGroveler.cs | 1 - .../Resources/ManifestBasedResourceGroveler.cs | 2 +- .../src/System/Resources/ResourceReader.cs | 8 -- .../src/System.Private.CoreLib.Shared.projitems | 2 + .../src/System/Resources/FastResourceComparer.cs} | 0 .../System/Runtime/CompilerServices/TaskAwaiter.cs | 53 ++++++++-- 9 files changed, 160 insertions(+), 58 deletions(-) rename src/{coreclr/src/System.Private.CoreLib/src/System/Resources/__FastResourceComparer.cs => libraries/System.Private.CoreLib/src/System/Resources/FastResourceComparer.cs} (100%) rename src/{coreclr/src => libraries}/System.Private.CoreLib/src/System/Runtime/CompilerServices/TaskAwaiter.cs (95%) diff --git a/src/coreclr/src/System.Private.CoreLib/System.Private.CoreLib.csproj b/src/coreclr/src/System.Private.CoreLib/System.Private.CoreLib.csproj index f4a0d64..f23d5eb 100644 --- a/src/coreclr/src/System.Private.CoreLib/System.Private.CoreLib.csproj +++ b/src/coreclr/src/System.Private.CoreLib/System.Private.CoreLib.csproj @@ -124,7 +124,6 @@ - @@ -548,7 +547,6 @@ - diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/IO/BinaryReader.cs b/src/coreclr/src/System.Private.CoreLib/src/System/IO/BinaryReader.cs index 6e0f54b..04fe45d 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/IO/BinaryReader.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/IO/BinaryReader.cs @@ -60,13 +60,19 @@ namespace System.IO throw new ArgumentNullException(nameof(encoding)); } if (!input.CanRead) + { throw new ArgumentException(SR.Argument_StreamNotReadable); + } + _stream = input; _decoder = encoding.GetDecoder(); _maxCharsSize = encoding.GetMaxCharCount(MaxCharBytesSize); int minBufferSize = encoding.GetMaxByteCount(1); // max bytes per one char if (minBufferSize < 16) + { minBufferSize = 16; + } + _buffer = new byte[minBufferSize]; // _charBuffer and _charBytes will be left null. @@ -89,11 +95,6 @@ namespace System.IO } } - public virtual void Close() - { - Dispose(true); - } - protected virtual void Dispose(bool disposing) { if (disposing) @@ -101,7 +102,9 @@ namespace System.IO Stream copyOfStream = _stream; _stream = null; if (copyOfStream != null && !_leaveOpen) + { copyOfStream.Close(); + } } _stream = null; _buffer = null; @@ -116,12 +119,26 @@ namespace System.IO Dispose(true); } + /// + /// Override Dispose(bool) instead of Close(). This API exists for compatibility purposes. + /// + public virtual void Close() + { + Dispose(true); + } + public virtual int PeekChar() { - if (_stream == null) throw Error.GetFileNotOpen(); + if (_stream == null) + { + throw Error.GetFileNotOpen(); + } if (!_stream.CanSeek) + { return -1; + } + long origPos = _stream.Position; int ch = Read(); _stream.Position = origPos; @@ -146,11 +163,17 @@ namespace System.IO public virtual byte ReadByte() { // Inlined to avoid some method call overhead with FillBuffer. - if (_stream == null) throw Error.GetFileNotOpen(); + if (_stream == null) + { + throw Error.GetFileNotOpen(); + } int b = _stream.ReadByte(); if (b == -1) + { throw Error.GetEndOfFile(); + } + return (byte)b; } @@ -188,7 +211,11 @@ namespace System.IO { if (_isMemoryStream) { - if (_stream == null) throw Error.GetFileNotOpen(); + if (_stream == null) + { + throw Error.GetFileNotOpen(); + } + // read directly from MemoryStream buffer MemoryStream mStream = _stream as MemoryStream; Debug.Assert(mStream != null, "_stream as MemoryStream != null"); @@ -266,7 +293,9 @@ namespace System.IO public virtual string ReadString() { if (_stream == null) + { throw Error.GetFileNotOpen(); + } int currPos = 0; int n; @@ -310,10 +339,15 @@ namespace System.IO charsRead = _decoder.GetChars(_charBytes, 0, n, _charBuffer, 0); if (currPos == 0 && n == stringLength) + { return new string(_charBuffer, 0, charsRead); + } if (sb == null) + { sb = StringBuilderCache.Acquire(stringLength); // Actual string length in chars may be smaller. + } + sb.Append(_charBuffer, 0, charsRead); currPos += n; } while (currPos < stringLength); @@ -339,9 +373,10 @@ namespace System.IO { throw new ArgumentException(SR.Argument_InvalidOffLen); } - if (_stream == null) + { throw Error.GetFileNotOpen(); + } // SafeCritical: index and count have already been verified to be a valid range for the buffer return InternalReadChars(new Span(buffer, index, count)); @@ -350,7 +385,9 @@ namespace System.IO public virtual int Read(Span buffer) { if (_stream == null) + { throw Error.GetFileNotOpen(); + } return InternalReadChars(buffer); } @@ -386,7 +423,9 @@ namespace System.IO if (_2BytesPerChar) numBytes <<= 1; if (numBytes > MaxCharBytesSize) + { numBytes = MaxCharBytesSize; + } int position = 0; byte[] byteBuffer = null; @@ -411,7 +450,6 @@ namespace System.IO } Debug.Assert(byteBuffer != null, "expected byteBuffer to be non-null"); - checked { if (position < 0 || numBytes < 0 || position > byteBuffer.Length - numBytes) @@ -455,11 +493,13 @@ namespace System.IO long posSav = posSav = 0; if (_stream.CanSeek) + { posSav = _stream.Position; + } if (_charBytes == null) { - _charBytes = new byte[MaxCharBytesSize]; + _charBytes = new byte[MaxCharBytesSize]; //REVIEW: We need at most 2 bytes/char here? } if (_singleChar == null) { @@ -477,13 +517,17 @@ namespace System.IO int r = _stream.ReadByte(); _charBytes[0] = (byte)r; if (r == -1) + { numBytes = 0; + } if (numBytes == 2) { r = _stream.ReadByte(); _charBytes[1] = (byte)r; if (r == -1) + { numBytes = 1; + } } if (numBytes == 0) @@ -503,7 +547,9 @@ namespace System.IO // Handle surrogate char if (_stream.CanSeek) + { _stream.Seek((posSav - _stream.Position), SeekOrigin.Current); + } // else - we can't do much here throw; @@ -530,7 +576,7 @@ namespace System.IO if (count == 0) { - return Array.Empty(); + return Array.Empty(); } // SafeCritical: we own the chars buffer, and therefore can guarantee that the index and count are valid @@ -549,44 +595,65 @@ namespace System.IO public virtual int Read(byte[] buffer, int index, int count) { if (buffer == null) + { throw new ArgumentNullException(nameof(buffer), SR.ArgumentNull_Buffer); + } if (index < 0) + { throw new ArgumentOutOfRangeException(nameof(index), SR.ArgumentOutOfRange_NeedNonNegNum); + } if (count < 0) + { throw new ArgumentOutOfRangeException(nameof(count), SR.ArgumentOutOfRange_NeedNonNegNum); + } if (buffer.Length - index < count) + { throw new ArgumentException(SR.Argument_InvalidOffLen); + } + if (_stream == null) + { + throw Error.GetFileNotOpen(); + } - if (_stream == null) throw Error.GetFileNotOpen(); return _stream.Read(buffer, index, count); } public virtual int Read(Span buffer) { if (_stream == null) + { throw Error.GetFileNotOpen(); + } return _stream.Read(buffer); } public virtual byte[] ReadBytes(int count) { - if (count < 0) throw new ArgumentOutOfRangeException(nameof(count), SR.ArgumentOutOfRange_NeedNonNegNum); - if (_stream == null) throw Error.GetFileNotOpen(); + if (count < 0) + { + throw new ArgumentOutOfRangeException(nameof(count), SR.ArgumentOutOfRange_NeedNonNegNum); + } + if (_stream == null) + { + throw new ObjectDisposedException(null, SR.ObjectDisposed_FileClosed); + } if (count == 0) { - return Array.Empty(); + return Array.Empty(); } byte[] result = new byte[count]; - int numRead = 0; do { int n = _stream.Read(result, numRead, count); if (n == 0) + { break; + } + numRead += n; count -= n; } while (count > 0); @@ -608,10 +675,14 @@ namespace System.IO { throw new ArgumentOutOfRangeException(nameof(numBytes), SR.ArgumentOutOfRange_BinaryReaderFillBuffer); } + int bytesRead = 0; int n = 0; - if (_stream == null) throw Error.GetFileNotOpen(); + if (_stream == null) + { + throw Error.GetFileNotOpen(); + } // Need to find a good threshold for calling ReadByte() repeatedly // vs. calling Read(byte[], int, int) for both buffered & unbuffered @@ -620,7 +691,10 @@ namespace System.IO { n = _stream.ReadByte(); if (n == -1) + { throw Error.GetEndOfFile(); + } + _buffer[0] = (byte)n; return; } @@ -636,7 +710,7 @@ namespace System.IO } while (bytesRead < numBytes); } - internal protected int Read7BitEncodedInt() + protected internal int Read7BitEncodedInt() { // Read out an Int32 7 bits at a time. The high bit // of the byte when on means to continue reading more bytes. @@ -648,7 +722,9 @@ namespace System.IO // Check for a corrupted stream. Read a max of 5 bytes. // In a future version, add a DataFormatException. if (shift == 5 * 7) // 5 bytes max per Int32, shift += 7 + { throw new FormatException(SR.Format_Bad7BitInt32); + } // ReadByte handles end of stream cases for us. b = ReadByte(); diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/AssemblyName.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/AssemblyName.cs index 9a6728c..4da56be 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/AssemblyName.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/AssemblyName.cs @@ -33,11 +33,11 @@ namespace System.Reflection // If you modify any of these fields, you must also update the // AssemblyBaseObject structure in object.h // - private String _Name; // Name + private string _Name; // Name private byte[] _PublicKey; private byte[] _PublicKeyToken; private CultureInfo _CultureInfo; - private String _CodeBase; // Potential location to get the file + private string _CodeBase; // Potential location to get the file private Version _Version; private StrongNameKeyPair _StrongNameKeyPair; @@ -61,7 +61,7 @@ namespace System.Reflection // Set and get the name of the assembly. If this is a weak Name // then it optionally contains a site. For strong assembly names, // the name partitions up the strong name's namespace - public String Name + public string Name { get { return _Name; } set { _Name = value; } @@ -92,7 +92,7 @@ namespace System.Reflection } } - public String CultureName + public string CultureName { get { @@ -104,13 +104,13 @@ namespace System.Reflection } } - public String CodeBase + public string CodeBase { get { return _CodeBase; } set { _CodeBase = value; } } - public String EscapedCodeBase + public string EscapedCodeBase { get { @@ -164,7 +164,7 @@ namespace System.Reflection // Make a copy of this assembly name. - public Object Clone() + public object Clone() { AssemblyName name = new AssemblyName(); name.Init(_Name, @@ -187,7 +187,7 @@ namespace System.Reflection * if the file contains an assembly manifest. This method causes * the file to be opened and closed. */ - public static AssemblyName GetAssemblyName(String assemblyFile) + public static AssemblyName GetAssemblyName(string assemblyFile) { if (assemblyFile == null) throw new ArgumentNullException(nameof(assemblyFile)); @@ -272,7 +272,7 @@ namespace System.Reflection set { _StrongNameKeyPair = value; } } - public String FullName + public string FullName { get { @@ -285,9 +285,9 @@ namespace System.Reflection } // Returns the stringized version of the assembly name. - public override String ToString() + public override string ToString() { - String s = FullName; + string s = FullName; if (s == null) return base.ToString(); else @@ -299,12 +299,12 @@ namespace System.Reflection throw new PlatformNotSupportedException(); } - public void OnDeserialization(Object sender) + public void OnDeserialization(object sender) { throw new PlatformNotSupportedException(); } - public AssemblyName(String assemblyName) + public AssemblyName(string assemblyName) { if (assemblyName == null) throw new ArgumentNullException(nameof(assemblyName)); @@ -390,14 +390,14 @@ namespace System.Reflection return ProcessorArchitecture.None; } - internal void Init(String name, + internal void Init(string name, byte[] publicKey, byte[] publicKeyToken, Version version, CultureInfo cultureInfo, AssemblyHashAlgorithm hashAlgorithm, AssemblyVersionCompatibility versionCompatibility, - String codeBase, + string codeBase, AssemblyNameFlags flags, StrongNameKeyPair keyPair) // Null if ref, matching Assembly if def { @@ -429,12 +429,12 @@ namespace System.Reflection // This call opens and closes the file, but does not add the // assembly to the domain. [MethodImplAttribute(MethodImplOptions.InternalCall)] - internal static extern AssemblyName nGetFileInformation(String s); + internal static extern AssemblyName nGetFileInformation(string s); [MethodImplAttribute(MethodImplOptions.InternalCall)] private extern byte[] nGetPublicKeyToken(); - internal static String EscapeCodeBase(String codebase) + internal static string EscapeCodeBase(string codebase) { if (codebase == null) return string.Empty; @@ -500,7 +500,7 @@ namespace System.Reflection c_MaxUnicodeCharsReallocate * c_MaxUTF_8BytesPerUnicodeChar); // This is the only exception that built in UriParser can throw after a Uri ctor. - // Should not happen unless the app tries to feed an invalid Unicode String + // Should not happen unless the app tries to feed an invalid Unicode string if (numberOfBytes == 0) throw new FormatException(SR.Arg_FormatException); diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Resources/FileBasedResourceGroveler.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Resources/FileBasedResourceGroveler.cs index 1546c5e..12e0832 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/Resources/FileBasedResourceGroveler.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/Resources/FileBasedResourceGroveler.cs @@ -42,7 +42,6 @@ namespace System.Resources ResourceSet rs = null; // Don't use Assembly manifest, but grovel on disk for a file. - // Create new ResourceSet, if a file exists on disk for it. String tempFileName = _mediator.GetResourceFileName(culture); fileName = FindResourceFile(culture, tempFileName); diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Resources/ManifestBasedResourceGroveler.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Resources/ManifestBasedResourceGroveler.cs index 9e28e7b..0248892 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/Resources/ManifestBasedResourceGroveler.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/Resources/ManifestBasedResourceGroveler.cs @@ -430,7 +430,7 @@ namespace System.Resources } // Perf optimization - Don't use Reflection for most cases with - // our .resources files. This makes our code run faster and we can + // our .resources files. This makes our code run faster and we can avoid // creating a ResourceReader via Reflection. This would incur // a security check (since the link-time check on the constructor that // takes a String is turned into a full demand with a stack walk) diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Resources/ResourceReader.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Resources/ResourceReader.cs index 6b9732d..f22d3b8 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/Resources/ResourceReader.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/Resources/ResourceReader.cs @@ -70,7 +70,6 @@ namespace System.Resources } } - public sealed class ResourceReader : IResourceReader { // A reasonable default buffer size for reading from files, especially @@ -453,7 +452,6 @@ namespace System.Resources { throw new FormatException(SR.Format(SR.BadImageFormat_ResourcesDataInvalidOffset, dataPos)); } - ResourceTypeCode junk; if (_version == 1) return LoadObjectV1(dataPos); @@ -495,7 +493,6 @@ namespace System.Resources if (typeCode == ResourceTypeCode.String) // ignore Null s = _store.ReadString(); } - return s; } @@ -749,8 +746,6 @@ namespace System.Resources throw new NotSupportedException(SR.NotSupported_ResourceObjectSerialization); } - - // Reads in the header information for a .resources file. Verifies some // of the assumptions about this resource set, and builds the class table // for the default resource file format. @@ -974,7 +969,6 @@ namespace System.Resources return _typeTable[typeIndex]; } - public void GetResourceData(string resourceName, out string resourceType, out byte[] resourceData) { if (resourceName == null) @@ -1066,8 +1060,6 @@ namespace System.Resources } } - - internal sealed class ResourceEnumerator : IDictionaryEnumerator { private const int ENUM_DONE = Int32.MinValue; diff --git a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems index 2021639..26dfbd8 100644 --- a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems +++ b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems @@ -379,6 +379,7 @@ + @@ -435,6 +436,7 @@ + diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Resources/__FastResourceComparer.cs b/src/libraries/System.Private.CoreLib/src/System/Resources/FastResourceComparer.cs similarity index 100% rename from src/coreclr/src/System.Private.CoreLib/src/System/Resources/__FastResourceComparer.cs rename to src/libraries/System.Private.CoreLib/src/System/Resources/FastResourceComparer.cs diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/TaskAwaiter.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/TaskAwaiter.cs similarity index 95% rename from src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/TaskAwaiter.cs rename to src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/TaskAwaiter.cs index 3d72798..493d984 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/TaskAwaiter.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/TaskAwaiter.cs @@ -37,13 +37,13 @@ // // =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- -using System; using System.Diagnostics; -using System.Diagnostics.CodeAnalysis; -using System.Security; +using System.Diagnostics.Tracing; using System.Threading; using System.Threading.Tasks; -using System.Diagnostics.Tracing; +#if !CORECLR +using Internal.Threading.Tasks.Tracing; +#endif // NOTE: For performance reasons, initialization is not verified. If a developer // incorrectly initializes a task awaiter, which should only be done by the compiler, @@ -209,7 +209,13 @@ namespace System.Runtime.CompilerServices // If TaskWait* ETW events are enabled, trace a beginning event for this await // and set up an ending event to be traced when the asynchronous await completes. - if (TplEtwProvider.Log.IsEnabled() || Task.s_asyncDebuggingEnabled) + if ( +#if CORECLR + TplEtwProvider.Log.IsEnabled() || Task.s_asyncDebuggingEnabled +#else + TaskTrace.Enabled +#endif + ) { continuation = OutputWaitEtwEvents(task, continuation); } @@ -218,6 +224,7 @@ namespace System.Runtime.CompilerServices task.SetContinuationForAwait(continuation, continueOnCapturedContext, flowExecutionContext); } +#if CORECLR /// Schedules the continuation onto the associated with this . /// The task being awaited. /// The action to invoke when the await operation completes. @@ -241,7 +248,7 @@ namespace System.Runtime.CompilerServices task.UnsafeSetContinuationForAwait(stateMachineBox, continueOnCapturedContext); } } - +#endif /// /// Outputs a WaitBegin ETW event, and augments the continuation action to output a WaitEnd ETW event. /// @@ -252,7 +259,7 @@ namespace System.Runtime.CompilerServices { Debug.Assert(task != null, "Need a task to wait on"); Debug.Assert(continuation != null, "Need a continuation to invoke when the wait completes"); - +#if CORECLR if (Task.s_asyncDebuggingEnabled) { Task.AddToActiveTasks(task); @@ -273,12 +280,23 @@ namespace System.Runtime.CompilerServices task.Id, TplEtwProvider.TaskWaitBehavior.Asynchronous, (continuationTask != null ? continuationTask.Id : 0)); } +#else + Debug.Assert(TaskTrace.Enabled, "Should only be used when ETW tracing is enabled"); + + // ETW event for Task Wait Begin + var currentTaskAtBegin = Task.InternalCurrent; + TaskTrace.TaskWaitBegin_Asynchronous( + (currentTaskAtBegin != null ? currentTaskAtBegin.m_taskScheduler.Id : TaskScheduler.Default.Id), + (currentTaskAtBegin != null ? currentTaskAtBegin.Id : 0), + task.Id); +#endif // Create a continuation action that outputs the end event and then invokes the user // provided delegate. This incurs the allocations for the closure/delegate, but only if the event // is enabled, and in doing so it allows us to pass the awaited task's information into the end event // in a purely pay-for-play manner (the alternatively would be to increase the size of TaskAwaiter // just for this ETW purpose, not pay-for-play, since GetResult would need to know whether a real yield occurred). +#if CORECLR return AsyncMethodBuilderCore.CreateContinuationWrapper(continuation, (innerContinuation,innerTask) => { if (Task.s_asyncDebuggingEnabled) @@ -315,6 +333,23 @@ namespace System.Runtime.CompilerServices EventSource.SetCurrentThreadActivityId(prevActivityId); } }, task); +#else + return () => + { + // ETW event for Task Wait End. + if (TaskTrace.Enabled) + { + var currentTaskAtEnd = Task.InternalCurrent; + TaskTrace.TaskWaitEnd( + (currentTaskAtEnd != null ? currentTaskAtEnd.m_taskScheduler.Id : TaskScheduler.Default.Id), + (currentTaskAtEnd != null ? currentTaskAtEnd.Id : 0), + task.Id); + } + + // Invoke the original continuation provided to OnCompleted. + continuation(); + }; +#endif } } @@ -424,9 +459,9 @@ namespace System.Runtime.CompilerServices // Its layout must remain the same. /// The task being awaited. - internal readonly Task m_task; + internal readonly Task m_task; /// Whether to attempt marshaling back to the original context. - internal readonly bool m_continueOnCapturedContext; + internal readonly bool m_continueOnCapturedContext; /// Initializes the . /// The to await. -- 2.7.4