Skip to content

Conversation

davidwrighton
Copy link
Member

@davidwrighton davidwrighton commented May 27, 2025

Add paths for reading the IL-Native map which doesn't create a DebuggerFunctionInfo or DebuggerJitInfo
Use it in the stackwalker code for EH, and for the event tracing MethodToILMap

This process is somewhat slower than the path through the diagnostic infrastructure, but it has no expensive locks required. To mitigate the performance loss, the format of the compressed bounds information was changed to a an encoding where all entries in the table are fixed size, but the actual bit widths of the various fields are optimally small. Overall this results in a size savings of about 50KB on System.Private.CoreLib.dll, as well as capturing back all of the performance costs of this change for relatively small functions.

In combination with the NativeToIL map cache added a few days ago, this allows us to eliminate locks from the native offset to il offset path, which should substantially reduce the cost when heavily multithreaded applications are experiencing significant error rates. (And the cache added mitigates the remaining performance loss from using the compressed data instead of the uncompressed form.)

In addition, by changing the event trace path to use the same approach, we will remove the DebuggerJitInfo generation path from most customers non-debugging scenarios. (It still remains in the ICorProfiler:GetILToNativeMapping apis, but usage of that api is relatively rare).

There is validation which utilizes the debugger apis to validate that the behavior hasn't changed at all.

- This removes it from normal path
- This also adds an assert that it is correct

Logically, this code should probably replace the current implementation of GetILOffsetFromNative but this first pass is just to get the code working

This is untested, but it does compile.
@Copilot Copilot AI review requested due to automatic review settings May 27, 2025 18:12
@davidwrighton davidwrighton marked this pull request as draft May 27, 2025 18:12
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

… chunks and doing very cheap operations per nibble read

- Add a fixed size cache around the native to IL mapping operations in the stacktrace logic
  - Unfortunately, simply reading the nibble data is pretty slow, so this is needed to avoid regressing performance
  - With this change the performance of newing up a StackTrace object on a multithreaded environment is primarily driven by GC allocation rate, and not significantly by any other locks.
This both generates less data (Experiments show about a 70KB reduction in size of System.Private.CoreLib.dll R2R image)
And the reader as implemented here, is quite a bit faster

When combined with the cache constructed in a different part of this PR, this results in the cost of IL offset determination for a given IP to have negligible cost.
- Add handling for event tracing uses of IL to Native mapping logic to avoid using the diagnostic infrastructure there
- Add validation logic for the event tracing path the ValidateILOffsets
- Limit the NativeOffset to IL Offset validation logic to do O(N) work instead of O(N*N) work in worst case.
…ng a template instead of 2 seperate implementations
@davidwrighton davidwrighton changed the title [DRAFT] Use il offset walker instead of debugger data to gather the IL offset during stack trace generation Walk compressed IL -> Native map for EventTrace and stacktrace symbolication scenarios Jul 8, 2025
@davidwrighton davidwrighton marked this pull request as ready for review July 8, 2025 05:47
@davidwrighton davidwrighton requested a review from noahfalk July 8, 2025 05:51
Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

Thanks David! This looks very nice overall. A few comments inline. I think there may be some unintended changed behavior in GetILToNativeMapping() that would cause trouble for profilers.

@noahfalk
Copy link
Member

noahfalk commented Jul 9, 2025

fyi @dotnet/dotnet-diag

Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

👍

@davidwrighton davidwrighton enabled auto-merge (squash) July 10, 2025 04:15
@davidwrighton davidwrighton merged commit f3548ef into dotnet:main Jul 10, 2025
95 of 97 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants