-
Notifications
You must be signed in to change notification settings - Fork 690
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid the blocking wait operation in RestoreOperationLogger #1588
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,19 +2,20 @@ | |
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using System.Collections.Concurrent; | ||
using System.ComponentModel.Composition; | ||
using System.Globalization; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Microsoft; | ||
using Microsoft.VisualStudio.Shell; | ||
using Microsoft.VisualStudio.Shell.Interop; | ||
using Microsoft.VisualStudio.Threading; | ||
using NuGet.Common; | ||
using NuGet.PackageManagement.VisualStudio; | ||
using NuGet.VisualStudio; | ||
using Task = System.Threading.Tasks.Task; | ||
using MSBuildVerbosityLevel = NuGet.SolutionRestoreManager.VerbosityLevel; | ||
using Microsoft; | ||
using Task = System.Threading.Tasks.Task; | ||
|
||
namespace NuGet.SolutionRestoreManager | ||
{ | ||
|
@@ -28,6 +29,9 @@ internal sealed class RestoreOperationLogger : LoggerBase, ILogger, IDisposable | |
private readonly IServiceProvider _serviceProvider; | ||
private readonly IOutputConsoleProvider _outputConsoleProvider; | ||
|
||
// Queue of (bool reportProgress, bool showAsOutputMessage, ILogMessage logMessage) | ||
private readonly ConcurrentQueue<Tuple<bool, bool, ILogMessage>> _loggedMessages = new ConcurrentQueue<Tuple<bool, bool, ILogMessage>>(); | ||
|
||
private Lazy<ErrorListTableDataSource> _errorListDataSource; | ||
private RestoreOperationSource _operationSource; | ||
private JoinableTaskFactory _taskFactory; | ||
|
@@ -130,7 +134,7 @@ public override void LogInformationSummary(string data) | |
/// <summary> | ||
/// Same as LogAsync but uses Do instead of DoAsync. | ||
/// </summary> | ||
public override void Log(ILogMessage logMessage) | ||
public sealed override void Log(ILogMessage logMessage) | ||
{ | ||
HandleErrorsAndWarnings(logMessage); | ||
|
||
|
@@ -145,10 +149,20 @@ public override void Log(ILogMessage logMessage) | |
// Avoid moving to the UI thread unless there is work to do | ||
if (reportProgress || showAsOutputMessage) | ||
{ | ||
// Make sure the message is queued in order of calls to LogAsync, but don't wait for the UI thread | ||
// to actually show it. | ||
_loggedMessages.Enqueue(Tuple.Create(reportProgress, showAsOutputMessage, logMessage)); | ||
|
||
// Run on the UI thread | ||
Do((_, progress) => | ||
var ignored = DoAsync((_, progress) => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ➡️ This is a candidate feature for a future version of C# (dotnet/csharplang#111), but currently isn't available. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is for the return value. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is already a parameter named |
||
{ | ||
LogToVS(reportProgress, showAsOutputMessage, logMessage, progress); | ||
// This might be a different message than the one enqueued above, but overall the printing order | ||
// will match the order of calls to LogAsync. | ||
Tuple<bool, bool, ILogMessage> message; | ||
if (_loggedMessages.TryDequeue(out message)) | ||
{ | ||
LogToVS(reportProgress: message.Item1, showAsOutputMessage: message.Item2, logMessage: message.Item3, progress: progress); | ||
} | ||
}); | ||
} | ||
} | ||
|
@@ -159,29 +173,9 @@ public override void Log(ILogMessage logMessage) | |
/// if the message needs to be logged before moving to the | ||
/// UI thread. | ||
/// </summary> | ||
public override Task LogAsync(ILogMessage logMessage) | ||
public sealed override Task LogAsync(ILogMessage logMessage) | ||
{ | ||
HandleErrorsAndWarnings(logMessage); | ||
|
||
if (DisplayMessage(logMessage.Level)) | ||
{ | ||
var verbosityLevel = GetMSBuildLevel(logMessage.Level); | ||
var reportProgress = ShouldReportProgress(logMessage); | ||
|
||
// Write to the output window if the verbosity level is high enough. | ||
var showAsOutputMessage = ShouldShowMessageAsOutput(verbosityLevel); | ||
|
||
// Avoid moving to the UI thread unless there is work to do | ||
if (reportProgress || showAsOutputMessage) | ||
{ | ||
// Run on the UI thread | ||
return DoAsync((_, progress) => | ||
{ | ||
LogToVS(reportProgress, showAsOutputMessage, logMessage, progress); | ||
}); | ||
} | ||
} | ||
|
||
Log(logMessage); | ||
return Task.FromResult(0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid allocating the task every time by returning a cached one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
➡️ Based on this, I decided to treat this as a code cleanup issue which can wait for a future commit dedicated to this effort. |
||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the tuple syntax?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tuple syntax would be very much preferable, but I wasn't sure if this project was using it yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
➡️ Currently
System.ValueTuple
is not available for use in this project.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create a private struct with the two fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benaadams I predict that in the near future someone will finally add
System.ValueTuple
to this project and go through the code searching forTuple.Create
. The currently implementation works well with such a transition. It's also part of the reason why I created the field's comment in the form I did.