From d9866495bf70979d9cd0ecd54ab655e17cc5d720 Mon Sep 17 00:00:00 2001 From: Vera Xia Date: Tue, 7 Jan 2025 16:39:28 -0800 Subject: [PATCH] improve comments and lock results --- source/darwin/dispatch_queue_event_loop.c | 87 +++++++++++-------- .../dispatch_queue_event_loop_private.h | 4 + 2 files changed, 53 insertions(+), 38 deletions(-) diff --git a/source/darwin/dispatch_queue_event_loop.c b/source/darwin/dispatch_queue_event_loop.c index 4d6d82015..340bda727 100644 --- a/source/darwin/dispatch_queue_event_loop.c +++ b/source/darwin/dispatch_queue_event_loop.c @@ -75,7 +75,7 @@ static struct aws_event_loop_vtable s_vtable = { * Functions ************ * `s_run_iteration`: The function execute on each single iteration * `begin_iteration`: Decide if we should run the iteration - * `end_iteration`: Clean up the related resource and decide if we should schedule next iteration + * `end_iteration`: Clean up the related resource and determine if we should schedule next iteration * */ @@ -98,6 +98,12 @@ struct dispatch_scheduling_state { /* Internal ref-counted dispatch loop context to processing Apple Dispatch Queue Resources */ struct dispatch_loop_context { + /** + * The conetxt lock is a read-write lock used to protect dispatch_loop. + * The write lock will be acquired when we make changes to dispatch_loop. And the read lock will be acquired + * when we need verify if the dispatch_loop is alive. This makes sure that the dispatch_loop will not be destroyed + * from other thread while we are using it. + */ struct aws_rw_lock lock; struct dispatch_loop *io_dispatch_loop; struct dispatch_scheduling_state scheduling_state; @@ -179,16 +185,18 @@ static void s_scheduled_service_entry_destroy(struct scheduled_service_entry *en aws_mem_release(entry->allocator, entry); } -// checks to see if another scheduled iteration already exists that will either -// handle our needs or reschedule at the end to do so -static bool s_should_schedule_iteration( - struct aws_linked_list *scheduled_iterations, - uint64_t proposed_iteration_time) { - if (aws_linked_list_empty(scheduled_iterations)) { +/** + * Helper function to check if another scheduled iteration already exists that will handle our needs + * + * The function should be wrapped with the following locks: + * scheduled_services lock: To safely access the scheduled_services list + */ +static bool s_should_schedule_iteration(struct aws_linked_list *scheduled_services, uint64_t proposed_iteration_time) { + if (aws_linked_list_empty(scheduled_services)) { return true; } - struct aws_linked_list_node *head_node = aws_linked_list_front(scheduled_iterations); + struct aws_linked_list_node *head_node = aws_linked_list_front(scheduled_services); struct scheduled_service_entry *entry = AWS_CONTAINER_OF(head_node, struct scheduled_service_entry, node); // is the next scheduled iteration later than what we require? @@ -231,15 +239,15 @@ static void s_dispatch_event_loop_destroy(void *context) { AWS_LOGF_DEBUG(AWS_LS_IO_EVENT_LOOP, "id=%p: Destroyed Dispatch Queue Event Loop.", (void *)event_loop); } -/** Return a aws_string* with unique dispatch queue id string. The id is In format of - * "com.amazonaws.commonruntime.eventloop."*/ -// static const size_t AWS_IO_APPLE_DISPATCH_QUEUE_ID_PREFIX_LENGTH = 37; static const char AWS_LITERAL_APPLE_DISPATCH_QUEUE_ID_PREFIX[] = "com.amazonaws.commonruntime.eventloop."; static const size_t AWS_IO_APPLE_DISPATCH_QUEUE_ID_PREFIX_LENGTH = AWS_ARRAY_SIZE(AWS_LITERAL_APPLE_DISPATCH_QUEUE_ID_PREFIX); static const size_t AWS_IO_APPLE_DISPATCH_QUEUE_ID_LENGTH = AWS_IO_APPLE_DISPATCH_QUEUE_ID_PREFIX_LENGTH + AWS_UUID_STR_LEN; - +/** + * Generates a unique identifier for a dispatch queue in the format "com.amazonaws.commonruntime.eventloop.". + * This identifier will be stored in the provided `result` buffer. + */ static void s_get_unique_dispatch_queue_id(char result[AWS_IO_APPLE_DISPATCH_QUEUE_ID_LENGTH]) { struct aws_uuid uuid; AWS_FATAL_ASSERT(aws_uuid_init(&uuid) == AWS_OP_SUCCESS); @@ -407,8 +415,12 @@ static int s_stop(struct aws_event_loop *event_loop) { return AWS_OP_SUCCESS; } -// returns true if we should execute an iteration, false otherwise -// The function should be wrapped with dispatch_loop->context.lock +/** + * The function decides if we should run this iteration. + * Returns true if we should execute an iteration, false otherwise + * + * The function should be wrapped with dispatch_loop->context.lock to retain the dispatch loop while running. + */ static bool begin_iteration(struct scheduled_service_entry *entry) { struct dispatch_loop *dispatch_loop = entry->dispatch_queue_context->io_dispatch_loop; @@ -418,8 +430,10 @@ static bool begin_iteration(struct scheduled_service_entry *entry) { return true; } -// conditionally schedule another iteration as needed -// The function should be wrapped with dispatch_loop->context.lock +/** + * Clean up the related resource and determine if we should schedule next iteration. + * The function should be wrapped with dispatch_loop->context.lock to retain the dispatch loop while running. + * */ static void end_iteration(struct scheduled_service_entry *entry) { struct dispatch_loop_context *context = entry->dispatch_queue_context; @@ -430,28 +444,21 @@ static void end_iteration(struct scheduled_service_entry *entry) { // Remove the node before do scheduling so we didnt consider the entry itself aws_linked_list_remove(&entry->node); - // if there are any cross-thread tasks, reschedule an iteration for now + + bool should_schedule = false; + uint64_t should_schedule_at_time = 0; if (!aws_linked_list_empty(&dispatch_loop->synced_data.cross_thread_tasks)) { - // added during service which means nothing was scheduled because will_schedule was true + should_schedule = true; + } + /* we already know there are tasks to be scheduled, we just want the next run time. */ + else if (aws_task_scheduler_has_tasks(&dispatch_loop->scheduler, &should_schedule_at_time)) { + should_schedule = true; + } + + if (should_schedule) { s_lock_service_entries(context); - s_try_schedule_new_iteration(context, 0); + s_try_schedule_new_iteration(context, should_schedule_at_time); s_unlock_service_entries(context); - } else { - // no cross thread tasks, so check internal time-based scheduler - uint64_t next_task_time = 0; - /* we already know it has tasks, we just scheduled one. We just want the next run time. */ - bool has_task = aws_task_scheduler_has_tasks(&dispatch_loop->scheduler, &next_task_time); - - if (has_task) { - // only schedule an iteration if there isn't an existing dispatched iteration for the next task time or - // earlier - s_lock_service_entries(context); - if (s_should_schedule_iteration( - &dispatch_loop->context->scheduling_state.scheduled_services, next_task_time)) { - s_try_schedule_new_iteration(context, next_task_time); - } - s_unlock_service_entries(context); - } } s_unlock_cross_thread_data(dispatch_loop); @@ -511,7 +518,10 @@ static void s_run_iteration(void *context) { * * If timestamp==0, the function will always schedule a new iteration as long as the event loop is not suspended. * - * The function should be wrapped with dispatch_loop->context->lock & dispatch_loop->synced_data.lock + * The function should be wrapped with the following locks: + * dispatch_loop->context->lock: To retain the dispatch loop + * dispatch_loop->synced_data.lock : To verify if the dispatch loop is suspended + * dispatch_loop_context->scheduling_state->services_lock: To modify the scheduled_services list */ static void s_try_schedule_new_iteration(struct dispatch_loop_context *dispatch_loop_context, uint64_t timestamp) { struct dispatch_loop *dispatch_loop = dispatch_loop_context->io_dispatch_loop; @@ -530,12 +540,13 @@ static void s_try_schedule_new_iteration(struct dispatch_loop_context *dispatch_ /** * The Apple dispatch queue uses automatic reference counting (ARC). If an iteration remains in the queue, it will * persist until it is executed. Scheduling a block far into the future can keep the dispatch queue alive - * unnecessarily, even if the app is destroyed. To avoid this, Ensure an iteration is scheduled within a 1-second - * interval to prevent it from remaining in the Apple dispatch queue indefinitely. + * unnecessarily, even if the app has shutdown. To avoid this, Ensure an iteration is scheduled within a + * 1-second interval to prevent it from remaining in the Apple dispatch queue indefinitely. */ delta = MIN(delta, AWS_TIMESTAMP_NANOS); if (delta == 0) { + // dispatch_after_f(0 , ...) is equivclient to dispatch_async_f(...) functionality wise, while // dispatch_after_f(0 , ...) is not as optimal as dispatch_async_f(...) // https://developer.apple.com/documentation/dispatch/1452878-dispatch_after_f dispatch_async_f(dispatch_loop->dispatch_queue, entry, s_run_iteration); diff --git a/source/darwin/dispatch_queue_event_loop_private.h b/source/darwin/dispatch_queue_event_loop_private.h index e7c91332e..394bb7f74 100644 --- a/source/darwin/dispatch_queue_event_loop_private.h +++ b/source/darwin/dispatch_queue_event_loop_private.h @@ -29,6 +29,10 @@ struct dispatch_loop { /* Synced data handle cross thread tasks and events, and event loop operations*/ struct { + /** + * The lock is used to protect synced_data across the threads. It should be acquired whenever we touched the + * data in this synced_data struct. + */ struct aws_mutex lock; /* * `is_executing` flag and `current_thread_id` together are used