[EventPipe] Optimize SequencePoint maps and TSS cleanup

Now that EventPipeThreads lifetimes do not depend on SequencePoint
lifetimes, the following can be optimized:

1) SequencePoints directly map native thread ids
There is no dependence on keeping a thread alive, and so ref counting
can be removed and thread ids can be used as keys.

2) ThreadSessionStates can be cleaned up without waiting for sequence
   points that refer to the thread id to be written

Author: mdh1418

src/native/eventpipe/ep-block.c

@@ -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,

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

@@ -429,8 +429,8 @@ buffer_manager_init_sequence_point_thread_list (
 			ep_thread_session_state_set_track_state (thread_session_state, EP_SEQUENCE_POINT_THREAD_ID_TRACKED);
 		}
 
-		dn_umap_ptr_uint32_insert (ep_sequence_point_get_thread_sequence_numbers (sequence_point), thread_session_state, sequence_number);
-		ep_thread_addref (ep_thread_holder_get_thread (ep_thread_session_state_get_thread_holder_ref (thread_session_state)));
+		ep_rt_thread_id_t os_thread_id = ep_thread_get_os_thread_id (ep_thread_session_state_get_thread (thread_session_state));
+		dn_umap_threadid_uint32_insert (ep_sequence_point_get_thread_sequence_numbers (sequence_point), os_thread_id, sequence_number);
 	} DN_LIST_FOREACH_END;
 
 	// This needs to come after querying the thread sequence numbers to ensure that any recorded
@@ -736,15 +736,8 @@ 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_rt_volatile_load_uint32_t_without_barrier (ep_thread_get_unregistered_ref (ep_thread_session_state_get_thread (thread_session_state))) > 0))
+					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
@@ -1188,9 +1181,10 @@ ep_buffer_manager_write_all_buffers_to_file_v4 (
 			if (buffer_manager->current_event == NULL)
 				break;
 
-			uint64_t capture_thread_id = ep_thread_get_os_thread_id (ep_buffer_get_writer_thread (buffer_manager->current_buffer));
+			uint64_t capture_thread_id = ep_rt_thread_id_t_to_uint64_t (ep_thread_get_os_thread_id (ep_buffer_get_writer_thread (buffer_manager->current_buffer)));
 
 			EventPipeThreadSessionState *current_thread_session_state = buffer_manager->current_thread_session_state;
+			ep_rt_thread_id_t os_thread_id = ep_thread_get_os_thread_id (ep_thread_session_state_get_thread (current_thread_session_state));
 
 			// loop across events on this thread
 			bool events_written_for_thread = false;
@@ -1207,11 +1201,23 @@ ep_buffer_manager_write_all_buffers_to_file_v4 (
 				buffer_manager_move_next_event_same_thread (buffer_manager, current_timestamp_boundary);
 			}
 
-			ep_thread_session_state_set_last_read_sequence_number (current_thread_session_state, sequence_number);
+			dn_list_it_t found = dn_list_find (buffer_manager->thread_session_state_list, current_thread_session_state);
+			if (!dn_list_it_end (found)) {
+				// If the ThreadSessionState hasn't been removed through buffer_manager_advance_to_non_empty_buffer,
+				// we need to update the last_read_sequence_number for any future sequence points being initialized.
+				ep_thread_session_state_set_last_read_sequence_number (current_thread_session_state, sequence_number);
+			}
+
 			if (sequence_point != NULL) {
-				dn_umap_it_t found = dn_umap_ptr_uint32_find (ep_sequence_point_get_thread_sequence_numbers (sequence_point), current_thread_session_state);
+				dn_umap_it_t found = dn_umap_threadid_uint32_find (ep_sequence_point_get_thread_sequence_numbers (sequence_point), os_thread_id);
 				EP_ASSERT (!dn_umap_it_end (found));
-				dn_umap_ptr_uint32_insert_or_assign (ep_sequence_point_get_thread_sequence_numbers (sequence_point), current_thread_session_state, sequence_number);
+				uint32_t thread_sequence_number = !dn_umap_it_end (found) ? dn_umap_it_value_uint32_t (found) : 0;
+				// Sequence numbers can overflow so we can't use a direct last_read > sequence_number comparison
+				// If a thread is able to drop more than 0x80000000 events in between sequence points then we will
+				// miscategorize it, but that seems unlikely.
+				uint32_t last_read_delta = sequence_number - thread_sequence_number;
+				if (0 < last_read_delta && last_read_delta < 0x80000000)
+					dn_umap_threadid_uint32_insert_or_assign (ep_sequence_point_get_thread_sequence_numbers (sequence_point), os_thread_id, sequence_number);
 			}
 
 			// Have we written events in any sequence point?
@@ -1227,42 +1233,11 @@ ep_buffer_manager_write_all_buffers_to_file_v4 (
 
 		// stopped at sequence point case
 
-		// the sequence point captured a lower bound for sequence number on each thread, but iterating
-		// through the events we may have observed that a higher numbered event was recorded. If so we
-		// should adjust the sequence numbers upwards to ensure the data in the stream is consistent.
-		EP_SPIN_LOCK_ENTER (&buffer_manager->rt_lock, section2)
-			for (dn_list_it_t it = dn_list_begin (buffer_manager->thread_session_state_list); !dn_list_it_end (it); ) {
-				EventPipeThreadSessionState *session_state = *dn_list_it_data_t (it, EventPipeThreadSessionState *);
-				dn_umap_it_t found = dn_umap_ptr_uint32_find (ep_sequence_point_get_thread_sequence_numbers (sequence_point), session_state);
-				uint32_t thread_sequence_number = !dn_umap_it_end (found) ? dn_umap_it_value_uint32_t (found) : 0;
-				uint32_t last_read_sequence_number = ep_thread_session_state_get_last_read_sequence_number (session_state);
-				EP_ASSERT (thread_sequence_number == 0 || thread_sequence_number == last_read_sequence_number);
-				// Sequence numbers can overflow so we can't use a direct last_read > sequence_number comparison
-				// If a thread is able to drop more than 0x80000000 events in between sequence points then we will
-				// miscategorize it, but that seems unlikely.
-				uint32_t last_read_delta = last_read_sequence_number - thread_sequence_number;
-				if (0 < last_read_delta && last_read_delta < 0x80000000) {
-					dn_umap_ptr_uint32_insert_or_assign (ep_sequence_point_get_thread_sequence_numbers (sequence_point), session_state, last_read_sequence_number);
-					if (dn_umap_it_end (found))
-						ep_thread_addref (ep_thread_holder_get_thread (ep_thread_session_state_get_thread_holder_ref (session_state)));
-				}
-
-				it = dn_list_it_next (it);
-
-				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)
-
 		// emit the sequence point into the file
 		ep_file_write_sequence_point (file, sequence_point);
 
 		// we are done with the sequence point
 		EP_SPIN_LOCK_ENTER (&buffer_manager->rt_lock, section3)
-			DN_VECTOR_PTR_FOREACH_BEGIN (EventPipeThreadSessionState *, thread_session_state, &session_states_to_delete) {
-				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;
 			buffer_manager_dequeue_sequence_point (buffer_manager);
 			sequence_point = NULL;
 		EP_SPIN_LOCK_EXIT (&buffer_manager->rt_lock, section3)
@@ -1349,27 +1324,6 @@ 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 (
@@ -1383,7 +1337,6 @@ buffer_manager_remove_and_delete_thread_session_state (
 
 	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);
 }
 

src/native/eventpipe/ep-event-instance.c

@@ -228,14 +228,6 @@ void
 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);
 }
 

src/native/eventpipe/ep-thread.c

@@ -31,7 +31,7 @@ ep_thread_alloc (void)
 	EventPipeThread *instance = ep_rt_object_alloc (EventPipeThread);
 	ep_raise_error_if_nok (instance != NULL);
 
-	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;
@@ -127,12 +127,8 @@ ep_thread_unregister (EventPipeThread *thread)
 		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);
-		}
+		if (ep_buffer_list_is_empty (thread_session_state->buffer_list))
+			ep_buffer_manager_remove_and_delete_thread_session_state (buffer_manager, thread_session_state);
 	}
 
 	bool found = false;
@@ -338,7 +334,6 @@ ep_thread_session_state_alloc (
 	instance->buffer_list = ep_buffer_list_alloc (buffer_manager, thread);
 	ep_raise_error_if_nok (instance->buffer_list != NULL);
 
-	instance->delete_deferred = false;
 	instance->track_state = EP_SEQUENCE_POINT_THREAD_ID_UNTRACKED;
 
 ep_on_exit:

src/native/eventpipe/ep-thread.h

@@ -54,7 +54,7 @@ struct _EventPipeThread_Internal {
 	EventPipeSession *rundown_session;
 	// 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.
@@ -90,7 +90,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(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);
 
@@ -293,15 +293,6 @@ 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;
 	EventPipeThreadSequencePointTrackState track_state;
 };
 
@@ -315,8 +306,6 @@ EP_DEFINE_GETTER_REF(EventPipeThreadSessionState *, thread_session_state, EventP
 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, bool, delete_deferred)
-EP_DEFINE_SETTER(EventPipeThreadSessionState *, thread_session_state, bool, delete_deferred)
 EP_DEFINE_GETTER(EventPipeThreadSessionState *, thread_session_state, EventPipeThreadSequencePointTrackState, track_state)
 EP_DEFINE_SETTER(EventPipeThreadSessionState *, thread_session_state, EventPipeThreadSequencePointTrackState, track_state)
 

src/native/eventpipe/ep-types.h

@@ -411,5 +411,12 @@ ep_system_time_set (
 	uint16_t second,
 	uint16_t milliseconds);
 
+/*
+ * EventPipe-specific container type definitions.
+ */
+
+DN_UMAP_IT_KEY_T (threadid, ep_rt_thread_id_t)
+DN_UMAP_T (threadid, ep_rt_thread_id_t, uint32, uint32_t)
+
 #endif /* ENABLE_PERFTRACING */
 #endif /* __EVENTPIPE_TYPES_H__ */