[EventPipe] Cleanup Exhausted ThreadSessionStates

Throughout an EventPipe Session's lifetime, any new thread that writes
an Event will induce a sequence of allocations including
EventPipeThread, EventPipeThreadSessionStates, EventPipeBufferList,
EventPipeBuffer, EventPipeSequencePoint, etc. Some of these objects are
held throughout the lifetime of the session instead of the scope of
their utility. Such behavior leads to a performance hit. For example,
the buffer_manager's thread_session_state_list is used to discover new
events, and without the proper cleanup, it will hold instances whose
buffers have been exhausted and may never received a new event.

To address a bloated thread_session_state_list, we add cleanup measures
to remove instances whose buffer_list is both empty and the
corresponding thread has been unregistered, indicating that the thread
has exited and will not write new events.

Author: mdh1418

src/native/eventpipe/ep-buffer-manager.c

@@ -124,6 +124,12 @@ buffer_manager_convert_buffer_to_read_only (
 	EventPipeBufferManager *buffer_manager,
 	EventPipeBuffer *new_read_buffer);
 
+static
+void
+buffer_manager_remove_and_delete_thread_session_state (
+	EventPipeBufferManager *buffer_manager,
+	EventPipeThreadSessionState *thread_session_state);
+
 /*
  * EventPipeBufferList.
  */
@@ -258,6 +264,14 @@ ep_buffer_list_get_and_remove_head (EventPipeBufferList *buffer_list)
 	return ret_buffer;
 }
 
+bool
+ep_buffer_list_is_empty (const EventPipeBufferList *buffer_list)
+{
+	EP_ASSERT (buffer_list != NULL);
+	EP_ASSERT (ep_buffer_list_ensure_consistency ((EventPipeBufferList *)buffer_list));
+	return buffer_list->head_buffer == NULL;
+}
+
 bool
 buffer_manager_try_reserve_buffer (
 	EventPipeBufferManager *buffer_manager,
@@ -701,6 +715,17 @@ buffer_manager_advance_to_non_empty_buffer (
 
 				// get the next buffer
 				current_buffer = buffer_list->head_buffer;
+
+				if (!current_buffer && (ep_rt_volatile_load_uint32_t_without_barrier (ep_thread_get_unregistered_ref (ep_thread_session_state_get_thread (thread_session_state))) > 0)) {
+					if (buffer_manager->sequence_point_alloc_budget != 0) {
+						// The next sequence point needs to update the sequence number for this thread
+						// Delay the thread session state removal until the next sequence point is written.
+						ep_thread_session_state_set_delete_deferred (thread_session_state, true);
+					} else {
+						buffer_manager_remove_and_delete_thread_session_state (buffer_manager, thread_session_state);
+					}
+				}
+
 				if (!current_buffer || ep_buffer_get_creation_timestamp (current_buffer) >= before_timestamp) {
 					// no more buffers in the list before this timestamp, we're done
 					current_buffer = NULL;
@@ -1197,19 +1222,8 @@ ep_buffer_manager_write_all_buffers_to_file_v4 (
 
 				it = dn_list_it_next (it);
 
-				// if a session_state was exhausted during this sequence point, mark it for deletion
-				if (ep_thread_session_state_get_buffer_list (session_state)->head_buffer == NULL) {
-					// We don't hold the thread lock here, so it technically races with a thread getting unregistered. This is okay,
-					// because we will either not have passed the above if statement (there were events still in the buffers) or we
-					// will catch it at the next sequence point.
-					EventPipeThread* thread = ep_thread_session_state_get_thread (session_state);
-					EventPipeSession* session = ep_thread_session_state_get_session (session_state);
-					if (ep_rt_volatile_load_uint32_t_without_barrier (ep_thread_get_unregistered_ref (thread)) > 0) {
-						dn_vector_ptr_push_back (&session_states_to_delete, session_state);
-						dn_list_remove (buffer_manager->thread_session_state_list, session_state);
-						ep_thread_set_session_state (thread, session, NULL);
-					}
-				}
+				if (ep_thread_session_state_get_delete_deferred (session_state))
+					dn_vector_ptr_push_back (&session_states_to_delete, session_state);
 			}
 		EP_SPIN_LOCK_EXIT (&buffer_manager->rt_lock, section2)
 
@@ -1218,35 +1232,12 @@ ep_buffer_manager_write_all_buffers_to_file_v4 (
 
 		// we are done with the sequence point
 		EP_SPIN_LOCK_ENTER (&buffer_manager->rt_lock, section3)
-			buffer_manager_dequeue_sequence_point (buffer_manager);
-		EP_SPIN_LOCK_EXIT (&buffer_manager->rt_lock, section3)
-	}
-
-	// There are sequence points created during this flush and we've marked session states for deletion.
-	// We need to remove these from the internal maps of the subsequent Sequence Points
-	if (dn_vector_ptr_size (&session_states_to_delete) > 0) {
-		EP_SPIN_LOCK_ENTER (&buffer_manager->rt_lock, section4)
-			if (buffer_manager_try_peek_sequence_point (buffer_manager, &sequence_point)) {
-				DN_LIST_FOREACH_BEGIN (EventPipeSequencePoint *, current_sequence_point, buffer_manager->sequence_points) {
-					// foreach (session_state in session_states_to_delete)
-					DN_VECTOR_PTR_FOREACH_BEGIN (EventPipeThreadSessionState *, thread_session_state, &session_states_to_delete) {
-						dn_umap_it_t found = dn_umap_ptr_uint32_find (ep_sequence_point_get_thread_sequence_numbers (current_sequence_point), thread_session_state);
-						if (!dn_umap_it_end (found)) {
-							dn_umap_erase (found);
-							// every entry of this map was holding an extra ref to the thread (see: ep-event-instance.{h|c})
-							ep_thread_release (ep_thread_session_state_get_thread (thread_session_state));
-						}
-					} DN_VECTOR_PTR_FOREACH_END;
-				} DN_LIST_FOREACH_END;
-			}
-
-			// foreach (thread_session_state in session_states_to_delete)
 			DN_VECTOR_PTR_FOREACH_BEGIN (EventPipeThreadSessionState *, thread_session_state, &session_states_to_delete) {
-				EP_ASSERT (thread_session_state != NULL);
-				ep_thread_session_state_free (thread_session_state);
+				if (ep_thread_session_state_get_delete_deferred (thread_session_state))
+					buffer_manager_remove_and_delete_thread_session_state (buffer_manager, thread_session_state);
 			} DN_VECTOR_PTR_FOREACH_END;
-
-		EP_SPIN_LOCK_EXIT (&buffer_manager->rt_lock, section4)
+			buffer_manager_dequeue_sequence_point (buffer_manager);
+		EP_SPIN_LOCK_EXIT (&buffer_manager->rt_lock, section3)
 	}
 
 ep_on_exit:
@@ -1330,6 +1321,73 @@ ep_buffer_manager_ensure_consistency (EventPipeBufferManager *buffer_manager)
 }
 #endif
 
+static
+void
+buffer_manager_remove_thread_session_state_from_sequence_points (
+	EventPipeBufferManager *buffer_manager,
+	EventPipeThreadSessionState *thread_session_state)
+{
+	EP_ASSERT (buffer_manager != NULL);
+	EP_ASSERT (thread_session_state != NULL);
+
+	ep_buffer_manager_requires_lock_held (buffer_manager);
+
+	DN_LIST_FOREACH_BEGIN (EventPipeSequencePoint *, current_sequence_point, buffer_manager->sequence_points) {
+		dn_umap_it_t found = dn_umap_ptr_uint32_find (ep_sequence_point_get_thread_sequence_numbers (current_sequence_point), thread_session_state);
+		if (!dn_umap_it_end (found)) {
+			dn_umap_erase (found);
+			// buffer_manager_init_sequence_point_thread_list - every entry of this map was holding an extra ref to the thread
+			ep_thread_release (ep_thread_session_state_get_thread (thread_session_state));
+		}
+	} DN_LIST_FOREACH_END;
+}
+
+static
+void
+buffer_manager_remove_and_delete_thread_session_state (
+	EventPipeBufferManager *buffer_manager,
+	EventPipeThreadSessionState *thread_session_state)
+{
+	EP_ASSERT (buffer_manager != NULL);
+	EP_ASSERT (thread_session_state != NULL);
+
+	ep_buffer_manager_requires_lock_held (buffer_manager);
+
+	dn_list_remove (buffer_manager->thread_session_state_list, thread_session_state);
+	ep_thread_set_session_state (ep_thread_session_state_get_thread (thread_session_state), ep_thread_session_state_get_session (thread_session_state), NULL);
+	buffer_manager_remove_thread_session_state_from_sequence_points (buffer_manager, thread_session_state);
+	ep_thread_session_state_free (thread_session_state);
+}
+
+void
+ep_buffer_manager_remove_and_delete_thread_session_state (
+	EventPipeBufferManager *buffer_manager,
+	EventPipeThreadSessionState *thread_session_state)
+{
+	EP_ASSERT (buffer_manager != NULL);
+	EP_ASSERT (thread_session_state != NULL);
+
+	ep_buffer_manager_requires_lock_not_held (buffer_manager);
+
+	EP_SPIN_LOCK_ENTER (&buffer_manager->rt_lock, section1)
+		buffer_manager_remove_and_delete_thread_session_state (buffer_manager, thread_session_state);
+	EP_SPIN_LOCK_EXIT (&buffer_manager->rt_lock, section1)
+
+ep_on_exit:
+	ep_buffer_manager_requires_lock_not_held (buffer_manager);
+	return;
+
+ep_on_error:
+	ep_exit_error_handler ();
+}
+
+bool
+ep_buffer_manager_uses_sequence_points (const EventPipeBufferManager *buffer_manager)
+{
+	EP_ASSERT (buffer_manager != NULL);
+	return buffer_manager->sequence_point_alloc_budget != 0;
+}
+
 #endif /* !defined(EP_INCLUDE_SOURCE_FILES) || defined(EP_FORCE_INCLUDE_SOURCE_FILES) */
 #endif /* ENABLE_PERFTRACING */
 

src/native/eventpipe/ep-buffer-manager.h

@@ -74,6 +74,9 @@ ep_buffer_list_insert_tail (
 EventPipeBuffer *
 ep_buffer_list_get_and_remove_head (EventPipeBufferList *buffer_list);
 
+bool
+ep_buffer_list_is_empty (const EventPipeBufferList *buffer_list);
+
 #ifdef EP_CHECKED_BUILD
 bool
 ep_buffer_list_ensure_consistency (EventPipeBufferList *buffer_list);
@@ -228,5 +231,13 @@ bool
 ep_buffer_manager_ensure_consistency (EventPipeBufferManager *buffer_manager);
 #endif
 
+void
+ep_buffer_manager_remove_and_delete_thread_session_state (
+	EventPipeBufferManager *buffer_manager,
+	EventPipeThreadSessionState *thread_session_state);
+
+bool
+ep_buffer_manager_uses_sequence_points (const EventPipeBufferManager *buffer_manager);
+
 #endif /* ENABLE_PERFTRACING */
 #endif /* __EVENTPIPE_BUFFERMANAGER_H__ */

src/native/eventpipe/ep-thread.c

@@ -119,6 +119,22 @@ ep_thread_unregister (EventPipeThread *thread)
 
 	ep_return_false_if_nok (thread != NULL);
 
+	for (uint32_t i = 0; i < EP_MAX_NUMBER_OF_SESSIONS; ++i) {
+		EventPipeThreadSessionState *thread_session_state = (EventPipeThreadSessionState *)ep_rt_volatile_load_ptr ((volatile void **)(&thread->session_state [i]));
+		if (thread_session_state == NULL)
+			continue;
+
+		EventPipeBufferManager *buffer_manager = ep_session_get_buffer_manager (thread_session_state->session);
+		EP_ASSERT (buffer_manager != NULL);
+
+		if (ep_buffer_list_is_empty (thread_session_state->buffer_list)) {
+			if (ep_buffer_manager_uses_sequence_points (buffer_manager))
+				thread_session_state->delete_deferred = true;
+			else
+				ep_buffer_manager_remove_and_delete_thread_session_state (buffer_manager, thread_session_state);
+		}
+	}
+
 	bool found = false;
 	EP_SPIN_LOCK_ENTER (&_ep_threads_lock, section1)
 		// Remove ourselves from the global list
@@ -321,9 +337,8 @@ ep_thread_session_state_alloc (
 
 	instance->buffer_list = ep_buffer_list_alloc (buffer_manager, thread);
 	ep_raise_error_if_nok (instance->buffer_list != NULL);
-#ifdef EP_CHECKED_BUILD
-	instance->buffer_manager = buffer_manager;
-#endif
+
+	instance->delete_deferred = false;
 
 ep_on_exit:
 	return instance;
@@ -367,7 +382,7 @@ EventPipeBuffer *
 ep_thread_session_state_get_write_buffer (const EventPipeThreadSessionState *thread_session_state)
 {
 	EP_ASSERT (thread_session_state != NULL);
-	ep_buffer_manager_requires_lock_held (thread_session_state->buffer_manager);
+	ep_buffer_manager_requires_lock_held (ep_session_get_buffer_manager (thread_session_state->session));
 
 	EP_ASSERT ((thread_session_state->write_buffer == NULL) || (ep_buffer_get_volatile_state (thread_session_state->write_buffer) == EP_BUFFER_STATE_WRITABLE));
 	return (EventPipeBuffer*) ep_rt_volatile_load_ptr_without_barrier ((volatile void **)&thread_session_state->write_buffer);
@@ -388,7 +403,7 @@ ep_thread_session_state_set_write_buffer (
 {
 	EP_ASSERT (thread_session_state != NULL);
 
-	ep_buffer_manager_requires_lock_held (thread_session_state->buffer_manager);
+	ep_buffer_manager_requires_lock_held (ep_session_get_buffer_manager (thread_session_state->session));
 
 	EP_ASSERT ((new_buffer == NULL) || (ep_buffer_get_volatile_state (new_buffer) == EP_BUFFER_STATE_WRITABLE));
 	EP_ASSERT ((thread_session_state->write_buffer == NULL) || (ep_buffer_get_volatile_state (thread_session_state->write_buffer) == EP_BUFFER_STATE_WRITABLE));
@@ -401,7 +416,7 @@ ep_thread_session_state_get_buffer_list (const EventPipeThreadSessionState *thre
 {
 	EP_ASSERT (thread_session_state != NULL);
 
-	ep_buffer_manager_requires_lock_held (thread_session_state->buffer_manager);
+	ep_buffer_manager_requires_lock_held (ep_session_get_buffer_manager (thread_session_state->session));
 
 	return thread_session_state->buffer_list;
 }

src/native/eventpipe/ep-thread.h

@@ -273,10 +273,6 @@ struct _EventPipeThreadSessionState_Internal {
 	// The sequence number of the last event that was read, only
 	// updated/read by the reader thread.
 	uint32_t last_read_sequence_number;
-#ifdef EP_CHECKED_BUILD
-	// 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.
@@ -297,6 +293,15 @@ struct _EventPipeThreadSessionState_Internal {
 	// buffers are later read and there are fewer than X events timestamped
 	// prior to the sequence point we can be certain the others were dropped.
 	volatile uint32_t sequence_number;
+	// An ThreadSessionState is no longer needed when the following are met:
+	// 1. The thread is unregistered - No more events can be written.
+	// 2. The buffers are empty - No more events need to be flushed.
+	// 3. The following SequencePoint is written - No more mapped sequence
+	//    numbers need to be updated.
+	// If this is set to true, that means we have detected 1 and 2 through
+	// buffer_manager_advance_to_non_empty_buffer, but sequence points are active,
+	// so we defer the deletion until the next sequence point updates its map.
+	bool delete_deferred;
 };
 
 #if !defined(EP_INLINE_GETTER_SETTER) && !defined(EP_IMPL_THREAD_GETTER_SETTER)
@@ -307,8 +312,10 @@ 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, uint32_t, last_read_sequence_number);
-EP_DEFINE_SETTER(EventPipeThreadSessionState *, thread_session_state, uint32_t, last_read_sequence_number);
+EP_DEFINE_GETTER(EventPipeThreadSessionState *, thread_session_state, uint32_t, last_read_sequence_number)
+EP_DEFINE_SETTER(EventPipeThreadSessionState *, thread_session_state, uint32_t, last_read_sequence_number)
+EP_DEFINE_GETTER(EventPipeThreadSessionState *, thread_session_state, bool, delete_deferred)
+EP_DEFINE_SETTER(EventPipeThreadSessionState *, thread_session_state, bool, delete_deferred)
 
 EventPipeThreadSessionState *
 ep_thread_session_state_alloc (