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

[mono][eventpipe] Implementing MethodDetails and BulkType events #68571

Merged
merged 28 commits into from
May 19, 2022

Conversation

mdh1418
Copy link
Member

@mdh1418 mdh1418 commented Apr 26, 2022

This PR looks to implement MethodDetails and BulkType trace events on mono, closely following the implementation of src/coreclr/vm/eventtrace.cpp's SendMethodDetailsEvent and FireBulkTypeEvent. In doing so, apps running on mono can be traced following steps from https://github.com/dotnet/runtime/blob/main/docs/design/mono/diagnostics-tracing.md and produce a .nettrace file with relevant MethodDetails and BulkType events. This .nettrace containing MethodDetails and BulkType events is necessary to leverage the dotnet-pgo tool to generate a corresponding .mibc file for future consumption in the mono AOT compiler.

This PR makes the following changes:

  • Implements MethodDetails events in ep_rt_mono_send_method_details_event (2nd commit)
  • Implements BulkType events in ep_rt_mono_fire_bulk_type_event (4th commit)
  • Extends LttngDataTypeMapping to have a mono friendly mapping (1st commit)
  • Utilizes mono method signature getter to avoid accessing method signatures prior to creation (3rd commit)

Creating a .mibc on mono from the HelloWorld sample

  1. Building mono+libs
  2. Building the HelloWorld sample with make run
  3. Generating a trace
    3a. DOTNET_DiagnosticPorts="/Users/mdhwang/myport,suspend" <path to app executable>
    3b. dotnet-trace collect --providers Microsoft-Windows-DotNETRuntime:0x1F000080018:5 --diagnostic-port ~/myport
  4. dotnet-pgo create-mibc --trace <path-to-trace> (built with a dotnet-pgo tool with AddAssembliesAssociatedWithType modified to skip generic parameters)
if (type.IsGenericParameter)
  return;

Modification no longer needed.


Implementation Details

For the most part, the implementation of the relevant functions are similar to CoreCLR. Below are some key differences

ep_rt_mono_send_method_details_event

Utilizes a helper get_typeid_for_type in order to obtain a unique type ID, to fulfill the niche of CoreCLR's type handle

ep_rt_mono_log_type_and_parameters_if_necessary

Decided to not implement the hash table until it will actually be used, which seems to be when ep_rt_mono_log_type_and_parameters_if_necessary is used outside of ep_rt_mono_send_method_details_event

ep_rt_mono_log_type_and_parameters

In line with LogTypeAndParameters except for the absence of typeLogBehavior, for currently this is only called following ep_rt_mono_send_method_details_event which always logs.

ep_rt_mono_log_single_type

Fills in rg_type_parameters and c_type_parameters by three categories of MonoType 1) MONO_TYPE_ARRAY MONO_TYPE_SZARRAY 2) MONO_TYPE_GENERICINST 3)MONO_TYPE_CLASS MONO_TYPE_VALUETYPE MONO_TYPE_PTR MONO_TYPE_BYREF for any embedded type parameters. Does not sets_nameand keeps it asNULL` until the data will actually be used in mono, which GC Heap Dumps are a likely candidate for.

ep_rt_mono_fire_bulk_type_event

Copies to the BulkTypeEvent buffer through helper functions.

Various helper functions and static const variables have been added in addition to BulkTypeValue and BulkTypeEventLogger structs with respective memory management init/fini/clear functions.


TODO/Questions

Does sName in eventtrace.cpp need to be carried over?
We have opted to keep s_name as NULL until it is actually used in mono, which GC Heap Dumps are a likely candidate for.

Variable size array for rg_type_parameters in BulkTypeValue struct that already has variable size field s_name
Figured out that StackSArray<ULONGLONG> has a fixed size that evaluates to 32

Where else to emit ep_rt_mono_send_method_details_event?

How necessary are the 0x06 method token mask and 0x02 type token masks, should dotnet-pgo be tweaked instead?
Found the equivalent for mono using mono_metadata_make_token and mono_metadata_token_index

How to implement if_necessary portion of LogTypeAndParametersIfNecessary (th.IsRestored and what exactly is client usage of LogTypeAndParametersIfNecessary)
Decided to not implement the hash table until it will actually be used, which seems to be when ep_rt_mono_log_type_and_parameters_if_necessary is used outside of ep_rt_mono_send_method_details_event

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

Good start. Could be a little bit less copy-pasty.

The biggest issue is the use of m_class_is_byreflike when you mean to look at MonoType:byref.

Also I'm concerned about locking and generally accessing the bulk types buffer from multiple threads. If multiple threads are writing and any one of them could fire the buffer, there's no guarantee that the other threads finished populating their entry.

I would suggest using a simple mutex to only allow one function to access the buffers at a time (basically make ep_rt_mono_log_type_and_parameters take a mutex),

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 26, 2022
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 27, 2022
@mdh1418 mdh1418 force-pushed the enable_mono_method_details_event branch from 85e8aaa to 8450ced Compare April 27, 2022 20:14
@mdh1418 mdh1418 requested a review from josalem April 27, 2022 20:26
@mdh1418 mdh1418 force-pushed the enable_mono_method_details_event branch from 8450ced to 3e2fa00 Compare April 29, 2022 05:03
@mdh1418 mdh1418 force-pushed the enable_mono_method_details_event branch from dc14adc to 43ea8b6 Compare May 5, 2022 04:53
Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

It's coming together

The main remaining question is around the memory management of s_name.

Also some nits about intptr_t (it's by definition either 8 or 4 bytes, depending on the architecture. so it's not a great idea to use it as a wire format type)

Set BulkTypeValue s_name to null for now

Fixed debug trace formatting for pointers

Fix type_name_id for types with no token

Remove write_event_buffer_intptr as the size is different on Windows x86

Rename ep_rt_bulk_type_value_init to ep_rt_bulk_type_value_clear
Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

Mono bits LGTM. @josalem or @lateralusX any other issues in the generator scripts or in ep-rt-mono that you think need to be addressed in this PR?

mdh1418 added 2 commits May 5, 2022 18:18
Cleanup formatting and style

Remove unused write_event_buffer_utf16_str

Add BulkTypeEventLogger pointer to function description
@mdh1418 mdh1418 requested a review from thaystg as a code owner May 5, 2022 22:36
Change ep_rt_mono_get_byte_count_in_event type
Copy link
Member

@lateralusX lateralusX left a comment

Choose a reason for hiding this comment

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

Overall it looks good, nice work! A couple of smaller things noted. I also think you should do a pass and drop all the variable prefixes used (p_, s_ m_ rg_ etc), its not Mono code style using that, and I assume you pretty much took variable names from CoreCLR C++ implementation and used them in this PR.

mdh1418 added 4 commits May 6, 2022 12:28
Drop variable prefixes

Clear BulkTypeValue via memset

Rename bulk_type_event_logger helpers to alloc and free

Remove redundant bulk type value byte count calculation

Utilize memcpy to copy type parameter arrays

Reduce padding by moving bulk_type_event_buffer field in struct
…ds from mono types when needed

Change type id type from intptr_t to uint64_t as ids in eventpipe are standardized to uint64_t
…namically allocate enough space for type parameters
Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

small quality of life suggestion

@mdh1418
Copy link
Member Author

mdh1418 commented May 16, 2022

Looks like all the default lanes built successfully (except a Browser wasm windows lane on runtime-staging that timed out after 3 hrs).

Looking at how using lttngDataTypeMapping on both mono + coreclr works for all platforms through CI as it appeared to work fine on OSX mono/coreclr. Looks like atleast ep_char8_t is problematic for coreclr, so keeping the mappings separate for mono and coreclr.

@josalem could I get another 👀 at this PR, looking to merge as soon as we can

@mdh1418 mdh1418 force-pushed the enable_mono_method_details_event branch from f7a60d4 to 49d7366 Compare May 16, 2022 14:52
@mdh1418
Copy link
Member Author

mdh1418 commented May 16, 2022

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@josalem josalem left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@mdh1418
Copy link
Member Author

mdh1418 commented May 19, 2022

The failures look unrelated to this PR

@mdh1418 mdh1418 merged commit bfc7359 into dotnet:main May 19, 2022
@mdh1418 mdh1418 deleted the enable_mono_method_details_event branch May 19, 2022 16:26
@mdh1418
Copy link
Member Author

mdh1418 commented May 19, 2022

Thanks @lambdageek @lateralusX @josalem for all of your help!

@ghost ghost locked as resolved and limited conversation to collaborators Jun 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants