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; }