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

profiler changes for tiered compilation #14612

Merged
merged 5 commits into from Oct 24, 2017

Conversation

Projects
None yet
3 participants
@davmason
Member

davmason commented Oct 20, 2017

Adds new profiler APIs for tiered compilation support. Also as part of this change I updated ProfToEEInterfaceImpl::GetILToNativeMapping2 to properly respect the requested rejit ID.

@davmason davmason self-assigned this Oct 20, 2017

@davmason davmason requested a review from noahfalk Oct 20, 2017

@@ -2595,21 +2599,16 @@ HRESULT ProfToEEInterfaceImpl::GetCodeInfo3(FunctionID functionId,
CodeVersionManager* pCodeVersionManager = pMethodDesc->GetCodeVersionManager();
ILCodeVersion ilCodeVersion = pCodeVersionManager->GetILCodeVersion(pMethodDesc, reJitId);
// Now that tiered compilation can create more than one jitted code version for the same rejit id
// we are arbitrarily choosing the first one to return. To return all of them we'd presumably need
// a new profiler API.

This comment has been minimized.

@noahfalk

noahfalk Oct 20, 2017

Member

This comment is still correct - we are arbitrarily choosing an implementation. The comment could also mention the new APIs you've added rather than refer to hypothetical new APIs.

This comment has been minimized.

@davmason
pCodeVersionManager->GetILCodeVersion(pMD, reJitId);
}

This comment has been minimized.

@noahfalk

noahfalk Oct 20, 2017

Member

Lets add another comment to indicate this is an arbitrarily chosen version and profilers are recommended to use GetILToNativeMapping3 to avoid that ambiguity.

This comment has been minimized.

@davmason

davmason Oct 20, 2017

Member

Ok, sounds good

