From 1df9d85f28e41e1396939a3827aa5b0a8c8f656c Mon Sep 17 00:00:00 2001 From: nick black Date: Thu, 18 Feb 2021 01:37:30 -0500 Subject: [PATCH] Lock accesses to notcurses_stats #1139 notcurses_stats() and notcurses_stats_reset() now take the new statlock member, as do stat modifications from render, raster, writeout, resize, plane creation, and plane destruction. Add nonnull attributes to stats API. Initialize and destroy statlock as part of notcurses struct. Update documentation. Free pilelock on error paths. Closes #1139. --- NEWS.md | 3 +++ USAGE.md | 6 +++--- include/notcurses/notcurses.h | 12 ++++++++---- src/lib/internal.h | 1 + src/lib/notcurses.c | 34 +++++++++++++++++++++++++++------- src/lib/render.c | 14 +++++++++++--- 6 files changed, 53 insertions(+), 17 deletions(-) diff --git a/NEWS.md b/NEWS.md index 61e1ca659..be330de2f 100644 --- a/NEWS.md +++ b/NEWS.md @@ -5,6 +5,9 @@ rearrangements of Notcurses. * `ncplane_qrcode()` no longer accepts a blitter argument, since `NCBLIT_2x1` is the only one that actually works with qr code scanners. I'm unaware of any external `ncplane_qrcode()` users, so hopefully this isn't a problem. + * `notcurses_stats()` no longer qualifies its `notcurses*` argument with + `const`, since it now takes a lock. I'm sorry about that, though on the + plus side, data races can no longer result in invalid stats. * 2.2.1 (2021-02-09): * Brown-bag release: fix UTF8 discovery in direct mode. Sorry! diff --git a/USAGE.md b/USAGE.md index fe917035c..0340b0417 100644 --- a/USAGE.md +++ b/USAGE.md @@ -3062,13 +3062,13 @@ typedef struct ncstats { ncstats* notcurses_stats_alloc(const struct notcurses* nc); // Acquire an atomic snapshot of the Notcurses object's stats. -void notcurses_stats(const struct notcurses* nc, ncstats* stats); +void notcurses_stats(struct notcurses* nc, ncstats* stats); -// Reset all cumulative stats (immediate ones, such as fbbytes, are not reset). +// Reset all cumulative stats (immediate ones, such as fbbytes, are not reset), +// first copying them into |*stats| (if |stats| is not NULL). void notcurses_stats_reset(struct notcurses* nc, ncstats* stats); ``` - ## C++ Marek Habersack has contributed (and maintains) C++ wrappers installed to diff --git a/include/notcurses/notcurses.h b/include/notcurses/notcurses.h index 41821e404..757af9ffe 100644 --- a/include/notcurses/notcurses.h +++ b/include/notcurses/notcurses.h @@ -1276,13 +1276,17 @@ typedef struct ncstats { // Allocate an ncstats object. Use this rather than allocating your own, since // future versions of Notcurses might enlarge this structure. -API ALLOC ncstats* notcurses_stats_alloc(const struct notcurses* nc); +API ALLOC ncstats* notcurses_stats_alloc(const struct notcurses* nc) + __attribute__ ((nonnull(1))); // Acquire an atomic snapshot of the Notcurses object's stats. -API void notcurses_stats(const struct notcurses* nc, ncstats* stats); +API void notcurses_stats(struct notcurses* nc, ncstats* stats) + __attribute__ ((nonnull(1, 2))); -// Reset all cumulative stats (immediate ones, such as fbbytes, are not reset). -API void notcurses_stats_reset(struct notcurses* nc, ncstats* stats); +// Reset all cumulative stats (immediate ones, such as fbbytes, are not reset), +// first copying them into |*stats| (if |stats| is not NULL). +API void notcurses_stats_reset(struct notcurses* nc, ncstats* stats) + __attribute__ ((nonnull(1))); // Resize the specified ncplane. The four parameters 'keepy', 'keepx', // 'keepleny', and 'keeplenx' define a subset of the ncplane to keep, diff --git a/src/lib/internal.h b/src/lib/internal.h index 2257a2f5e..492a0024a 100644 --- a/src/lib/internal.h +++ b/src/lib/internal.h @@ -348,6 +348,7 @@ typedef struct notcurses { int cursory; // desired cursor placement according to user. int cursorx; // -1 is don't-care, otherwise moved here after each render. + pthread_mutex_t statlock; ncstats stats; // some statistics across the lifetime of the notcurses ctx FILE* ttyfp; // FILE* for writing rasterized data diff --git a/src/lib/notcurses.c b/src/lib/notcurses.c index c822c6a80..9d19194cb 100644 --- a/src/lib/notcurses.c +++ b/src/lib/notcurses.c @@ -221,10 +221,12 @@ void free_plane(ncplane* p){ if(p){ // ncdirect fakes an ncplane with no ->pile if(ncplane_pile(p)){ + notcurses* nc = ncplane_notcurses(p); + pthread_mutex_lock(&nc->statlock); --ncplane_notcurses(p)->stats.planes; ncplane_notcurses(p)->stats.fbbytes -= sizeof(*p->fb) * p->leny * p->lenx; + pthread_mutex_unlock(&nc->statlock); if(p->above == NULL && p->below == NULL){ - struct notcurses* nc = ncplane_notcurses(p); pthread_mutex_lock(&nc->pilelock); ncpile_destroy(ncplane_pile(p)); pthread_mutex_unlock(&nc->pilelock); @@ -357,8 +359,10 @@ ncplane* ncplane_new_internal(notcurses* nc, ncplane* n, }else{ // new pile make_ncpile(nc, p); } + pthread_mutex_lock(&nc->statlock); nc->stats.fbbytes += fbsize; ++nc->stats.planes; + pthread_mutex_unlock(&nc->statlock); pthread_mutex_unlock(&nc->pilelock); } loginfo(nc, "Created new %dx%d plane \"%s\" @ %dx%d\n", @@ -541,6 +545,7 @@ int ncplane_resize_internal(ncplane* n, int keepy, int keepx, int keepleny, return -1; } loginfo(ncplane_notcurses(n), "%dx%d @ %d/%d → %d/%d @ %d/%d (keeping %dx%d from %d/%d)\n", rows, cols, n->absy, n->absx, ylen, xlen, n->absy + keepy + yoff, n->absx + keepx + xoff, keepleny, keeplenx, keepy, keepx); + notcurses* nc = ncplane_notcurses(n); // we're good to resize. we'll need alloc up a new framebuffer, and copy in // those elements we're retaining, zeroing out the rest. alternatively, if // we've shrunk, we will be filling the new structure. @@ -559,8 +564,10 @@ int ncplane_resize_internal(ncplane* n, int keepy, int keepx, int keepleny, n->x = xlen - 1; } nccell* preserved = n->fb; + pthread_mutex_lock(&nc->statlock); ncplane_notcurses(n)->stats.fbbytes -= sizeof(*preserved) * (rows * cols); ncplane_notcurses(n)->stats.fbbytes += fbsize; + pthread_mutex_unlock(&nc->statlock); n->fb = fb; const int oldabsy = n->absy; // go ahead and move. we can no longer fail at this point. but don't yet @@ -712,8 +719,10 @@ reset_stats(ncstats* stats){ stats->planes = planes; } -void notcurses_stats(const notcurses* nc, ncstats* stats){ +void notcurses_stats(notcurses* nc, ncstats* stats){ + pthread_mutex_lock(&nc->statlock); memcpy(stats, &nc->stats, sizeof(*stats)); + pthread_mutex_unlock(&nc->statlock); } ncstats* notcurses_stats_alloc(const notcurses* nc __attribute__ ((unused))){ @@ -721,10 +730,12 @@ ncstats* notcurses_stats_alloc(const notcurses* nc __attribute__ ((unused))){ } void notcurses_stats_reset(notcurses* nc, ncstats* stats){ + pthread_mutex_lock(&nc->statlock); if(stats){ memcpy(stats, &nc->stats, sizeof(*stats)); } reset_stats(&nc->stats); + pthread_mutex_unlock(&nc->statlock); } // unless the suppress_banner flag was set, print some version information and @@ -879,6 +890,7 @@ recursive_lock_init(pthread_mutex_t *lock){ #endif } +// FIXME cut this up into a few distinct pieces, yearrrgh notcurses* notcurses_core_init(const notcurses_options* opts, FILE* outfp){ notcurses_options defaultopts; memset(&defaultopts, 0, sizeof(defaultopts)); @@ -915,11 +927,6 @@ notcurses* notcurses_core_init(const notcurses_options* opts, FILE* outfp){ if(outfp == NULL){ outfp = stdout; } - if(recursive_lock_init(&ret->pilelock)){ - fprintf(stderr, "Couldn't initialize pile mutex\n"); - free(ret); - return NULL; - } ret->margin_t = opts->margin_t; ret->margin_b = opts->margin_b; ret->margin_l = opts->margin_l; @@ -956,6 +963,16 @@ notcurses* notcurses_core_init(const notcurses_options* opts, FILE* outfp){ }else{ fprintf(stderr, "Defaulting to %dx%d (output is not to a terminal)\n", DEFAULT_ROWS, DEFAULT_COLS); } + if(recursive_lock_init(&ret->pilelock)){ + fprintf(stderr, "Couldn't initialize pile mutex\n"); + free(ret); + return NULL; + } + if(pthread_mutex_init(&ret->statlock, NULL)){ + pthread_mutex_destroy(&ret->pilelock); + free(ret); + return NULL; + } if(setup_signals(ret, (opts->flags & NCOPTION_NO_QUIT_SIGHANDLERS), (opts->flags & NCOPTION_NO_WINCH_SIGHANDLER), notcurses_stop_minimal)){ @@ -1046,6 +1063,8 @@ err: // FIXME looks like we have some memory leaks on this error path? tcsetattr(ret->ttyfd, TCSANOW, &ret->tpreserved); drop_signals(ret); + pthread_mutex_destroy(&ret->statlock); + pthread_mutex_destroy(&ret->pilelock); free(ret); return NULL; } @@ -1172,6 +1191,7 @@ int notcurses_stop(notcurses* nc){ } } del_curterm(cur_term); + ret |= pthread_mutex_destroy(&nc->statlock); ret |= pthread_mutex_destroy(&nc->pilelock); free(nc); } diff --git a/src/lib/render.c b/src/lib/render.c index 75d26adad..d8bf9faf0 100644 --- a/src/lib/render.c +++ b/src/lib/render.c @@ -97,7 +97,7 @@ blocking_write(int fd, const char* buf, size_t buflen){ return 0; } -// update timings for writeout. only call on success. +// update timings for writeout. only call on success. call only under statlock. static void update_write_stats(const struct timespec* time1, const struct timespec* time0, ncstats* stats, int bytes){ @@ -118,7 +118,7 @@ update_write_stats(const struct timespec* time1, const struct timespec* time0, } } -// negative 'bytes' is recorded as a failure. +// negative 'bytes' is recorded as a failure. call only while holding statlock. static void update_render_bytes(ncstats* stats, int bytes){ if(bytes >= 0){ @@ -134,6 +134,7 @@ update_render_bytes(ncstats* stats, int bytes){ } } +// call only while holding statlock. static void update_render_stats(const struct timespec* time1, const struct timespec* time0, ncstats* stats){ @@ -152,6 +153,7 @@ update_render_stats(const struct timespec* time1, const struct timespec* time0, } } +// call only while holding statlock. static void update_raster_stats(const struct timespec* time1, const struct timespec* time0, ncstats* stats){ @@ -1200,10 +1202,12 @@ int ncpile_rasterize(ncplane* n){ clock_gettime(CLOCK_MONOTONIC, &rasterdone); int bytes = notcurses_rasterize(nc, pile, nc->rstate.mstreamfp); // accepts -1 as an indication of failure - update_render_bytes(&nc->stats, bytes); clock_gettime(CLOCK_MONOTONIC, &writedone); + pthread_mutex_lock(&nc->statlock); + update_render_bytes(&nc->stats, bytes); update_raster_stats(&rasterdone, &start, &nc->stats); update_write_stats(&writedone, &rasterdone, &nc->stats, bytes); + pthread_mutex_unlock(&nc->statlock); if(bytes < 0){ return -1; } @@ -1246,7 +1250,9 @@ int ncpile_render(ncplane* n){ notcurses_stdplane(nc)->absy, notcurses_stdplane(nc)->absx); clock_gettime(CLOCK_MONOTONIC, &renderdone); + pthread_mutex_lock(&nc->statlock); update_render_stats(&renderdone, &start, &nc->stats); + pthread_mutex_unlock(&nc->statlock); return 0; } @@ -1267,7 +1273,9 @@ int notcurses_render_to_buffer(notcurses* nc, char** buf, size_t* buflen){ return -1; } int bytes = notcurses_rasterize_inner(nc, ncplane_pile(stdn), nc->rstate.mstreamfp); + pthread_mutex_lock(&nc->statlock); update_render_bytes(&nc->stats, bytes); + pthread_mutex_unlock(&nc->statlock); if(bytes < 0){ return -1; }