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

Tiered jitting: Implement additional profiler APIs #12610

Closed
noahfalk opened this Issue Jul 4, 2017 · 10 comments

Comments

Projects
None yet
6 participants
@noahfalk
Member

noahfalk commented Jul 4, 2017

Tiered jitting breaks a previous invariant that there would only be one jitted code body per <FunctionID, ReJitID, generic instantiation> tuple. In order to address this we need to add some new APIs that profilers can use to get information about these new method bodies. The breaking change doc added in #12193 has a bit more background. The likely APIs (design not final) would be:

//Given functionId + rejitId, enumerate the native code start address of all jitted versions of this code that currently exist
ICorProfilerInfo9::GetNativeCodeStartAddresses(FunctionID functionID, ReJITID reJitId, ULONG32 cCodeStartAddresses, ULONG32 *pcCodeStartAddresses, UINT_PTR codeStartAddresses[]);

//Given the native code start address, return the native->IL mapping information for this jitted version of the code
ICorProfilerInfo9::GetILToNativeMapping3(UINT_PTR pNativeCodeStartAddress, ULONG32 cMap, ULONG32 *pcMap, COR_DEBUG_IL_TO_NATIVE_MAP map[]);

//Given the native code start address, return the the blocks of virtual memory that store this code (method code is not necessarily stored in a single contiguous memory region)
ICorProfilerInfo9::GetCodeInfo4(UINT_PTR pNativeCodeStartAddress, ULONG32 cCodeInfos, ULONG32* pcCodeInfos, COR_PRF_CODE_INFO codeInfos[]);

There may also be a tiered compilation opt-out mechanism added to the profiler API, but any work for that is tracked under #12609.

@noahfalk

This comment has been minimized.

Show comment
Hide comment
@noahfalk

noahfalk Jul 4, 2017

Member

@mjsabby @SergeyKanzhelev @discostu105 - Figured you guys might have particular interest in these. Let me know if you think the APIs I suggested above sound reasonable.

Also please point out if you think there are any gaps that need addressing. For example, do we need a new API that maps code pointer in method -> code start address? As-is if you wanted to do that transformation you would need to do a multi-step process like this:
a) GetFunctionFromIP2 to get FunctionID+rejitID
b) GetCodeNativeCodeStartAddresses to map (FunctionID+rejitID) -> list of start addresses
c) Iterate GetCodeInfo4 to convert list of start addresses -> Dictionary<list of code regions, start address>
d) Lookup code pointer to determine which region it was in, then map that back to the start address with the dictionary you made.

Member

noahfalk commented Jul 4, 2017

@mjsabby @SergeyKanzhelev @discostu105 - Figured you guys might have particular interest in these. Let me know if you think the APIs I suggested above sound reasonable.

Also please point out if you think there are any gaps that need addressing. For example, do we need a new API that maps code pointer in method -> code start address? As-is if you wanted to do that transformation you would need to do a multi-step process like this:
a) GetFunctionFromIP2 to get FunctionID+rejitID
b) GetCodeNativeCodeStartAddresses to map (FunctionID+rejitID) -> list of start addresses
c) Iterate GetCodeInfo4 to convert list of start addresses -> Dictionary<list of code regions, start address>
d) Lookup code pointer to determine which region it was in, then map that back to the start address with the dictionary you made.

@SergeyKanzhelev

This comment has been minimized.

Show comment
Hide comment
@SergeyKanzhelev

SergeyKanzhelev Jul 5, 2017

CC: @jacdavis
Are those methods used for anything else? Sorry for ignorance, I haven't looked into code, but why not simply make this work?

SergeyKanzhelev commented Jul 5, 2017

CC: @jacdavis
Are those methods used for anything else? Sorry for ignorance, I haven't looked into code, but why not simply make this work?

@noahfalk

This comment has been minimized.

Show comment
Hide comment
@noahfalk

noahfalk Jul 5, 2017

Member

ICorProfilerFunctionControl::SetILInstrumentedCodeMap will work, but it is orthogonal to the APIs above. The APIs above would be used in scenarios where you previously used GetILToNativeMapping1/2 and GetCodeCodeInfo1/2/3. For example in the past to map a native offset to an IL offset, may have called:

  1. GetFunctionFromIP to learn the FunctionID
  2. GetCodeInfo to determine the segments of memory this function uses for code, so that you could compute a native offset within the method
  3. GetILToNativeMapping to compute the IL offset for that native offset.

That flow breaks down as soon as you can have more than one jitted method body for the same FunctionID (and same rejitid). You need a stronger identifier to indicate which jitted method body you are trying to refer to, because each jitted method body may have a different native->IL map and definately will have different memory bounds.

Member

noahfalk commented Jul 5, 2017

ICorProfilerFunctionControl::SetILInstrumentedCodeMap will work, but it is orthogonal to the APIs above. The APIs above would be used in scenarios where you previously used GetILToNativeMapping1/2 and GetCodeCodeInfo1/2/3. For example in the past to map a native offset to an IL offset, may have called:

  1. GetFunctionFromIP to learn the FunctionID
  2. GetCodeInfo to determine the segments of memory this function uses for code, so that you could compute a native offset within the method
  3. GetILToNativeMapping to compute the IL offset for that native offset.

That flow breaks down as soon as you can have more than one jitted method body for the same FunctionID (and same rejitid). You need a stronger identifier to indicate which jitted method body you are trying to refer to, because each jitted method body may have a different native->IL map and definately will have different memory bounds.

@SergeyKanzhelev

This comment has been minimized.

Show comment
Hide comment
@SergeyKanzhelev

SergeyKanzhelev Jul 5, 2017

Just to confirm - when you call to ICorProfilerFunctionControl::SetILInstrumentedCodeMap it set's the mapping between new IL to old IL. And new IL is mapped to the native code. So customer's original instructions and source code will be mapped to the Native code by proxy.

Is it fair to say you only need proposed API when you change method body drastically? By drastically I mean not making an additive code modification like inserting prefix, postfix or something in the middle. You need new API when you inserting new code that should be debuggable and need it's own mapping to Native Code.

SergeyKanzhelev commented Jul 5, 2017

Just to confirm - when you call to ICorProfilerFunctionControl::SetILInstrumentedCodeMap it set's the mapping between new IL to old IL. And new IL is mapped to the native code. So customer's original instructions and source code will be mapped to the Native code by proxy.

Is it fair to say you only need proposed API when you change method body drastically? By drastically I mean not making an additive code modification like inserting prefix, postfix or something in the middle. You need new API when you inserting new code that should be debuggable and need it's own mapping to Native Code.

@noahfalk

This comment has been minimized.

Show comment
Hide comment
@noahfalk

noahfalk Jul 6, 2017

Member

Is it fair to say you only need proposed API when you change method body drastically?

Nope : ) It sounds like you are analyzing this from the perspective of using ReJIT in the profiler, but ignore that part for the moment. Imagine that you have a simple profiler that never uses ReJIT or any form of IL instrumentation. The changes to the method body aren't coming from your profiler, they are coming from the runtime when it jits a method again using different jit optimization flags. If that hypothetical simple profiler was using GetILToNativeMapping or GetCodeInfo before, it probably needs to be using the new APIs now.

Does your profiler use GetILToNativeMapping or GetCodeInfo today?

Member

noahfalk commented Jul 6, 2017

Is it fair to say you only need proposed API when you change method body drastically?

Nope : ) It sounds like you are analyzing this from the perspective of using ReJIT in the profiler, but ignore that part for the moment. Imagine that you have a simple profiler that never uses ReJIT or any form of IL instrumentation. The changes to the method body aren't coming from your profiler, they are coming from the runtime when it jits a method again using different jit optimization flags. If that hypothetical simple profiler was using GetILToNativeMapping or GetCodeInfo before, it probably needs to be using the new APIs now.

Does your profiler use GetILToNativeMapping or GetCodeInfo today?

@SergeyKanzhelev

This comment has been minimized.

Show comment
Hide comment
@SergeyKanzhelev

SergeyKanzhelev commented Jul 6, 2017

no. Sorry

@noahfalk

This comment has been minimized.

Show comment
Hide comment
@noahfalk

noahfalk Jul 6, 2017

Member

Nothing to be sorry about - thats good, it means you probably don't need to worry about these changes.

Member

noahfalk commented Jul 6, 2017

Nothing to be sorry about - thats good, it means you probably don't need to worry about these changes.

@mjsabby

This comment has been minimized.

Show comment
Hide comment
@mjsabby

mjsabby Jul 25, 2017

Member

@noahfalk I think we're good on this, we do use the APIs you mentioned and I'll update it. Conceptually things should work for our profilers but have to make code changes.

Member

mjsabby commented Jul 25, 2017

@noahfalk I think we're good on this, we do use the APIs you mentioned and I'll update it. Conceptually things should work for our profilers but have to make code changes.

@lt72 lt72 added the enhancement label Oct 31, 2017

@lt72 lt72 added this to the 2.1.0 milestone Oct 31, 2017

@lt72 lt72 assigned davmason and unassigned noahfalk Nov 7, 2017

@lt72

This comment has been minimized.

Show comment
Hide comment
@lt72

lt72 Nov 7, 2017

Contributor

@davmason: this should be complete, but for the tests... ?

Contributor

lt72 commented Nov 7, 2017

@davmason: this should be complete, but for the tests... ?

@noahfalk

This comment has been minimized.

Show comment
Hide comment
@noahfalk

noahfalk Feb 6, 2018

Member

Looping back to this. We ran the tests and found a bug in the feature, which @kouvel has now fixed (#16145). Unfortunately this fix didn't make the Preview 1 build, but it should be available for any future preview builds or the 2.1 release build. I think all the work here is done now.

@davmason @lt72 @mjsabby

Member

noahfalk commented Feb 6, 2018

Looping back to this. We ran the tests and found a bug in the feature, which @kouvel has now fixed (#16145). Unfortunately this fix didn't make the Preview 1 build, but it should be available for any future preview builds or the 2.1 release build. I think all the work here is done now.

@davmason @lt72 @mjsabby

@noahfalk noahfalk closed this Feb 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment