-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Defer resolution of StackTrace debug info until use #115019
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
Changes from 9 commits
38fb5cc
cd7ee17
f23f1fe
62a11ef
110eef1
47cd097
da5fbca
9185c18
90a75dc
28dbd25
4c0a1de
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 |
|---|---|---|
|
|
@@ -94,8 +94,40 @@ public StackFrameHelper() | |
| internal void InitializeSourceInfo(bool fNeedFileInfo, Exception? exception) | ||
| { | ||
| StackTrace.GetStackFramesInternal(this, fNeedFileInfo, exception); | ||
| } | ||
|
|
||
| public MethodBase? GetMethodBase(int i) | ||
| { | ||
| // There may be a better way to do this. | ||
| // we got RuntimeMethodHandles here and we need to go to MethodBase | ||
| // but we don't know whether the reflection info has been initialized | ||
| // or not. So we call GetMethods and GetConstructors on the type | ||
| // and then we fetch the proper MethodBase!! | ||
| IntPtr mh = rgMethodHandle![i]; | ||
|
|
||
| if (mh == IntPtr.Zero) | ||
| return null; | ||
|
|
||
| IRuntimeMethodInfo? mhReal = RuntimeMethodHandle.GetTypicalMethodDefinition(new RuntimeMethodInfoStub(new RuntimeMethodHandleInternal(mh), this)); | ||
|
|
||
| return RuntimeType.GetMethodBase(mhReal); | ||
| } | ||
|
|
||
| private void InitializeFrameSourceInfo(int index) | ||
| { | ||
| // rgiMethodToken is null if file info wasn't requested when collecting the stack trace. | ||
| if (rgiMethodToken == null) | ||
| return; | ||
|
|
||
| if (!fNeedFileInfo) | ||
| // We use rgiMethodToken[i] to indicate whether we've initialized the info for this frame. | ||
| // If the native code set it to zero, then information is already initialized from the native | ||
| // symbol reader. If the native code set it to non-zero, then we're supposed to use the managed | ||
| // symbol reader to try to resolve debug information. | ||
| // | ||
| // This is the only purpose the field has, however, so we can assign it to zero AFTER resolving | ||
| // symbol information to indicate that it's already been resolved without causing any issues | ||
| // elsewhere. | ||
| if (rgiMethodToken[index] == 0) | ||
| return; | ||
|
|
||
| // Check if this function is being reentered because of an exception in the code below | ||
|
|
@@ -112,17 +144,12 @@ internal void InitializeSourceInfo(bool fNeedFileInfo, Exception? exception) | |
| Interlocked.CompareExchange(ref s_stackTraceSymbolsCache, CreateStackTraceSymbols(), null); | ||
| } | ||
|
|
||
| for (int index = 0; index < iFrameCount; index++) | ||
| { | ||
| // If there was some reason not to try get the symbols from the portable PDB reader like the module was | ||
| // ENC or the source/line info was already retrieved, the method token is 0. | ||
| if (rgiMethodToken![index] != 0) | ||
| { | ||
| GetSourceLineInfo(s_stackTraceSymbolsCache!, rgAssembly![index], rgAssemblyPath![index]!, rgLoadedPeAddress![index], rgiLoadedPeSize![index], rgiIsFileLayout![index], | ||
| rgInMemoryPdbAddress![index], rgiInMemoryPdbSize![index], rgiMethodToken![index], | ||
| rgiILOffset![index], out rgFilename![index], out rgiLineNumber![index], out rgiColumnNumber![index]); | ||
| } | ||
| } | ||
| GetSourceLineInfo(s_stackTraceSymbolsCache!, rgAssembly![index], rgAssemblyPath![index]!, rgLoadedPeAddress![index], rgiLoadedPeSize![index], rgiIsFileLayout![index], | ||
| rgInMemoryPdbAddress![index], rgiInMemoryPdbSize![index], rgiMethodToken![index], | ||
| rgiILOffset![index], out rgFilename![index], out rgiLineNumber![index], out rgiColumnNumber![index]); | ||
|
|
||
| // Make sure we mark down that debug information for this frame was resolved | ||
| rgiMethodToken[index] = 0; | ||
|
Member
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. Does this need to be thread safe? Other parts of the change use Interlocked.CompareExchange that suggests this is expected to be thread safe.
Contributor
Author
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. Most likely. I think it should be sufficient to outline the
Member
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. Yes, something like this should work
Contributor
Author
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. Should be done now |
||
| } | ||
| catch | ||
| { | ||
|
|
@@ -133,28 +160,37 @@ internal void InitializeSourceInfo(bool fNeedFileInfo, Exception? exception) | |
| } | ||
| } | ||
|
|
||
| public MethodBase? GetMethodBase(int i) | ||
| { | ||
| // There may be a better way to do this. | ||
| // we got RuntimeMethodHandles here and we need to go to MethodBase | ||
| // but we don't know whether the reflection info has been initialized | ||
| // or not. So we call GetMethods and GetConstructors on the type | ||
| // and then we fetch the proper MethodBase!! | ||
| IntPtr mh = rgMethodHandle![i]; | ||
|
|
||
| if (mh == IntPtr.Zero) | ||
| return null; | ||
|
|
||
| IRuntimeMethodInfo? mhReal = RuntimeMethodHandle.GetTypicalMethodDefinition(new RuntimeMethodInfoStub(new RuntimeMethodHandleInternal(mh), this)); | ||
|
|
||
| return RuntimeType.GetMethodBase(mhReal); | ||
| } | ||
|
|
||
| public int GetOffset(int i) { return rgiOffset![i]; } | ||
| public int GetILOffset(int i) { return rgiILOffset![i]; } | ||
| public string? GetFilename(int i) { return rgFilename?[i]; } | ||
| public int GetLineNumber(int i) { return rgiLineNumber == null ? 0 : rgiLineNumber[i]; } | ||
| public int GetColumnNumber(int i) { return rgiColumnNumber == null ? 0 : rgiColumnNumber[i]; } | ||
| public string? GetFilename(int i) | ||
| { | ||
| InitializeFrameSourceInfo(i); | ||
| return rgFilename?[i]; | ||
| } | ||
| public int GetLineNumber(int i) | ||
| { | ||
| if (rgiLineNumber == null) | ||
| { | ||
| return 0; | ||
| } | ||
| else | ||
| { | ||
| InitializeFrameSourceInfo(i); | ||
| return rgiLineNumber[i]; | ||
| } | ||
| } | ||
| public int GetColumnNumber(int i) | ||
| { | ||
| if (rgiColumnNumber == null) | ||
| { | ||
| return 0; | ||
| } | ||
| else | ||
| { | ||
| InitializeFrameSourceInfo(i); | ||
| return rgiColumnNumber[i]; | ||
| } | ||
| } | ||
|
|
||
| public bool IsLastFrameFromForeignExceptionStackTrace(int i) | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -95,5 +95,10 @@ internal void ToString(TraceFormat traceFormat, StringBuilder builder) | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| builder.AppendLine(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #endif | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private StackFrame[] GetFramesCore() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return _stackFrames ?? []; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
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. Should NativeAOT get the same optimization? We do not want NativeAOT performance to be lagging behind.
Contributor
Author
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. I'm not sure if debug info resolution is as significant on NAOT; I haven't benched it. My driving scenario is JIT-only for entirely separate reasons anyway. It shouldn't be too hard to do there, though I'd want to profile it first.
Member
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. For consistency I think doing the same defered StackFrame creation would be nice. I suspect the optimization won't make a substantial difference unless it is run together with a DeveloperExperience implementation that does more appreciable work: runtime/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Diagnostics/StackFrame.NativeAot.cs Lines 84 to 118 in d34eb61
Member
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.
I would be good to see some numbers.
DeveloperExperience is on by default in NativeAOT.
Member
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. #122227 is adding support for reading line information on the native AOT side so this optimization becomes relevant there too. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
It might be nice to also defer Windows PDB data resolution, but that requires some nontrivial changes in the runtime.
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.
Agreed but totally content if it is out of scope for this PR.