Skip to content

Commit eca7cb3

Browse files
gdamoregdamore
committed
fixes #1488 aio expiration list performance work needed
There were several problems with the array implementation, both from performance and from correctness. This corrects those errors (hopefully) and restores the expiration lists as linked lists.
1 parent 22f089b commit eca7cb3

File tree

3 files changed

+102
-114
lines changed

3 files changed

+102
-114
lines changed

‎src/core/aio.c

+92-106
Original file line numberDiff line numberDiff line change
@@ -12,14 +12,13 @@
1212
#include <string.h>
1313

1414
struct nni_aio_expire_q {
15-
nni_mtx eq_mtx;
16-
nni_cv eq_cv;
17-
nni_aio **eq_list;
18-
uint32_t eq_len;
19-
uint32_t eq_cap;
20-
nni_aio * eq_aio; // currently expiring (task dispatch)
21-
nni_thr eq_thr;
22-
bool eq_exit;
15+
nni_mtx eq_mtx;
16+
nni_cv eq_cv;
17+
nni_list eq_list;
18+
uint32_t eq_len;
19+
nni_thr eq_thr;
20+
nni_time eq_next; // next expiration
21+
bool eq_exit;
2322
};
2423

2524
static nni_aio_expire_q **nni_aio_expire_q_list;
@@ -117,7 +116,7 @@ void
117116
nni_aio_fini(nni_aio *aio)
118117
{
119118
nni_aio_cancel_fn fn;
120-
void * arg;
119+
void *arg;
121120
nni_aio_expire_q *eq = aio->a_expire_q;
122121

123122
// This is like aio_close, but we don't want to dispatch
@@ -126,10 +125,10 @@ nni_aio_fini(nni_aio *aio)
126125
// We also wait if the aio is being expired.
127126
nni_mtx_lock(&eq->eq_mtx);
128127
aio->a_stop = true;
129-
nni_aio_expire_rm(aio);
130-
while (eq->eq_aio == aio) {
128+
while (aio->a_expiring) {
131129
nni_cv_wait(&eq->eq_cv);
132130
}
131+
nni_aio_expire_rm(aio);
133132
fn = aio->a_cancel_fn;
134133
arg = aio->a_cancel_arg;
135134
aio->a_cancel_fn = NULL;
@@ -203,7 +202,7 @@ nni_aio_stop(nni_aio *aio)
203202
{
204203
if (aio != NULL) {
205204
nni_aio_cancel_fn fn;
206-
void * arg;
205+
void *arg;
207206
nni_aio_expire_q *eq = aio->a_expire_q;
208207

209208
nni_mtx_lock(&eq->eq_mtx);
@@ -228,7 +227,7 @@ nni_aio_close(nni_aio *aio)
228227
{
229228
if (aio != NULL) {
230229
nni_aio_cancel_fn fn;
231-
void * arg;
230+
void *arg;
232231
nni_aio_expire_q *eq = aio->a_expire_q;
233232

234233
nni_mtx_lock(&eq->eq_mtx);
@@ -407,7 +406,7 @@ void
407406
nni_aio_abort(nni_aio *aio, int rv)
408407
{
409408
nni_aio_cancel_fn fn;
410-
void * arg;
409+
void *arg;
411410
nni_aio_expire_q *eq = aio->a_expire_q;
412411

413412
nni_mtx_lock(&eq->eq_mtx);
@@ -508,125 +507,114 @@ static void
508507
nni_aio_expire_add(nni_aio *aio)
509508
{
510509
nni_aio_expire_q *eq = aio->a_expire_q;
511-
if (eq->eq_len >= eq->eq_cap) {
512-
nni_aio **new_list =
513-
nni_zalloc(eq->eq_cap * 2 * sizeof(nni_aio *));
514-
for (uint32_t i = 0; i < eq->eq_len; i++) {
515-
new_list[i] = eq->eq_list[i];
516-
}
517-
nni_free(eq->eq_list, eq->eq_cap * sizeof(nni_aio *));
518-
eq->eq_list = new_list;
519-
eq->eq_cap *= 2;
520-
}
521510

522-
eq->eq_list[eq->eq_len++] = aio;
523-
// Fire the latest aio, but it cames with performance punishment
524-
nni_cv_wake(&eq->eq_cv);
511+
nni_list_append(&eq->eq_list, aio);
512+
513+
if (eq->eq_next > aio->a_expire) {
514+
eq->eq_next = aio->a_expire;
515+
nni_cv_wake(&eq->eq_cv);
516+
}
525517
}
526518

527519
static void
528520
nni_aio_expire_rm(nni_aio *aio)
529521
{
530-
nni_aio_expire_q *eq = aio->a_expire_q;
531-
532-
for (uint32_t i = 0; i < eq->eq_len; i++) {
533-
if (aio == eq->eq_list[i]) {
534-
eq->eq_list[i] = eq->eq_list[eq->eq_len - 1];
535-
eq->eq_len--;
536-
break;
537-
}
538-
}
522+
nni_list_node_remove(&aio->a_expire_node);
539523

540-
if (eq->eq_len < eq->eq_cap / 4 && eq->eq_cap > NNI_EXPIRE_Q_SIZE) {
541-
nni_aio **new_list =
542-
nni_zalloc(eq->eq_cap * sizeof(nni_aio *) / 4);
543-
for (uint32_t i = 0; i < eq->eq_len; i++) {
544-
new_list[i] = eq->eq_list[i];
545-
}
546-
nni_free(eq->eq_list, eq->eq_cap * sizeof(nni_aio *));
547-
eq->eq_list = new_list;
548-
eq->eq_cap /= 4;
549-
}
524+
// If this item is the one that is going to wake the loop,
525+
// don't worry about it. It will wake up normally, or when we
526+
// add a new aio to it. Worst case is just one spurious wake up,
527+
// which we'd need to do anyway.
550528
}
551529

552530
static void
553531
nni_aio_expire_loop(void *arg)
554532
{
555533
nni_aio_expire_q *q = arg;
556-
nni_mtx * mtx = &q->eq_mtx;
557-
nni_cv * cv = &q->eq_cv;
558-
nni_aio ** list;
534+
nni_mtx *mtx = &q->eq_mtx;
535+
nni_cv *cv = &q->eq_cv;
559536
nni_time now;
560-
uint32_t aio_idx;
537+
uint32_t exp_idx;
538+
nni_aio *expires[NNI_EXPIRE_BATCH];
561539

562540
nni_thr_set_name(NULL, "nng:aio:expire");
563541

564-
now = nni_clock();
565542
nni_mtx_lock(mtx);
566543

567544
for (;;) {
568545
nni_aio *aio;
569546
int rv;
570-
571-
if (q->eq_len == 0) {
572-
573-
if (q->eq_exit) {
574-
nni_mtx_unlock(mtx);
575-
return;
576-
}
577-
578-
nni_cv_wait(cv);
579-
580-
now = nni_clock();
547+
nni_time next;
548+
549+
next = q->eq_next;
550+
now = nni_clock();
551+
552+
// Each time we wake up, we scan the entire list of elements.
553+
// We scan forward, moving up to NNI_EXPIRE_Q_SIZE elements
554+
// (a batch) to a saved array of things we are going to cancel.
555+
// This mostly runs in O(n), provided you don't have many
556+
// elements (> NNI_EXPIRE_Q_SIZE) all expiring simultaneously.
557+
aio = nni_list_first(&q->eq_list);
558+
if ((aio == NULL) && (q->eq_exit)) {
559+
nni_mtx_unlock(mtx);
560+
return;
561+
}
562+
if (now < next) {
563+
// Early wake up (just to reschedule), no need to
564+
// rescan the list. This is an optimization.
565+
nni_cv_until(cv, next);
581566
continue;
582567
}
583-
584-
// Find the timer with min expire time.
585-
list = q->eq_list;
586-
aio_idx = 0;
587-
aio = list[aio_idx];
588-
for (uint32_t i = 0; i < q->eq_len; i++) {
589-
if (list[i]->a_expire < aio->a_expire) {
590-
aio = list[i];
591-
aio_idx = i;
568+
q->eq_next = NNI_TIME_NEVER;
569+
exp_idx = 0;
570+
while (aio != NULL) {
571+
if ((aio->a_expire < now) &&
572+
(exp_idx < NNI_EXPIRE_BATCH)) {
573+
nni_aio *nxt;
574+
575+
// This one is expiring.
576+
expires[exp_idx++] = aio;
577+
// save the next node
578+
nxt = nni_list_next(&q->eq_list, aio);
579+
nni_list_remove(&q->eq_list, aio);
580+
// Place a temporary hold on the aio.
581+
// This prevents it from being destroyed.
582+
aio->a_expiring = true;
583+
aio = nxt;
584+
continue;
592585
}
593-
}
594-
if (now < aio->a_expire) {
595-
// Unexpired; we just wait for the next expired aio.
596-
nni_cv_until(cv, aio->a_expire);
597-
now = nni_clock();
598-
continue;
586+
if (aio->a_expire < q->eq_next) {
587+
q->eq_next = aio->a_expire;
588+
}
589+
aio = nni_list_next(&q->eq_list, aio);
599590
}
600591

601-
// The time has come for this aio. Expire it, canceling any
602-
// outstanding I/O.
603-
list[aio_idx] = list[q->eq_len - 1];
604-
q->eq_len--;
605-
rv = aio->a_expire_ok ? 0 : NNG_ETIMEDOUT;
592+
for (uint32_t i = 0; i < exp_idx; i++) {
593+
aio = expires[i];
594+
rv = aio->a_expire_ok ? 0 : NNG_ETIMEDOUT;
606595

607-
nni_aio_cancel_fn cancel_fn = aio->a_cancel_fn;
608-
void * cancel_arg = aio->a_cancel_arg;
596+
nni_aio_cancel_fn cancel_fn = aio->a_cancel_fn;
597+
void *cancel_arg = aio->a_cancel_arg;
609598

610-
aio->a_cancel_fn = NULL;
611-
aio->a_cancel_arg = NULL;
612-
// Place a temporary hold on the aio. This prevents it
613-
// from being destroyed.
614-
q->eq_aio = aio;
599+
aio->a_cancel_fn = NULL;
600+
aio->a_cancel_arg = NULL;
615601

616-
// We let the cancel function handle the completion.
617-
// If there is no cancellation function, then we cannot
618-
// terminate the aio - we've tried, but it has to run
619-
// to it's natural conclusion.
620-
nni_mtx_unlock(mtx);
621-
cancel_fn(aio, cancel_arg, rv);
622-
623-
// Get updated time before reacquiring lock.
624-
now = nni_clock();
625-
626-
nni_mtx_lock(mtx);
627-
628-
q->eq_aio = NULL;
602+
// We let the cancel function handle the completion.
603+
// If there is no cancellation function, then we cannot
604+
// terminate the aio - we've tried, but it has to run
605+
// to its natural conclusion.
606+
if (cancel_fn != NULL) {
607+
nni_mtx_unlock(mtx);
608+
cancel_fn(aio, cancel_arg, rv);
609+
nni_mtx_lock(mtx);
610+
}
611+
aio->a_expiring = false;
612+
}
629613
nni_cv_wake(cv);
614+
615+
if (now < q->eq_next) {
616+
nni_cv_until(cv, q->eq_next);
617+
}
630618
}
631619
}
632620

@@ -756,7 +744,6 @@ nni_aio_expire_q_free(nni_aio_expire_q *eq)
756744
nni_mtx_unlock(&eq->eq_mtx);
757745
}
758746

759-
nni_free(eq->eq_list, eq->eq_cap * sizeof(nni_aio *));
760747
nni_thr_fini(&eq->eq_thr);
761748
nni_cv_fini(&eq->eq_cv);
762749
nni_mtx_fini(&eq->eq_mtx);
@@ -773,9 +760,8 @@ nni_aio_expire_q_alloc(void)
773760
}
774761
nni_mtx_init(&eq->eq_mtx);
775762
nni_cv_init(&eq->eq_cv, &eq->eq_mtx);
776-
eq->eq_cap = NNI_EXPIRE_Q_SIZE;
777-
eq->eq_len = 0;
778-
eq->eq_list = nni_zalloc(eq->eq_cap * sizeof(nni_aio *));
763+
NNI_LIST_INIT(&eq->eq_list, nni_aio, a_expire_node);
764+
eq->eq_next = NNI_TIME_NEVER;
779765
eq->eq_exit = false;
780766

781767
if (nni_thr_init(&eq->eq_thr, nni_aio_expire_loop, eq) != 0) {

‎src/core/aio.h

+6-5
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
//
2-
// Copyright 2020 Staysail Systems, Inc. <[email protected]>
2+
// Copyright 2021 Staysail Systems, Inc. <[email protected]>
33
// Copyright 2018 Capitar IT Group BV <[email protected]>
44
//
55
// This software is supplied under the terms of the MIT License, a
@@ -167,10 +167,10 @@ extern void nni_aio_sys_fini(void);
167167

168168
typedef struct nni_aio_expire_q nni_aio_expire_q;
169169

170-
// An nni_aio is an async I/O handle. The details of this aio structure
170+
// nng_aio is an async I/O handle. The details of this aio structure
171171
// are private to the AIO framework. The structure has the public name
172172
// (nng_aio) so that we minimize the pollution in the public API namespace.
173-
// It is a coding error for anything out side of the AIO framework to access
173+
// It is a coding error for anything outside the AIO framework to access
174174
// any of these members -- the definition is provided here to facilitate
175175
// inlining, but that should be the only use.
176176
struct nng_aio {
@@ -181,6 +181,7 @@ struct nng_aio {
181181
bool a_stop; // Shutting down (no new operations)
182182
bool a_sleep; // Sleeping with no action
183183
bool a_expire_ok; // Expire from sleep is ok
184+
bool a_expiring; // Expiration in progress
184185
nni_task a_task;
185186

186187
// Read/write operations.
@@ -198,9 +199,9 @@ struct nng_aio {
198199

199200
// Provider-use fields.
200201
nni_aio_cancel_fn a_cancel_fn;
201-
void * a_cancel_arg;
202+
void *a_cancel_arg;
202203
nni_list_node a_prov_node; // Linkage on provider list.
203-
void * a_prov_extra[2]; // Extra data used by provider
204+
void *a_prov_extra[2]; // Extra data used by provider
204205

205206
nni_aio_expire_q *a_expire_q;
206207
nni_list_node a_expire_node; // Expiration node

‎src/core/defs.h

+4-3
Original file line numberDiff line numberDiff line change
@@ -165,9 +165,10 @@ typedef nni_type nni_opt_type;
165165
// NNI_MAX_HEADER_SIZE is our header size.
166166
#define NNI_MAX_HEADER_SIZE ((NNI_MAX_MAX_TTL + 1) * sizeof(uint32_t))
167167

168-
// NNI_EXPIRE_Q_SIZE is the default size of aio expire queue
169-
#ifndef NNI_EXPIRE_Q_SIZE
170-
#define NNI_EXPIRE_Q_SIZE 256
168+
// NNI_EXPIRE_BATCH lets us handle expiration in batches,
169+
// reducing the number of traverses of the expiration list we perform.
170+
#ifndef NNI_EXPIRE_BATCH
171+
#define NNI_EXPIRE_BATCH 100
171172
#endif
172173

173174
#endif // CORE_DEFS_H

0 commit comments

Comments
 (0)