Skip to content
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

Add support for user created buffers for raw profiler hooks #451

Merged
merged 5 commits into from
Nov 16, 2021

Conversation

wiktork
Copy link
Member

@wiktork wiktork commented Oct 22, 2021

Adds support for 3 different scenarios in raw profiler hooks:

  • SetILFunctionBody is called right after DefineMethod during ModuleLoad.
  • SetILFunctionBody is called right after DefineMethod during JitCompilationStarted.
  • SetILFunctionBody is called with a custom buffer (one not allocated with our FunctionBodyAllocator)

TODO:

  • GetInstrumentationResults (test only) may result on having a non-zero buffer.

@wiktork wiktork changed the title Dev/wiktork/custom buffer4 Add support for user created buffers for raw profiler hooks Oct 22, 2021
@delmyers
Copy link
Contributor

HRESULT MicrosoftInstrumentationEngine::CCorProfilerInfoWrapper::SetILFunctionBody(

Do we need to do something similar for SetILInstrumentedCodeMap? I.E. if there is no CMethodInfoInstance, and user buffers are enabled, then just directly set the code map?


Refers to: src/InstrumentationEngine/CorProfilerInfoWrapper.cpp:434 in c259b82. [](commit_id = c259b82, deletion_comment = False)

Copy link
Contributor

@delmyers delmyers left a comment

Choose a reason for hiding this comment

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

🕐

@WilliamXieMSFT
Copy link
Member

WilliamXieMSFT commented Oct 25, 2021

Please remember to update the changelog.md file


In reply to: 951147810

@wiktork
Copy link
Member Author

wiktork commented Oct 26, 2021

Do we need to do something similar for SetILInstrumentedCodeMap?

I don't think we need to set these for brand new methods, since there's no mapping from old il to new il. But you are correct, we will fail this call since we won't find the existing method.

@delmyers
Copy link
Contributor

Yeah, I don't know what the CLR will do in this case. Technically, I don't think that you are limited to setting the IL for a method at module load only if it is a brand new method. I can imagine a world where someone reads the IL from disk, creates a new method, and sets the code map before the first jit... but that is probably not a common use case.


In reply to: 952157974

@wiktork wiktork linked an issue Oct 29, 2021 that may be closed by this pull request
@wiktork wiktork force-pushed the dev/wiktork/customBuffer4 branch from c259b82 to 69a533d Compare October 29, 2021 06:21
@wiktork
Copy link
Member Author

wiktork commented Oct 29, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wiktork
Copy link
Member Author

wiktork commented Nov 3, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wiktork wiktork marked this pull request as ready for review November 3, 2021 18:22
@wiktork wiktork requested a review from a team as a code owner November 3, 2021 18:22
@@ -13,6 +13,8 @@ MicrosoftInstrumentationEngine::CCorMethodMalloc::CCorMethodMalloc() : m_cbBuffe

PVOID MicrosoftInstrumentationEngine::CCorMethodMalloc::Alloc(_In_ ULONG cb)
{
//TODO Any consecutive calls to Alloc will destroy the previous buffer.
Copy link
Member Author

Choose a reason for hiding this comment

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

Consider failing subsequent calls or leaking the buffer

Copy link
Member Author

Choose a reason for hiding this comment

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

leak + log

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like this is the default code flow; we don't have independent cleanup for this buffer, we rely on subsequent AllocCalls or ref counting to cleanup. I will leave this as is for now.

if (moduleId == m_testModule)
{
if (m_firstJit)
{
Copy link
Member Author

@wiktork wiktork Nov 3, 2021

Choose a reason for hiding this comment

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

Extra tests:

  • Double Alloc
  • Order between IM and Raw maintained
  • Order inversion with both modifying the method.
  • Make sure repeat calls to SetILFunctionBody behave as expected.

@wiktork wiktork force-pushed the dev/wiktork/customBuffer4 branch from 4185b6c to ef32601 Compare November 10, 2021 23:51
@wiktork
Copy link
Member Author

wiktork commented Nov 11, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@WilliamXieMSFT WilliamXieMSFT left a comment

Choose a reason for hiding this comment

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

:shipit:

@wiktork
Copy link
Member Author

wiktork commented Nov 12, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wiktork wiktork merged commit 0d1cfa7 into microsoft:main Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Raw profiler hook doesn't work with a profiler that adds methods
3 participants