Conversation
taooceros
left a comment
taooceros
left a comment
There was a problem hiding this comment.
A few comment about the changes.
Hope we won't experience serious merge conflict in the future XD
| public struct ResultsForUpdate | ||
| { | ||
| public List<Result> Results { get; } | ||
| public IReadOnlyList<Result> Results { get; } |
There was a problem hiding this comment.
It has been changed because I add an emptyResult List singleton for null result
| catch (OperationCanceledException) | ||
| { | ||
| // return empty result when cancelled | ||
| return results; | ||
| } |
There was a problem hiding this comment.
Previously OperationCanceledException is thrown as normal exception, causing the slowness of logging exception.
| static ProgramLogger() | ||
| { | ||
| var path = Path.Combine(DataLocation.DataDirectory(), DirectoryName, Constant.Version); | ||
| if (!Directory.Exists(path)) | ||
| { | ||
| Directory.CreateDirectory(path); | ||
| } | ||
|
|
||
| var configuration = new LoggingConfiguration(); | ||
| var target = new FileTarget(); | ||
| configuration.AddTarget("file", target); | ||
| target.FileName = path.Replace(@"\", "/") + "/${shortdate}.txt"; | ||
| #if DEBUG | ||
| var rule = new LoggingRule("*", LogLevel.Debug, target); | ||
| #else | ||
| var rule = new LoggingRule("*", LogLevel.Error, target); | ||
| #endif | ||
| configuration.LoggingRules.Add(rule); | ||
| LogManager.Configuration = configuration; | ||
| } | ||
|
|
There was a problem hiding this comment.
It is removed because it will override the config setting in Log.cs (which is the same in before so no effect is shown).
| Debug.WriteLine($"ERROR{classname}|{callingMethodName}|{loadingProgramPath}|{interpretationMessage}"); | ||
|
|
There was a problem hiding this comment.
It is removed because the use of NLog internal Debug Target.
| var debugTarget = new OutputDebugStringTarget | ||
| { | ||
| Layout = layout | ||
| }; |
There was a problem hiding this comment.
The target to output information to debug console (not the console target for command line app).
| if (currentCancellationToken.IsCancellationRequested) | ||
| return; |
|
What's tested so far with this Have you taken it for a spin to ensure there is no degradation or slowness |
jjw24
added
the
review in progress
Theoretically, there will only be performance boost due to the use of Async Wrapper in Debug mode (debug mode has quite significant parallel logging). The remove of Synchronize Implementation should make logging Exception from multiple thread much faster than before. |
|
|
||
| token.ThrowIfCancellationRequested(); | ||
| if (results == null) | ||
| return results; |
There was a problem hiding this comment.
just curious why return null when results is already null? does this actually change anything?
jjw24
added
enhancement
2 participants
Part of the commit in #470