PROTON-2326: epoll proactor refactor - "schedule" instead of "wake", …#290
PROTON-2326: epoll proactor refactor - "schedule" instead of "wake", …#290cliffjansen wants to merge 4 commits intoapache:masterfrom
Conversation
…"task" instead of "context"
| bool working; | ||
| bool on_wake_list; | ||
| bool wake_pending; // unprocessed eventfd wake callback | ||
| struct pcontext_t *wake_next; // wake list, guarded by proactor eventfd_mutex | ||
| bool on_ready_list; | ||
| bool ready; // ready to run and on ready list. Poller notified by eventfd. | ||
| struct task_t *ready_next; // ready list, guarded by proactor eventfd_mutex | ||
| bool closing; | ||
| // Next 4 are protected by the proactor mutex | ||
| struct pcontext_t* next; /* Protected by proactor.mutex */ | ||
| struct pcontext_t* prev; /* Protected by proactor.mutex */ | ||
| struct task_t* next; /* Protected by proactor.mutex */ | ||
| struct task_t* prev; /* Protected by proactor.mutex */ | ||
| int disconnect_ops; /* ops remaining before disconnect complete */ | ||
| bool disconnecting; /* pn_proactor_disconnect */ | ||
| // Protected by schedule mutex | ||
| tslot_t *runner __attribute__((aligned(64))); /* designated or running thread */ | ||
| tslot_t *prev_runner; | ||
| bool sched_wake; | ||
| bool sched_ready; | ||
| bool sched_pending; /* If true, one or more unseen epoll or other events to process() */ | ||
| bool runnable ; /* in need of scheduling */ | ||
| } pcontext_t; | ||
| bool runnable ; /* on one of the runnable lists */ |
There was a problem hiding this comment.
Looking at all the bools here, I wonder if there is actually a hidden state machine that would make the task state more comprehensible and safer. I expect that these bools aren't truly independent and so naming and coding out the actual allowed states might make the lifecycle of tasks easier to understand.
There might be some locking issues here as the bools seem to be covered by different locks, but this seems to be a code smell in itself anyway as I'd expect things covered by a lock to be in the same structure as the lock not elsewhere!!
astitcher
left a comment
astitcher
left a comment
There was a problem hiding this comment.
This basically looks fine - There is no actual logic change here anyway (is there?)
TBH the only way to know if it's an improvement is to get someone with no knowledge of the code to try and understand it!
…ument to notify_poller(), not indirect via task
…eric manner. This closes apache#290
2 participants
Part 1 of epoll proactor code cleanup:
wake->schedule
context->task
wake_notify -> notify_poller
WAKE -> EVENT_FD (epoll_type-t)
wake_pending, wake_list_xxx -> ready, ready_list_xxx
similar changes to use of wake and ctx as local vars
unused sched_wakes_pending removed