[AOT] Refactor Logger function to improve performance and mark managedCommon as AOT compatible #36327
Conversation
moooyo
added
the
Needs-Review
|
When we publish application with AOT enable. Some reflection usage will be forbidden. So, we need to make this change. |
crutkas
added
In for .88
Don't merge - Hold for release
| } | ||
|
|
||
| private static MethodBase GetCallerMethod(StackTrace stackTrace) | ||
| private static string GetCallerInfo(string memberName, string sourceFilePath, int sourceLineNumber) |
There was a problem hiding this comment.
For sourceLineNumber.
Given originally they don't have, we need to ask for PMs, that any parser of the log it will break this.
If Yes, we should still keep the original "TypeName::MethodName" format first.
Last but not least, do we know how to view PowerToys log? If we can parrse the log and show a summary of before after, it will give confidence of this changes.
| { | ||
| Log(string.Empty, TraceFlag); | ||
| Log(string.Empty, TraceFlag, memberName, sourceFilePath, sourceLineNumber); | ||
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] |
There was a problem hiding this comment.
May be we don't need this one anymore.
| @@ -53,18 +52,37 @@ public static void InitializeLogger(string applicationLogPath, bool isLocalLow = | |||
| Trace.AutoFlush = true; | |||
| } | |||
|
|
|||
| public static string GetVersion() | |||
There was a problem hiding this comment.
Can we just ask the caller to pass in its own class type?
From Github copilot, the following code should be AOT compatible:
var versionInfo = FileVersionInfo.GetVersionInfo(typeof(Program).Assembly.Location);
Console.WriteLine($"Version: {versionInfo.FileVersion}
jaimecbernardo
removed
the
Don't merge - Hold for release
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
moooyo
changed the title
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Summary of the Pull Request
To drop all reflection usage, we can get about 18% performance improvement by this PR change. Here's a simple test which call Logger.LogError 100k times. (Left: before this change, Right with this change)
By this change, we can remove the wired useage of reflection which try to get some infromation from stack frame. It's hard to understand and maintain.
About the log format. I've compared the advancedPaste module's log. Most of them are equal, but some of them are not the same.
Such as:
The original code will retrieve the caller info from the stack frame. The caller name will be so strange in some case. After this change merged, we will get a more clear information.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed