Skip to content
This repository was archived by the owner on Aug 1, 2024. It is now read-only.

Avoid Task.Run and Factory.StartNew with async #581

Merged
merged 9 commits into from
Dec 21, 2017
Merged

Avoid Task.Run and Factory.StartNew with async #581

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
82f4d4e
CloudQueue and CloudQueueClient now properly respect the async best p…
danielmarbach Nov 24, 2017
f47d023
Executor now properly respects the async best practices
danielmarbach Nov 24, 2017
94328f3
TableQuery and TableQueryGeneric now properly respect the async best …
danielmarbach Nov 24, 2017
803881a
CloudAppendBlob, CloudBlob and CloudBlobClient now properly respect t…
danielmarbach Nov 24, 2017
fb4a499
Removed unnecessary task scheduling inside HttpResponseAdapterMessage…
danielmarbach Nov 27, 2017
8668ce3
Blob now properly respect the async best practices
danielmarbach Nov 28, 2017
90f7b27
File now properly respect the async best practices
danielmarbach Nov 28, 2017
b76e368
Table now properly respect the async best practices
danielmarbach Nov 28, 2017
20ed414
Missing ConfigureAwait
danielmarbach Nov 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Lib/AspNet/Microsoft.WindowsAzure.Storage/Extensions/StreamExtension.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,9 @@ public static Stream AsStreamForWrite(this Stream stream)
/// <param name="stream">input stream</param>
/// <param name="buffer">buffer to write to the stream</param>
/// <returns>Async task</returns>
public static async Task WriteAsync(this Stream stream, byte[] buffer)
public static Task WriteAsync(this Stream stream, byte[] buffer)
{
await stream.WriteAsync(buffer, 0, buffer.Length);
return stream.WriteAsync(buffer, 0, buffer.Length);
}

/// <summary>
Expand Down
8 changes: 4 additions & 4 deletions Lib/ClassLibraryCommon/Table/Protocol/HttpResponseAdapterMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ namespace Microsoft.WindowsAzure.Storage.Table.Protocol
internal class HttpResponseAdapterMessage : IODataResponseMessage
{
private HttpWebResponse resp = null;
private Stream str = null;
private Task<Stream> strAsCachedTask = null;
private string responseContentType = null;

public HttpResponseAdapterMessage(HttpWebResponse resp, Stream str)
Expand All @@ -38,13 +38,13 @@ public HttpResponseAdapterMessage(HttpWebResponse resp, Stream str)
public HttpResponseAdapterMessage(HttpWebResponse resp, Stream str, string responseContentType)
{
this.resp = resp;
this.str = str;
this.strAsCachedTask = Task.FromResult(str);
this.responseContentType = responseContentType;
}

public Task<Stream> GetStreamAsync()
{
return Task.Factory.StartNew(() => this.str);
return this.strAsCachedTask;
}

public string GetHeader(string headerName)
Expand Down Expand Up @@ -73,7 +73,7 @@ public string GetHeader(string headerName)

public Stream GetStream()
{
return this.str;
return this.strAsCachedTask.Result; // safe since completed task and avoids additional field for stream
}

public IEnumerable<KeyValuePair<string, string>> Headers
Expand Down
2 changes: 1 addition & 1 deletion Lib/Common/Core/MultiBufferMemoryStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,7 +572,7 @@ public async Task FastCopyToAsync(Stream destination, DateTime? expiryTime)

// Copy the block
int blockReadLength = (int)Math.Min(leftToRead, currentBlock.Count);
await destination.WriteAsync(currentBlock.Array, currentBlock.Offset, blockReadLength);
await destination.WriteAsync(currentBlock.Array, currentBlock.Offset, blockReadLength).ConfigureAwait(false);

this.AdvancePosition(ref leftToRead, blockReadLength);
}
Expand Down
7 changes: 7 additions & 0 deletions Lib/Common/Core/NullType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
// </copyright>
//-----------------------------------------------------------------------

using System.Threading.Tasks;

