Update ILToNativeMap event to include IL version#71479
Conversation
jakobbotsch
left a comment
There was a problem hiding this comment.
I don't think this will be enough for the problem they are trying to solve in #70187 for the reasons I've mentioned in #70187 (comment).
|
@jakobbotsch, yes you are correct. I opened this prematurely, in a bit I will add the native code version too |
Sounds good, though I think it would be better to expose the native code version in the ReJITID field, similar to the |
|
The approach I went with was to use the native code version as the ReJITID in the event to match the other uses, then I also created a V1 event and added the IL version too in case anyone needs it in the future |
| if ((dwEventOptions & ETW::EnumerationLog::EnumerationStructs::JitMethodILToNativeMap) != 0) | ||
| { | ||
| FireEtwMethodILToNativeMap( | ||
| FireEtwMethodILToNativeMap_V1( |
There was a problem hiding this comment.
Is this considered a breaking change? Considering that the payload changes the meaning of ReJITID for this event?
There was a problem hiding this comment.
Yes it is technically a breaking change, I think it is worth the slight risk though. The native code version is much more interesting when looking at IL to native maps, and it's hard to imagine anyone actually takes a dependency on knowing the IL version. The only way you'll see a new IL version is profiler rejit.
There was a problem hiding this comment.
I am happy with that, just probably good do document in breaking changes release notes for the release and call it good.
|
This new version of the code also works properly for VS and PerfView goto source I trust? |
I did some in depth testing with an app where it did a long running operation three ways:
Perfview handles it fine for all, VS handles it fine for tiering. For the rejit case, VS will show two different method entries with the same name for samples: That doesn't bother me at all given the relative rarity of rejit. |
Fixes #70187
Tested that go to source continues to work with Perfview and VS CPU analysis still works.