Skip to content

Expose setting CPU SamplingProfiler rate #83635

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.

Sign up for GitHub

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

Closed
wants to merge 9 commits into from
Closed

Expose setting CPU SamplingProfiler rate #83635

Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f67de95
Expose setting CPU SamplingProfiler rate
pedrobsaila Mar 18, 2023
0120edd
fix build issues with _strtoui64
pedrobsaila Mar 18, 2023
389516c
include stdlib
pedrobsaila Mar 18, 2023
b160fe5
change to stroull
pedrobsaila Mar 18, 2023
6b3a71c
make function static
pedrobsaila Mar 18, 2023
ad342f8
fix warning on pointer cast
pedrobsaila Mar 18, 2023
5f5f1fb
another way of casting 2
pedrobsaila Mar 18, 2023
54d83a0
another way of casting 3
pedrobsaila Mar 18, 2023
40450b5
delete separate prototype
pedrobsaila Mar 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 43 additions & 1 deletion src/native/eventpipe/ep-sample-profiler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,17 @@ static
void
sample_profiler_enable (void);

static
void
ep_sample_event_pipe_callback(
Copy link
Member

Choose a reason for hiding this comment

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

Separate prototype isn't needed.

const uint8_t* source_id,
unsigned long is_enabled,
uint8_t level,
uint64_t match_any_keywords,
uint64_t match_all_keywords,
EventFilterDescriptor* filter_data,
void* callback_data);

/*
* EventPipeSampleProfiler.
*/
Expand Down Expand Up @@ -205,13 +216,44 @@ sample_profiler_enable (void)
}
}

static
void
ep_sample_event_pipe_callback(
const uint8_t* source_id,
unsigned long is_enabled,
uint8_t level,
uint64_t match_any_keywords,
uint64_t match_all_keywords,
EventFilterDescriptor* filter_data,
void* callback_data)
{
if (filter_data) {
ep_char8_t *filter_data_char = (ep_char8_t *)((uintptr_t)(filter_data->ptr));
Copy link
Member

Choose a reason for hiding this comment

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

Do I read that this is implemented as filter data that is passed to the provider? If so, I have a bit of a concern around how to de-duplicate requests when there are multiple sessions. Right now, this is a single value. If we are going to allow this to be specified on provider enable, we should probably have some behavior that de-conflicts requests. I'm not sure if that means taking the most frequent rate of all session requests, or something else. Another option is to consider having an API outside of enablement that sets this, at which point it can remain a global, and that is expressed at the API level.

const uint32_t filter_data_size = filter_data->size;
ep_char8_t *candidateKey = NULL;
size_t offset = 0;
const ep_char8_t *sampleProfilerIntervalMSKey = "SampleProfilerIntervalMS";

while (offset < filter_data_size) {
candidateKey = filter_data_char + offset;
if (strcmp(candidateKey, sampleProfilerIntervalMSKey) == 0) {
ep_sample_profiler_set_sampling_rate(strtoull(candidateKey + 25, NULL, 10) * 1000000);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should check that strtoull returns a value greater than 0. If someone passes in a bad value that cannot be parsed it would return 0 and the sample thread would sample continuously and busy lock the app.

Copy link
Member

@lateralusX lateralusX Apr 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We should probably even add logic to make sure the value falls within some accepted limits, so maybe a value between 1000 samples/second (current default and not sure we want to go lower than that with current sampling solution) and some lower limit that makes sense. You might want to sample quite infrequently in some cases, so not sure what the lower limit could be, so maybe we should just cap it at some sane value, like 1 sample/second, since its still a sample profiler that you configure. If you would like to get even more infrequent samples, then maybe it should be handled by a custom provider and not the sample profiler.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed that keeping a sane range is a good idea. For example, ETW will not allow you to sample any more than every 0.125 seconds if I recall correctly. I'm not super concerned about the other side of the range because there isn't a risk of affecting the availability of the app - there is a risk of not getting data. I think it's probably safe to allow any large value, though at a certain point it doesn't really provide much value for the scenarios that we are currently tracking.

break;
}
else {
offset += strlen(candidateKey) + 1;
}
}
}
}

void
ep_sample_profiler_init (EventPipeProviderCallbackDataQueue *provider_callback_data_queue)
{
ep_requires_lock_held ();

if (!_sampling_provider) {
_sampling_provider = provider_create_register (ep_config_get_sample_profiler_provider_name_utf8 (), NULL, NULL, provider_callback_data_queue);
_sampling_provider = provider_create_register (ep_config_get_sample_profiler_provider_name_utf8 (), ep_sample_event_pipe_callback, NULL, provider_callback_data_queue);
ep_raise_error_if_nok (_sampling_provider != NULL);
_thread_time_event = provider_add_event (
_sampling_provider,
Expand Down