namespace Microsoft.WindowsAzure.Storage.Core
{
/// <summary>
Expand All @@ -27,6 +29,11 @@ public sealed class NullType
/// </summary>
internal static readonly NullType Value = new NullType();

/// <summary>
/// Represents a no-return from a task.
/// </summary>
internal static readonly Task<NullType> ValueTask = Task.FromResult(Value);

/// <summary>
/// Prevents a default instance of the <see cref="NullType"/> class from being created.
/// </summary>
Expand Down
2 changes: 1 addition & 1 deletion Lib/Common/Core/Util/Exceptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ internal async static Task<StorageException> PopulateStorageExceptionFromHttpRes
}
else
{
currentResult.ExtendedErrorInformation = await StorageExtendedErrorInformation.ReadFromStreamAsync(errStream.AsInputStream());
currentResult.ExtendedErrorInformation = await StorageExtendedErrorInformation.ReadFromStreamAsync(errStream.AsInputStream()).ConfigureAwait(false);
}
}
catch (Exception)
Expand Down
6 changes: 3 additions & 3 deletions Lib/Common/StorageExtendedErrorInformation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,9 @@ public static StorageExtendedErrorInformation ReadFromStream(IInputStream inputS
return ReadFromStream(inputStream.AsStreamForRead());
}

public static async Task<StorageExtendedErrorInformation> ReadFromStreamAsync(IInputStream inputStream)
public static Task<StorageExtendedErrorInformation> ReadFromStreamAsync(IInputStream inputStream)
{
return await ReadFromStreamAsync(inputStream.AsStreamForRead());
return ReadFromStreamAsync(inputStream.AsStreamForRead());
}
#endif

Expand Down Expand Up @@ -139,7 +139,7 @@ public static async Task<StorageExtendedErrorInformation> ReadFromStreamAsync(St

using (XmlReader reader = XmlReader.Create(inputStream, settings))
{
await reader.ReadAsync();
await reader.ReadAsync().ConfigureAwait(false);
extendedErrorInfo.ReadXml(reader);
}

Expand Down
8 changes: 4 additions & 4 deletions Lib/WindowsDesktop/Table/DataServices/DataServicesResponseAdapterMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ namespace Microsoft.WindowsAzure.Storage.Table.DataServices
internal class DataServicesResponseAdapterMessage : IODataResponseMessage
{
private IDictionary<string, string> responseHeaders;
private Stream inputStream = null;
private Task<Stream> inputStreamAsCachedTask = null;
private string responseContentType = null;

public DataServicesResponseAdapterMessage(Dictionary<string, string> responseHeaders, Stream inputStream)
Expand All @@ -37,13 +37,13 @@ public DataServicesResponseAdapterMessage(Dictionary<string, string> responseHea
public DataServicesResponseAdapterMessage(IDictionary<string, string> responseHeaders, Stream inputStream, string responseContentType)
{
this.responseHeaders = responseHeaders;
this.inputStream = inputStream;
this.inputStreamAsCachedTask = Task.FromResult(inputStream);
this.responseContentType = responseContentType;
}

public Task<Stream> GetStreamAsync()
{
return Task.Factory.StartNew(() => this.inputStream);
return inputStreamAsCachedTask;
}

public string GetHeader(string headerName)
Expand Down Expand Up @@ -77,7 +77,7 @@ public string GetHeader(string headerName)

public Stream GetStream()
{
return this.inputStream;
return this.inputStreamAsCachedTask.Result; // safe since completed task and avoids additional field for stream
}

public IEnumerable<KeyValuePair<string, string>> Headers
Expand Down
10 changes: 5 additions & 5 deletions Lib/WindowsRuntime/Blob/BlobReadStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public override int Read(byte[] buffer, int offset, int count)
/// <param name="count">The maximum number of bytes to read.</param>
/// <param name="cancellationToken">The token to monitor for cancellation requests.</param>
/// <returns>A task that represents the asynchronous read operation.</returns>
public override async Task<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
public override Task<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
Copy link
Contributor

Choose a reason for hiding this comment

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

is the slight change of behavior here by eliding the async keyword gaining us much perf improvement? other than skipping the internal state machine?

Copy link

Choose a reason for hiding this comment

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

@erezvani1529 there should be no change in behaviour, solely performance gain.
If all the method returns is a task, rather than awaiting for the method and for the operation in the method, only a single await (state machine) is involved.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am referring to the change of behavior with Exceptions and where to expect them, when the method call and await are separated.

Copy link

Choose a reason for hiding this comment

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

Exception will occur where you await the task, if I understood your question correctly. As Task returned by a method is not executed until awaited.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is true if the async keyword exists or the method call and await aren't separated. Stephen Cleary has a great blog-post about this as well, which could be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SeanFeldman #579 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erezvani1529 regarding your comment in #581 (comment)

Eliding the keyword on the hot path means you can save the extra allocations of the statemachine display class. .NET 2.1 will contain improvements that will optimize the statemachine slightly see dotnet/coreclr#14178 and dotnet/coreclr#13105 but still there is allocation overhead.

See for example

BenchmarkDotNet=v0.10.11, OS=Windows 10 Redstone 2 [1703, Creators Update] (10.0.15063.726)
Processor=Intel Core i7-4980HQ CPU 2.80GHz (Haswell), ProcessorCount=8
Frequency=2728068 Hz, Resolution=366.5598 ns, Timer=TSC
  [Host]     : .NET Framework 4.7 (CLR 4.0.30319.42000), 64bit RyuJIT-v4.7.2115.0
  DefaultJob : .NET Framework 4.7 (CLR 4.0.30319.42000), 64bit RyuJIT-v4.7.2115.0
Method Mean Error StdDev StdErr Min Q1 Median Q3 Max Op/s Scaled Allocated
WithStatemachine 15.61 ms 0.0166 ms 0.0155 ms 0.0040 ms 15.58 ms 15.60 ms 15.61 ms 15.62 ms 15.63 ms 64.06 1.00 640 B
WithoutStatemachine 15.61 ms 0.0128 ms 0.0120 ms 0.0031 ms 15.59 ms 15.60 ms 15.61 ms 15.62 ms 15.63 ms 64.06 1.00 384 B 
BenchmarkDotNet=v0.10.11, OS=Windows 10 Redstone 2 [1703, Creators Update] (10.0.15063.726)
Processor=Intel Core i7-4980HQ CPU 2.80GHz (Haswell), ProcessorCount=8
Frequency=2728068 Hz, Resolution=366.5598 ns, Timer=TSC
  [Host]     : .NET Framework 4.7 (CLR 4.0.30319.42000), 64bit RyuJIT-v4.7.2115.0
  Job-YTNNJT : .NET Core 2.0.3 (Framework 4.6.25815.02), 64bit RyuJIT

Toolchain=.NET Core 2.0
Method Mean Error StdDev StdErr Min Q1 Median Q3 Max Op/s Scaled Allocated
WithStatemachine 15.61 ms 0.0155 ms 0.0138 ms 0.0037 ms 15.58 ms 15.60 ms 15.60 ms 15.61 ms 15.63 ms 64.08 1.00 528 B
WithoutStatemachine 15.60 ms 0.0186 ms 0.0174 ms 0.0045 ms 15.57 ms 15.59 ms 15.60 ms 15.62 ms 15.63 ms 64.08 1.00 312 B

As you can imagine the allocations here would add up pretty quickly.

It would be possible to unbundle those things from the PR. but I do think it is worth keeping.

{
CommonUtility.AssertNotNull("buffer", buffer);
CommonUtility.AssertInBounds("offset", offset, 0, buffer.Length);
Expand All @@ -108,16 +108,16 @@ public override async Task<int> ReadAsync(byte[] buffer, int offset, int count,

if ((this.currentOffset == this.Length) || (count == 0))
{
return 0;
return Task.FromResult(0);
}

int readCount = this.ConsumeBuffer(buffer, offset, count);
if (readCount > 0)
{
return readCount;
return Task.FromResult(readCount);
}

return await this.DispatchReadAsync(buffer, offset, count);
return this.DispatchReadAsync(buffer, offset, count);
}

/// <summary>
Expand All @@ -140,7 +140,7 @@ await this.blob.DownloadRangeToStreamAsync(
this.GetReadSize(),
this.accessCondition,
this.options,
this.operationContext);
this.operationContext).ConfigureAwait(false);

this.internalBuffer.Seek(0, SeekOrigin.Begin);
return this.ConsumeBuffer(buffer, offset, count);
Expand Down
22 changes: 11 additions & 11 deletions Lib/WindowsRuntime/Blob/BlobWriteStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ public override async Task WriteAsync(byte[] buffer, int offset, int count, Canc

if (bytesToWrite == maxBytesToWrite)
{
await this.DispatchWriteAsync();
await this.DispatchWriteAsync().ConfigureAwait(false);
}
}
}
Expand Down Expand Up @@ -188,7 +188,7 @@ public override async Task FlushAsync(CancellationToken cancellationToken)
throw new InvalidOperationException(SR.BlobStreamAlreadyCommitted);
}

await this.DispatchWriteAsync();
await this.DispatchWriteAsync().ConfigureAwait(false);
await Task.Run(() => this.noPendingWritesEvent.Wait(), cancellationToken);

if (this.lastException != null)
Expand Down Expand Up @@ -225,7 +225,7 @@ protected override void Dispose(bool disposing)
/// <returns>A task that represents the asynchronous commit operation.</returns>
public override async Task CommitAsync()
{
await this.FlushAsync();
await this.FlushAsync().ConfigureAwait(false);
this.committed = true;

try
Expand All @@ -237,14 +237,14 @@ public override async Task CommitAsync()
this.blockBlob.Properties.ContentMD5 = this.blobMD5.ComputeHash();
}

await this.blockBlob.PutBlockListAsync(this.blockList, this.accessCondition, this.options, this.operationContext);
await this.blockBlob.PutBlockListAsync(this.blockList, this.accessCondition, this.options, this.operationContext).ConfigureAwait(false);
}
else
{
if (this.blobMD5 != null)
{
this.Blob.Properties.ContentMD5 = this.blobMD5.ComputeHash();
await this.Blob.SetPropertiesAsync(this.accessCondition, this.options, this.operationContext);
await this.Blob.SetPropertiesAsync(this.accessCondition, this.options, this.operationContext).ConfigureAwait(false);
}
}
}
Expand Down Expand Up @@ -282,7 +282,7 @@ private async Task DispatchWriteAsync()
{
string blockId = this.GetCurrentBlockId();
this.blockList.Add(blockId);
await this.WriteBlockAsync(bufferToUpload, blockId, bufferMD5);
await this.WriteBlockAsync(bufferToUpload, blockId, bufferMD5).ConfigureAwait(false);
}
else if (this.pageBlob != null)
{
Expand All @@ -294,7 +294,7 @@ private async Task DispatchWriteAsync()

long offset = this.currentBlobOffset;
this.currentBlobOffset += bufferToUpload.Length;
await this.WritePagesAsync(bufferToUpload, offset, bufferMD5);
await this.WritePagesAsync(bufferToUpload, offset, bufferMD5).ConfigureAwait(false);
}
else
{
Expand All @@ -310,7 +310,7 @@ private async Task DispatchWriteAsync()
throw this.lastException;
}

