Grand dispatch queue #661

sbSteveK · 2024-07-29T17:09:50Z

Integration of Apple's grand dispatch queue with event loop

The CI is failing for dispatch queue because of the apple network socket is not in. The socket related changes is in PR: #662

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-commenter · 2024-07-29T19:09:45Z

Codecov Report

Attention: Patch coverage is 63.15789% with 7 lines in your changes missing coverage. Please review.

Project coverage is 79.68%. Comparing base (fdb053b) to head (4fadfee).

Files with missing lines	Patch %	Lines
source/event_loop.c	63.15%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #661      +/-   ##
==========================================
- Coverage   79.69%   79.68%   -0.01%     
==========================================
  Files          30       30              
  Lines        6117     6119       +2     
==========================================
+ Hits         4875     4876       +1     
- Misses       1242     1243       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ueue by default

…_dispatch_queue

This reverts commit aef1b14.

This reverts commit 22e68b2.

…io into grand_dispatch_queue

…h_queue

sbSteveK · 2025-01-22T21:50:03Z

source/darwin/dispatch_queue_event_loop.c

+     * 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;


Trivial: The comment above refers to this lock as the "conetxt lock". Spelling on "context lock" and also we should clarify by either calling it "The dispatch_loop_context's lock" or renaming the lock member here to "context_lock"

source/darwin/dispatch_queue_event_loop.c

sbSteveK · 2025-01-22T22:47:59Z

source/darwin/dispatch_queue_event_loop.c

+            DEFAULT_QUEUE_SIZE,
+            sizeof(struct scheduled_service_entry *),
+            &s_compare_timestamps)) {
+        goto clean_up;


If this fails and we goto clean_up and dispatch_loop_context *context has not yet been assigned to dispatch_loop->context.

This means when s_dispatch_event_loop_destroy(loop) checks whether it should decref context by doing a NULL check on dispatch_loop->context, it fails and leaks memory.

We should assign dispatch_loop->context with this context immediately following its ref_count being initialized so it gets cleaned properly if we goto cleanup here.

sbSteveK · 2025-01-23T17:26:25Z

source/darwin/dispatch_queue_event_loop.c

+
+    s_lock_cross_thread_data(dispatch_loop);
+    if (dispatch_loop->synced_data.suspended) {
+        AWS_LOGF_INFO(AWS_LS_IO_EVENT_LOOP, "id=%p: Starting event-loop thread.", (void *)event_loop);


Since dispatch loops are created in a running state when we retrieve it from Apple's framework, we should add this log to the point where the dispatch loop is started in the aws_event_loop_new_with_dispatch_queue function. This one will never get called on a newly created and run dispatch loop but we want the log that one was created and Started for debugging purposes.

sbSteveK · 2025-01-23T17:27:33Z

source/darwin/dispatch_queue_event_loop.c

+    if (!dispatch_loop->synced_data.suspended) {
+        dispatch_loop->synced_data.suspended = true;
+        AWS_LOGF_INFO(AWS_LS_IO_EVENT_LOOP, "id=%p: Stopping event-loop thread.", (void *)event_loop);
+        /* Suspend will increase the dispatch reference count. It is required to call resume before


Comment clarification is required. Specify that the reference count discussed here is Apple's reference count on the dispatch loop and not ours. Instead of stating we need to call "resume", specify that we either need to call Apple's API dispatch_resume() or that we need to call run() which detects that the dispatch loop is suspended and calls dispatch_resume().

None of this is necessary if we simply remove the ability to suspend the dispatch loop entirely.

sbSteveK · 2025-01-23T17:42:04Z

source/darwin/dispatch_queue_event_loop.c

+    return AWS_OP_SUCCESS;
+}
+
+static int s_stop(struct aws_event_loop *event_loop) {


Dispatch loop shouldn't ever be put into a stop state (dispatch_suspend). If this is called, we should either ignore it, or return an error (platform not supported maybe?) and not mess with any complexity we need to add to support a thing that shouldn't be happening.

s_stop and s_run can be emptied, we will no longer need to check that the dispatch loop is in a non-suspended state before attempting to destroy it, and we can clean up the dispatch_loop by removing the bool suspended from synced_data. Let's only add complexity where it's required.

sbSteveK · 2025-01-23T21:13:24Z

source/darwin/dispatch_queue_event_loop.c

+ *
+ * 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


If we are removing the ability to suspend a dispatch queue, this is no longer a necessary comment.

sbSteveK · 2025-01-23T21:15:21Z

source/darwin/dispatch_queue_event_loop_private.h

+    /* 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


Trivial: "It should be acquired whenever data in the synced_data struct is accessed or modified." ?

sbSteveK · 2025-01-23T21:18:37Z

source/darwin/dispatch_queue_event_loop.c

+ *
+ * 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 the following locks:


Trivial: "This function"

sbSteveK · 2025-01-23T21:28:09Z

source/darwin/dispatch_queue_event_loop.c

+ * 2. Process cross-thread tasks.
+ * 3. Execute all runnable tasks.
+ *
+ * Apple Dispatch queues are FIFO queues to which the application can submit tasks in the form of block objects, and the


We should state that we create a serial dispatch queue and not that Apple Dispatch queues are FIFO. Concurrent Apple dispatch queues can be created so we want to be clear here what we are doing.

sbSteveK · 2025-01-23T22:11:30Z

source/darwin/dispatch_queue_event_loop.c

+ *
+ * Apple Dispatch queues are FIFO queues to which the application can submit tasks in the form of block objects, and the
+ * block objects will be executed on a system defined thread pool. Instead of executing the loop on a single thread, we
+ * tried to recurrently run a single iteration of the execution loop as a dispatch queue block object.


Trivial: remove "tried to". I think we simply are doing it here.

sbSteveK · 2025-01-24T16:02:32Z

source/darwin/dispatch_queue_event_loop.c

+/**
+ * The function should be wrapped around scheduling_status->lock
+ */
+static void s_scheduled_service_entry_destroy(


This is only called as part of cleanup on s_run_iteration. Can we remove this function and add its execution code into s_run_iteration.

If we keep the function, we should pull in the locking and unlocking of the scheduling_status->lock into this function itself to keep the other end cleaner as well as update the comment to explain what this function is doing in the comment description above.

sbSteveK · 2025-01-24T16:07:10Z

source/darwin/dispatch_queue_event_loop.c

+    s_acquire_dispatch_loop_context(dispatch_queue_context);
+    s_rlock_dispatch_loop_context(dispatch_queue_context);
+
+    if (!begin_iteration(entry)) {


The begin_iteration() function is only used in this one spot. It might be cleaner and easier to follow if we move the internals of that check (it looks to be checking whether the entry->dispatch_queue_context->io_dispatch_loop is NULL).

sbSteveK · 2025-01-24T16:22:53Z

include/aws/io/io.h

 struct aws_io_handle {
    union {
        int fd;
+        /* on Apple systems, handle is of type nw_connection_t. On Windows, it's a SOCKET handle. */


Clarity needed. I don't think we're using nw_connection_t unless dispatch queue is turned on.

"On Apple systems using dispatch queue event loops handle is of type nw_connection_t"

sbSteveK · 2025-01-24T16:29:18Z

include/aws/io/private/event_loop_impl.h

+/**
+ * @internal - Don't use outside of testing.
+ *
+ * Return the default event loop type. If the return value is `AWS_ELT_PLATFORM_DEFAULT`, the function failed to


I think you changed this to AWS_EVENT_LOOP_PLATFORM_DEFAULT as AWS_ELT_PLATFORM_DEFAULT doesn't exist.

Also, it looks like there is nothing returned on a failure to retrieve the default type value. I think its a compile time error?

sbSteveK · 2025-01-24T19:19:19Z

source/darwin/dispatch_queue_event_loop.c

+ * */
+static void end_iteration(struct scheduled_service_entry *entry) {
+
+    struct dispatch_loop_context *context = entry->dispatch_queue_context;


Let's rename this variable to be consistent in its name across various functions for clarity. context -> dispatch_queue_context

sbSteveK · 2025-01-24T22:14:58Z

source/darwin/dispatch_queue_event_loop.c

+    s_get_unique_dispatch_queue_id(dispatch_queue_id);
+
+    dispatch_loop->dispatch_queue = dispatch_queue_create(dispatch_queue_id, DISPATCH_QUEUE_SERIAL);
+    if (!dispatch_loop->dispatch_queue) {


dispatch_queue_create Apple API will always return a dispatch_queue_t. If it can't a crash on the OS level is expected on this API call.

sbSteveK added 10 commits July 29, 2024 09:25

CMakeLists.txt

9389f14

event_loop.h

e825924

io.h

452217c

kqueue_event_loop.c

c0d4086

dispatch_queue_event_loop.c

84fd773

event_loop.c

98c8ef0

epoll_event_loop.c

89e8ece

iocp_event_loop.c

9c27144

event_loop_test.c

f375bb2

try ifdef on TargetConditionals.h and comment failing event loop test

41a5fa1

xiazhvera added 19 commits July 30, 2024 13:56

reorder the event loop config, so apple platform would use dispatch q…

04c2b93

…ueue by default

bring in dispatch queue changes

0d301d2

clangformat

4afaea6

remove unused args

a7f6904

clean up

89635db

clean up dispatch queue

195ca1c

Merge branch 'main' of https://github.com/awslabs/aws-c-io into grand…

24ce65b

…_dispatch_queue

clang-format

287094f

more comments and format clean up

bd58da0

quick windows test

f0e5dde

TEST: quick error verification

aef1b14

Revert "TEST: quick error verification"

41bb257

This reverts commit aef1b14.

TEST: using struct instead of union

22e68b2

Revert "TEST: using struct instead of union"

a28cb37

This reverts commit 22e68b2.

add back definition for union

c67e966

WINDOWS: rename function

3ca34ce

fix compile error

f8c26f5

remove unused finalize functions

a428cd8

fix event loop schedule future

5ab8f24

xiazhvera added 5 commits January 7, 2025 13:17

lint and fix warning

f9ff79a

Merge branch 'runtime_select_event_loop' of github.com:awslabs/aws-c-…

98ea949

…io into grand_dispatch_queue

rename private headers

6933da0

cr update

16c36e8

improve comments and lock results

d986649

Base automatically changed from runtime_select_event_loop to main January 8, 2025 17:19

xiazhvera added 6 commits January 8, 2025 09:31

Merge branch 'main' of github.com:awslabs/aws-c-io into grand_dispatc…

54f4883

…h_queue

use aws_min & enable sanitizer in ci

84c6c4c

use priority queue for service entry list

cce6210

clean up priority_queue

7058676

acquire context for iteration

52fe388

clean up comments

c2dab8d

sbSteveK commented Jan 22, 2025

View reviewed changes

source/darwin/dispatch_queue_event_loop.c Show resolved Hide resolved

sbSteveK commented Jan 22, 2025

View reviewed changes

sbSteveK commented Jan 23, 2025

View reviewed changes

fix memory leak, and fix dispatch_queue_id_prefix

4fadfee

sbSteveK commented Jan 23, 2025

View reviewed changes

sbSteveK commented Jan 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Grand dispatch queue #661

Grand dispatch queue #661

sbSteveK commented Jul 29, 2024 •

edited by xiazhvera

Loading

codecov-commenter commented Jul 29, 2024 •

edited

Loading

sbSteveK Jan 22, 2025

sbSteveK Jan 22, 2025 •

edited

Loading

sbSteveK Jan 23, 2025

sbSteveK Jan 23, 2025 •

edited

Loading

sbSteveK Jan 23, 2025

sbSteveK Jan 23, 2025

sbSteveK Jan 23, 2025

sbSteveK Jan 23, 2025

sbSteveK Jan 23, 2025

sbSteveK Jan 23, 2025

sbSteveK Jan 24, 2025

sbSteveK Jan 24, 2025 •

edited

Loading

sbSteveK Jan 24, 2025

sbSteveK Jan 24, 2025

sbSteveK Jan 24, 2025

sbSteveK Jan 24, 2025

Grand dispatch queue #661

Are you sure you want to change the base?

Grand dispatch queue #661

Conversation

sbSteveK commented Jul 29, 2024 • edited by xiazhvera Loading

codecov-commenter commented Jul 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

sbSteveK Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbSteveK Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbSteveK Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbSteveK commented Jul 29, 2024 •

edited by xiazhvera

Loading

codecov-commenter commented Jul 29, 2024 •

edited

Loading

sbSteveK Jan 22, 2025 •

edited

Loading

sbSteveK Jan 23, 2025 •

edited

Loading

sbSteveK Jan 24, 2025 •

edited

Loading