Skip to content

[EventPipe] Optimize ThreadSessionState Management and Cleanup #118029

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
mdh1418 wants to merge 6 commits into from
Closed

[EventPipe] Optimize ThreadSessionState Management and Cleanup #118029

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f1b7a75
[EventPipe] Cleanup logic
mdh1418 Jul 22, 2025
29649ec
[EventPipe][BufferManager] Track thread_session_state
mdh1418 Jul 22, 2025
ce56ed8
[EventPipe] Directly store thread_id in seq point map
mdh1418 Jul 23, 2025
f930c15
[EventPipe] Revert v4 ThreadSessionState cleanup
mdh1418 Jul 23, 2025
393914e
[EventPipe] Move Sequence Point Map update
mdh1418 Jul 24, 2025
1fedd96
[EventPipe] Cleanup Exhausted ThreadSessionStates
mdh1418 Jul 24, 2025
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
2 changes: 2 additions & 0 deletions src/native/containers/dn-umap-t.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ dn_umap_ ## key_type_name ## _ ## value_type_name ## _find (dn_umap_t *map, key_
}

DN_UMAP_IT_KEY_T (ptr, void *)
DN_UMAP_IT_KEY_T (threadid, ep_rt_thread_id_t)
Copy link
Member

@lateralusX lateralusX Jul 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

You need to do this in ep code, since ep types are not shared between libraries that could use the container headers.


DN_UMAP_IT_VALUE_T (bool, bool)
DN_UMAP_IT_VALUE_T (int8_t, int8_t)
Expand All @@ -73,5 +74,6 @@ DN_UMAP_T (ptr, void *, uint16, uint16_t)
DN_UMAP_T (ptr, void *, int32, int32_t)
DN_UMAP_T (ptr, void *, uint32, uint32_t)
DN_UMAP_T (uint32, uint32_t, ptr, void *)
DN_UMAP_T (threadid, ep_rt_thread_id_t, uint32, uint32_t)
Copy link
Member

@lateralusX lateralusX Jul 28, 2025

Choose a reason for hiding this comment

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

Same as above, no ep types in container headers.


