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.
This commit is contained in:
nick black 2021-02-18 01:37:30 -05:00
parent 9be2f138e3
commit 1df9d85f28
No known key found for this signature in database
GPG Key ID: 5F43400C21CBFACC
6 changed files with 53 additions and 17 deletions

View File

@ -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!

View File

@ -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

View File

@ -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,

View File

@ -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

View File

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

View File

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