-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[NativeAOT] A few cleanups in stack walking on Windows #86917
Conversation
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsA subset of #86690 As discussed in: #86690 (comment)
|
@@ -10,17 +10,17 @@ namespace System | |||
[StructLayout(LayoutKind.Sequential)] | |||
public struct RuntimeTypeHandle | |||
{ | |||
private EETypePtr _pEEType; | |||
private IntPtr _pEEType; |
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.
What motivated this change? We have another copy at src\coreclr\nativeaot\Runtime.Base\src\System\RuntimeTypeHandle.cs
that can be updated as well.
Also, while you are on it - could you please delete this comment
runtime/src/coreclr/nativeaot/System.Private.CoreLib/src/System/RuntimeTypeHandle.cs
Lines 19 to 23 in 4860b6c
// | |
// Caution: There can be and are multiple MethodTable for the "same" type (e.g. int[]). That means | |
// you can't use the raw IntPtr value for comparisons. | |
// | |
private IntPtr _value;
to the top in that file?
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.
What motivated this change?
I was looking at what stack traces do allocations and at some point I had doubts that stacks are correct. So I tried to look at optimized=false
traces. It was mostly the same, in terms of allocating paths, except a few cases of boxing, that are otherwise optimized.
What surprised me is the wrapping/unwrapping of EETypePtr in the RuntimeTypeHandle - it was near the top. It looks like a common operation, so I "fixed" it to not clutter the profile.
This change is not required, since JIT can optimize this away, but I figured it is better if I keep "fixed" variant.
@@ -10,17 +10,17 @@ namespace System | |||
[StructLayout(LayoutKind.Sequential)] | |||
public struct RuntimeTypeHandle | |||
{ | |||
private EETypePtr _pEEType; | |||
private IntPtr _pEEType; |
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.
Also, it may be nice to rename _pEEType
to _value
so that it is in sync with the full implementation.
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.
Thanks!
Thanks! |
A subset of #86690
As discussed in: #86690 (comment)