@@ -6583,6 +6592,190 @@ HRESULT ProfToEEInterfaceImpl::GetDynamicFunctionInfo(FunctionID functionId,
}
/*
* GetNativeCodeStartAddresses
*
* TODO: Description

This comment has been minimized.

@noahfalk

noahfalk Oct 20, 2017

Member

TODO : )

This comment has been minimized.

@davmason

davmason Oct 20, 2017

Member

Whoops! Taken care of now

ULONG32 cCodeStartAddresses,
ULONG32 *pcCodeStartAddresses,
UINT_PTR codeStartAddresses[])
{

This comment has been minimized.

@noahfalk

noahfalk Oct 20, 2017

Member

Check out GetModuleInfo2 for example of how it deals with the variable size character buffer and the associated size in/out parameters. I know there aren't a ton of total examples in the profiler API and we already aren't strictly consistent which adds to the confusion, but I think GetModuleInfo2's behavior is a reasonable standard to emulate.

This comment has been minimized.

@davmason

davmason Oct 20, 2017

Member

Ok, I made it so that pcCodeStartAddresses and codeStartAddresses are optional. If that's not what you meant, let me know.

This comment has been minimized.

@noahfalk

noahfalk Oct 21, 2017

Member

I was thinking of the different ways you can call GetModuleInfo2 that don't work here yet:

  • You can call with only pcchName/pcCodeStartAddresses non-null and receive the size of the buffer that would be needed (here this only works if the caller also allocated a sufficiently large buffer)
  • You can call with pcchName/pcCodeStartAddresses null if you don't care about the size (looks like you still go to E_INVALIDARG in this case)

This comment has been minimized.

@davmason

davmason Oct 21, 2017

Member

Ok, I see what you're saying now. I missed it the first time. I just updated it to be able to call with just with either pcCodeAddresses or codeStartAddresses null now and the other will be filled out appropriately

ILCodeVersion ilCodeVersion = NULL;
{
CodeVersionManager::TableLockHolder lockHolder(pCodeVersionManager);

This comment has been minimized.

@noahfalk

noahfalk Oct 20, 2017

Member

I'd put the entire enumeration under the lock. Did anything lead you to avoid putting it under the lock (we might want to change whatever that was too)

This comment has been minimized.

@davmason

davmason Oct 20, 2017

Member

I forgot to add the lock initially, then when I ran the tests only GetILCodeversion had an assert about the lock so I just put the it around that one place.

I have changed it to be around the whole enumeration now though

/*
* GetILToNativeMapping3
*
* TODO: Description

This comment has been minimized.

@noahfalk

noahfalk Oct 20, 2017

Member

Another todo to do

This comment has been minimized.

@davmason
@@ -577,6 +577,29 @@ class ProfToEEInterfaceImpl : public ICorProfilerInfo8
// end ICorProfilerInfo8
// beging ICorProfiler9

This comment has been minimized.

@noahfalk

noahfalk Oct 20, 2017

Member

begin ICorProfilerInfo9

This comment has been minimized.

@davmason

davmason Oct 20, 2017

Member

thanks, done

ULONG32* pcCodeInfos,
COR_PRF_CODE_INFO codeInfos[]);
// end ICorProfiler9

This comment has been minimized.

@noahfalk

noahfalk Oct 20, 2017

Member

ICorProfilerInfo9

This comment has been minimized.

@davmason

davmason Oct 20, 2017

Member

done too

@@ -2595,21 +2599,16 @@ HRESULT ProfToEEInterfaceImpl::GetCodeInfo3(FunctionID functionId,
CodeVersionManager* pCodeVersionManager = pMethodDesc->GetCodeVersionManager();

This comment has been minimized.

@noahfalk

noahfalk Oct 20, 2017

Member

I notice a pre-existing bug - this code path didn't grab the code version manager lock and it needs to

This comment has been minimized.

@noahfalk

noahfalk Oct 20, 2017

Member

If you run the tests with a debug build it should give you asserts anywhere you are missing the lock.

This comment has been minimized.

@davmason

davmason Oct 20, 2017

Member

Thanks. I haven't run the existing profiler tests yet, but in general I am running with debug coreclr so I should catch these once I do.

pCodeVersionManager->GetILCodeVersion(pMD, reJitId);
}
NativeCodeVersionCollection nativeCodeVersions = ilCodeVersion.GetNativeCodeVersions(pMD);

This comment has been minimized.

@noahfalk

noahfalk Oct 20, 2017

Member

The whole enumeration should be inside the lock (but you can leave the call to GetILtoNativeMapping3 outside of it)

This comment has been minimized.

@davmason

davmason Oct 20, 2017

Member

Ok, done

{
for(ULONG32 i = 0; i < cCodeStartAddresses; ++i)
{
codeStartAddresses[i] = addresses[i];

This comment has been minimized.

@noahfalk

noahfalk Oct 23, 2017

Member

You've got a buffer overrun here. cCodeAddresses can be larger than trueLen but addresses only has length trueLen.

@noahfalk

This comment has been minimized.

Member

noahfalk commented Oct 23, 2017

LGTM, modulo that one remaining buffer overrun

@davmason davmason merged commit 5d66791 into dotnet:master Oct 24, 2017

16 checks passed

CROSS Check Build finished.
Details
CentOS7.1 x64 Checked Innerloop Build and Test Build finished.
Details
CentOS7.1 x64 Debug Innerloop Build Build finished.
Details
OSX10.12 x64 Checked Innerloop Build and Test Build finished.
Details
Perf Build and Test Build finished.
Details
Ubuntu arm64 Cross Debug Innerloop Build Build finished.
Details
Ubuntu armlb Cross Debug Innerloop Build Build finished.
Details
Ubuntu x64 Checked Innerloop Build and Test Build finished.
Details
Ubuntu x64 Innerloop Formatting Build finished.
Details
Ubuntu16.04 armlb Cross Debug Innerloop Build Build finished.
Details
WIP ready for review
Details
Windows_NT x64 Checked Innerloop Build and Test Build finished.
Details
Windows_NT x64 Innerloop Formatting Build finished.
Details
Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 Checked Innerloop Build and Test Build finished.
Details
Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details

@davmason davmason deleted the davmason:new_apis branch Oct 24, 2017

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