[GitHub] [qpid-proton] astitcher commented on a change in pull request #275: PROTON-1496: epoll proactor - single timer queue and timerfd for all …

[GitBox <gi...@apache.org>]

astitcher commented on a change in pull request #275:
URL: https://github.com/apache/qpid-proton/pull/275#discussion_r508676592



##########
File path: c/include/proton/cid.h
##########
@@ -68,7 +68,8 @@ typedef enum {
   CID_pn_proactor,
 
   CID_pn_listener_socket,
-  CID_pn_raw_connection
+  CID_pn_raw_connection,
+  CID_pn_tick_timer

Review comment:
       Is this actually needed? As in will the code compile without this enum definition? It should only be needed for application visible types. For example, if you need to put a pn_tick_timer into an event sent to the application. But this is not happening here.

##########
File path: c/src/proactor/epoll-internal.h
##########
@@ -54,11 +54,13 @@ extern "C" {
 typedef struct acceptor_t acceptor_t;
 typedef struct tslot_t tslot_t;
 typedef pthread_mutex_t pmutex;
+typedef struct pconnection_t pconnection_t;
+typedef struct pn_tick_timer_t pn_tick_timer_t;
 
 typedef enum {
   WAKE,   /* see if any work to do in proactor/psocket context */
   PCONNECTION_IO,
-  PCONNECTION_TIMER,
+  CTIMERQ_TIMER,

Review comment:
       Please don't use this obtuse name: Is it TICK_TIMER? TIMER_QUEUE? CONNECTION_TIMER_QUEUE_TIMER??? That last seems to be the expansion of the name, but isn't very comprehensible besides being redundant!

##########
File path: c/src/proactor/epoll-internal.h
##########
@@ -77,15 +79,14 @@ typedef struct ptimer_t {
   pmutex mutex;
   epoll_extended_t epoll_io;
   bool timer_active;
-  bool in_doubt;  // 0 or 1 callbacks are possible
-  bool shutting_down;
 } ptimer_t;
 
 typedef enum {
   PROACTOR,
   PCONNECTION,
   LISTENER,
-  RAW_CONNECTION
+  RAW_CONNECTION,
+  CONN_TIMERQ

Review comment:
       As above, bad naming - I think here just TIMER would be good enough, if this name is no good because it clashes then PTIMER.

##########
File path: c/src/proactor/epoll-internal.h
##########
@@ -360,6 +384,13 @@ pcontext_t *pni_raw_connection_context(praw_connection_t *rc);
 praw_connection_t *pni_batch_raw_connection(pn_event_batch_t* batch);
 void pni_raw_connection_done(praw_connection_t *rc);
 
+bool ctimerq_init(pn_proactor_t *p);
+void ctimerq_finalize(pn_proactor_t *p);
+bool ctimerq_register(connection_timerq_t *ctq, pconnection_t *pc);
+void ctimerq_deregister(connection_timerq_t *ctq, pconnection_t *pc);
+void ctimerq_schedule_tick(connection_timerq_t *ctq, pconnection_t *pc, uint64_t deadline, uint64_t now);
+pn_event_batch_t *ctimerq_process(connection_timerq_t *ctq, bool timeout);
+

Review comment:
       These are misnamed all internal API has pni_ prefix
   
   Also ctimerq_t is not the struct name so they are misnamed that that way too. 
   If connection_timerq_t is too long in one context it is in all contexts! I don't see the relevance of the connection_ bit. Why not just timer_queue_t? Why is it specific to connections? Shouldn't it also handle the proactor timers too?

##########
File path: c/src/proactor/epoll-internal.h
##########
@@ -347,6 +366,11 @@ bool start_polling(epoll_extended_t *ee, int epollfd);
 void stop_polling(epoll_extended_t *ee, int epollfd);
 void rearm_polling(epoll_extended_t *ee, int epollfd);
 
+bool ptimer_init(ptimer_t *pt, epoll_type_t ep_type);
+void ptimer_finalize(ptimer_t *pt);
+void ptimer_set(ptimer_t *pt, uint64_t t_millis);
+bool ptimer_callback(ptimer_t *pt);

Review comment:
       These are now misnamed all internal API has the pni_ prefix
   
   And also seeing as they are now API external to epoll.c why not put them in their own file?

##########
File path: c/src/proactor/epoll-internal.h
##########
@@ -217,16 +227,15 @@ typedef struct psocket_t {
   uint32_t working_io_events;
 } psocket_t;
 
-typedef struct pconnection_t {
+struct pconnection_t {
   psocket_t psocket;
   pcontext_t context;
-  ptimer_t timer;  // TODO: review one timerfd per connection
   const char *host, *port;
+  pn_tick_timer_t *timer;     // Idle timeout timer. Protected by and sole use by ctimerq.

Review comment:
       Comment incorrect: I assume this means really part of context.proactor.ctimerq




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org