await this.WriteAppendBlockAsync(bufferToUpload, offset, bufferMD5);
await this.WriteAppendBlockAsync(bufferToUpload, offset, bufferMD5).ConfigureAwait(false);
}
}

Expand All @@ -324,7 +324,7 @@ private async Task DispatchWriteAsync()
private async Task WriteBlockAsync(Stream blockData, string blockId, string blockMD5)
{
this.noPendingWritesEvent.Increment();
await this.parallelOperationSemaphore.WaitAsync();
await this.parallelOperationSemaphore.WaitAsync().ConfigureAwait(false);
Task putBlockTask = this.blockBlob.PutBlockAsync(blockId, blockData, blockMD5, this.accessCondition, this.options, this.operationContext).ContinueWith(task =>
{
if (task.Exception != null)
Expand All @@ -347,7 +347,7 @@ private async Task WriteBlockAsync(Stream blockData, string blockId, string bloc
private async Task WritePagesAsync(Stream pageData, long offset, string contentMD5)
{
this.noPendingWritesEvent.Increment();
await this.parallelOperationSemaphore.WaitAsync();
await this.parallelOperationSemaphore.WaitAsync().ConfigureAwait(false);
Task writePagesTask = this.pageBlob.WritePagesAsync(pageData, offset, contentMD5, this.accessCondition, this.options, this.operationContext).ContinueWith(task =>
{
if (task.Exception != null)
Expand All @@ -372,7 +372,7 @@ private async Task WritePagesAsync(Stream pageData, long offset, string contentM
private async Task WriteAppendBlockAsync(Stream blockData, long offset, string blockMD5)
{
this.noPendingWritesEvent.Increment();
await this.parallelOperationSemaphore.WaitAsync();
await this.parallelOperationSemaphore.WaitAsync().ConfigureAwait(false);

this.accessCondition.IfAppendPositionEqual = offset;

Expand Down
Loading