#endif /* __DN_UMAP_T_H__ */
4 changes: 2 additions & 2 deletions src/native/eventpipe/ep-block.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -906,10 +906,10 @@ ep_sequence_point_block_init (
const uint32_t thread_count = dn_umap_size (map);
ep_write_buffer_uint32_t (&sequence_point_block->block.write_pointer, thread_count);

DN_UMAP_FOREACH_BEGIN (const EventPipeThreadSessionState *, key, uint32_t, sequence_number, map) {
DN_UMAP_FOREACH_BEGIN (ep_rt_thread_id_t, key, uint32_t, sequence_number, map) {
ep_write_buffer_uint64_t (
&sequence_point_block->block.write_pointer,
ep_thread_get_os_thread_id (ep_thread_session_state_get_thread (key)));
ep_rt_thread_id_t_to_uint64_t (key));

ep_write_buffer_uint32_t (
&sequence_point_block->block.write_pointer,
Expand Down
243 changes: 109 additions & 134 deletions src/native/eventpipe/ep-buffer-manager.c
View file
Open in desktop

Large diffs are not rendered by default.

11 changes: 8 additions & 3 deletions src/native/eventpipe/ep-buffer-manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,6 @@ struct _EventPipeBufferList_Internal {
EventPipeBuffer *tail_buffer;
// The number of buffers in the list.
uint32_t buffer_count;
// The sequence number of the last event that was read, only
// updated/read by the reader thread.
uint32_t last_read_sequence_number;
};

#if !defined(EP_INLINE_GETTER_SETTER) && !defined(EP_IMPL_BUFFER_MANAGER_GETTER_SETTER)
Expand Down Expand Up @@ -113,6 +110,9 @@ struct _EventPipeBufferManager_Internal {
EventPipeEventInstance *current_event;
EventPipeBuffer *current_buffer;
EventPipeBufferList *current_buffer_list;
// The thread session state grabbed from the thread_session_state_list containing the current event
// that is being processed by the reader thread.
EventPipeThreadSessionState *current_thread_session_state;
// The total allocation size of buffers under management.
volatile size_t size_of_all_buffers;
// The maximum allowable size of buffers under management.
Expand Down Expand Up @@ -243,5 +243,10 @@ bool
ep_buffer_manager_ensure_consistency (EventPipeBufferManager *buffer_manager);
#endif

void
ep_buffer_manager_remove_thread_if_buffer_list_empty (
EventPipeBufferManager *buffer_manager,
EventPipeThreadSessionState *thread_session_state);

#endif /* ENABLE_PERFTRACING */
#endif /* __EVENTPIPE_BUFFERMANAGER_H__ */
7 changes: 0 additions & 7 deletions src/native/eventpipe/ep-event-instance.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,13 +229,6 @@ sequence_point_fini (EventPipeSequencePoint *sequence_point)
{
EP_ASSERT (sequence_point != NULL);

// Each entry in the map owns a ref-count on the corresponding thread
if (dn_umap_size (sequence_point->thread_sequence_numbers) != 0) {
DN_UMAP_FOREACH_KEY_BEGIN (EventPipeThreadSessionState *, key, sequence_point->thread_sequence_numbers) {
ep_thread_release (ep_thread_session_state_get_thread (key));
} DN_UMAP_FOREACH_END;
}

dn_umap_free (sequence_point->thread_sequence_numbers);
}

Expand Down
21 changes: 14 additions & 7 deletions src/native/eventpipe/ep-thread.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ ep_thread_alloc (void)
ep_rt_spin_lock_alloc (&instance->rt_lock);
ep_raise_error_if_nok (ep_rt_spin_lock_is_valid (&instance->rt_lock));

instance->os_thread_id = ep_rt_thread_id_t_to_uint64_t (ep_rt_current_thread_get_id ());
instance->os_thread_id = ep_rt_current_thread_get_id ();
memset (instance->session_state, 0, sizeof (instance->session_state));

instance->writing_event_in_progress = UINT32_MAX;
Expand All @@ -56,11 +56,10 @@ ep_thread_free (EventPipeThread *thread)

EP_ASSERT (ep_rt_volatile_load_uint32_t ((const volatile uint32_t *)&thread->ref_count) == 0);

#ifdef EP_CHECKED_BUILD
for (uint32_t i = 0; i < EP_MAX_NUMBER_OF_SESSIONS; ++i) {
EP_ASSERT (thread->session_state [i] == NULL);
ep_thread_session_state_free (thread->session_state [i]);
Copy link
Member

@noahfalk noahfalk Jul 25, 2025

Choose a reason for hiding this comment

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

I don't think thread->session_state[i] can ever be non-NULL here. The TSS has a ThreadHolder inside of it which maintains a ref count on the thread. As long as any TSS exists the ref count is > 0 and ep_thread_free() won't get called.

TSS objects are currently getting cleaned up during session cleanup and I proposed in another comment that we should also be doing it in remove_thread_if_buffer_list_empty().

thread->session_state [i] = NULL;
}
#endif

ep_rt_spin_lock_free (&thread->rt_lock);
ep_rt_object_free (thread);
Expand Down Expand Up @@ -123,6 +122,16 @@ ep_thread_unregister (EventPipeThread *thread)

ep_return_false_if_nok (thread != NULL);

for (uint32_t i = 0; i < EP_MAX_NUMBER_OF_SESSIONS; ++i) {
if (thread->session_state [i] == NULL)
continue;

EventPipeBufferManager *buffer_manager = ep_thread_session_state_get_buffer_manager (thread->session_state [i]);
EP_ASSERT (buffer_manager != NULL);

ep_buffer_manager_remove_thread_if_buffer_list_empty (buffer_manager, thread->session_state [i]);
}

bool found = false;
EP_SPIN_LOCK_ENTER (&_ep_threads_lock, section1)
// Remove ourselves from the global list
Expand Down Expand Up @@ -343,10 +352,7 @@ ep_thread_session_state_alloc (

instance->session = session;
instance->sequence_number = 1;

#ifdef EP_CHECKED_BUILD
instance->buffer_manager = buffer_manager;
#endif

ep_on_exit:
return instance;
Expand Down Expand Up @@ -375,6 +381,7 @@ ep_thread_session_state_free (EventPipeThreadSessionState *thread_session_state)
{
ep_return_void_if_nok (thread_session_state != NULL);
ep_thread_holder_fini (&thread_session_state->thread_holder);
ep_buffer_list_free (thread_session_state->buffer_list);
ep_rt_object_free (thread_session_state);
}

Expand Down
7 changes: 3 additions & 4 deletions src/native/eventpipe/ep-thread.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ struct _EventPipeThread_Internal {
ep_rt_spin_lock_handle_t rt_lock;
// This is initialized when the Thread object is first constructed and remains
// immutable afterwards.
uint64_t os_thread_id;
ep_rt_thread_id_t os_thread_id;
// The EventPipeThreadHolder maintains one count while the thread is alive
// and each session's EventPipeBufferList maintains one count while it
// exists.
Expand Down Expand Up @@ -73,7 +73,7 @@ ep_thread_get_activity_id_cref (ep_rt_thread_activity_id_handle_t activity_id_ha
EP_DEFINE_GETTER(EventPipeThread *, thread, EventPipeSession *, rundown_session);
EP_DEFINE_SETTER(EventPipeThread *, thread, EventPipeSession *, rundown_session);
EP_DEFINE_GETTER_REF(EventPipeThread *, thread, ep_rt_spin_lock_handle_t *, rt_lock);
EP_DEFINE_GETTER(EventPipeThread *, thread, uint64_t, os_thread_id);
EP_DEFINE_GETTER(EventPipeThread *, thread, ep_rt_thread_id_t, os_thread_id);
EP_DEFINE_GETTER_REF(EventPipeThread *, thread, int32_t *, ref_count);
EP_DEFINE_GETTER_REF(EventPipeThread *, thread, volatile uint32_t *, unregistered);

Expand Down Expand Up @@ -267,10 +267,8 @@ struct _EventPipeThreadSessionState_Internal {
// the buffer allocation logic is estimating how many
// buffers a given thread has used (see: ep_thread_session_state_get_buffer_count_estimate and its uses).
EventPipeBufferList *buffer_list;
Copy link
Member

@noahfalk noahfalk Jul 25, 2025

Choose a reason for hiding this comment

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

The comment on the field needs to be updated, its description of the lifetime is out-of-date.

#ifdef EP_CHECKED_BUILD
Copy link
Member

@noahfalk noahfalk Jul 25, 2025

Choose a reason for hiding this comment

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

I don't think we need to store this field? You can get the same value by calling ep_session_get_buffer_manager (tss->session)

// protected by the buffer manager lock.
EventPipeBufferManager *buffer_manager;
#endif
// The number of events that were attempted to be written by this
// thread. Each event was either successfully recorded in a buffer
// or it was dropped.
Expand Down Expand Up @@ -301,6 +299,7 @@ struct _EventPipeThreadSessionState {

EP_DEFINE_GETTER_REF(EventPipeThreadSessionState *, thread_session_state, EventPipeThreadHolder *, thread_holder)
EP_DEFINE_GETTER(EventPipeThreadSessionState *, thread_session_state, EventPipeSession *, session)
EP_DEFINE_GETTER(EventPipeThreadSessionState *, thread_session_state, EventPipeBufferManager *, buffer_manager)

EventPipeThreadSessionState *
ep_thread_session_state_alloc (
Expand Down
Loading