From c2097785e752fee94c5c9ef46f03b8312694251a Mon Sep 17 00:00:00 2001 From: Simon Ruderich Date: Fri, 7 Jun 2013 01:27:58 +0200 Subject: [PATCH] Use static list of descriptors to reduce mallocs. Only if file descriptors with a value > 255 occur, the list implementation is used. --- src/coloredstderr.c | 13 ++-- src/constants.h | 4 ++ src/trackfds.h | 145 +++++++++++++++++++++++++++++++---------- tests/example.c | 11 ++++ tests/example.expected | 3 + 5 files changed, 134 insertions(+), 42 deletions(-) diff --git a/src/coloredstderr.c b/src/coloredstderr.c index ccefdf3..008da26 100644 --- a/src/coloredstderr.c +++ b/src/coloredstderr.c @@ -66,7 +66,10 @@ static int check_handle_fd(int fd) { if (!initialized) { init_from_environment(); } - if (tracked_fds_list_count == 0) { + + /* tracked_fds_find() is most likely faster than calling isatty(), + * therefore check if we are tracking this file descriptor first. */ + if (!tracked_fds_find(fd)) { return 0; } @@ -76,7 +79,7 @@ static int check_handle_fd(int fd) { return 0; } - return tracked_fds_find(fd); + return 1; } static void dup_fd(int oldfd, int newfd) { @@ -87,9 +90,6 @@ static void dup_fd(int oldfd, int newfd) { if (!initialized) { init_from_environment(); } - if (tracked_fds_list_count == 0) { - return; - } /* We are already tracking this file descriptor, add newfd to the list as * it will reference the same descriptor. */ @@ -112,9 +112,6 @@ static void close_fd(int fd) { if (!initialized) { init_from_environment(); } - if (tracked_fds_list_count == 0) { - return; - } tracked_fds_remove(fd); } diff --git a/src/constants.h b/src/constants.h index f7c9b77..66fe03f 100644 --- a/src/constants.h +++ b/src/constants.h @@ -30,6 +30,10 @@ #define DEFAULT_PRE_STRING "\e[91m" #define DEFAULT_POST_STRING "\e[0m" +/* Number of elements to allocate statically. Highest descriptor observed in + * normal use was 255 (by bash), which yielded this limit to prevent + * unnecessary calls to malloc() whenever possible. */ +#define TRACKFDS_STATIC_COUNT 256 /* Number of new elements to allocate per realloc(). */ #define TRACKFDS_REALLOC_STEP 10 diff --git a/src/trackfds.h b/src/trackfds.h index 67b9562..4571b2c 100644 --- a/src/trackfds.h +++ b/src/trackfds.h @@ -20,7 +20,11 @@ #ifndef TRACKFDS_H #define TRACKFDS_H 1 -/* List of tracked file descriptors. */ +/* Array of tracked file descriptors. Used for fast lookups for the normally + * used file descriptors (0 <= fd < TRACKFDS_STATIC_COUNT). */ +static int tracked_fds[TRACKFDS_STATIC_COUNT]; + +/* List of tracked file descriptors >= TRACKFDS_STATIC_COUNT. */ static int *tracked_fds_list; /* Current number of items in the list. */ static size_t tracked_fds_list_count; @@ -30,16 +34,39 @@ static size_t tracked_fds_list_space; #ifdef DEBUG static void tracked_fds_debug(void) { - debug(" tracked_fds_list: %d/%d\t\t[%d]\n", tracked_fds_list_count, - tracked_fds_list_space, - getpid()); size_t i; + + for (i = 0; i < TRACKFDS_STATIC_COUNT; i++) { + if (tracked_fds[i]) { + debug(" tracked_fds[%d]: %d\n", i, tracked_fds[i]); + } + } + debug(" tracked_fds_list: %d/%d\t[%d]\n", tracked_fds_list_count, + tracked_fds_list_space, + getpid()); for (i = 0; i < tracked_fds_list_count; i++) { debug(" tracked_fds_list[%d]: %d\n", i, tracked_fds_list[i]); } } #endif +static int init_tracked_fds_list(size_t count) { + /* Reduce reallocs. */ + count += TRACKFDS_REALLOC_STEP; + + tracked_fds_list = malloc(count * sizeof(*tracked_fds_list)); + if (!tracked_fds_list) { +#ifdef DEBUG + warning("malloc(tracked_fds_list, %d) failed [%d]\n", + count * sizeof(*tracked_fds_list), getpid()); +#endif + return 0; + } + + tracked_fds_list_space = count; + return 1; +} + /* Load tracked file descriptors from the environment. The environment is used * to pass the information to child processes. * @@ -77,21 +104,11 @@ static void init_from_environment(void) { count++; } } - tracked_fds_list_space = count + TRACKFDS_REALLOC_STEP; - - tracked_fds_list = malloc(tracked_fds_list_space * sizeof(*tracked_fds_list)); - if (!tracked_fds_list) { -#ifdef DEBUG - warning("malloc(%zu): failed [%d]\n", - tracked_fds_list_space * sizeof(*tracked_fds_list), getpid()); -#endif - return; - } size_t i = 0; /* Parse file descriptor numbers from environment string and store them as - * integers in tracked_fds_list. */ + * integers in tracked_fds and tracked_fds_list. */ char *last; for (x = env_copy, last = env_copy; *x; x++) { if (*x != ',') { @@ -108,43 +125,76 @@ static void init_from_environment(void) { } *x = 0; - tracked_fds_list[i++] = atoi(last); + int fd = atoi(last); + if (fd < TRACKFDS_STATIC_COUNT) { + tracked_fds[fd] = 1; + } else { + if (!tracked_fds_list) { + /* Pessimistic count estimate, but allocating a few more + * elements doesn't hurt. */ + if (!init_tracked_fds_list(count)) { + /* Couldn't allocate memory, skip this entry. */ + warning("foo\n"); + goto next; + } + } + tracked_fds_list[i++] = fd; +#ifdef DEBUG + debug(" large fd: %d\n", fd); +#endif + } + +next: last = x + 1; } - tracked_fds_list_count = count; + tracked_fds_list_count = i; #ifdef DEBUG tracked_fds_debug(); #endif } -static void update_environment_buffer(char *x) { - size_t i; - for (i = 0; i < tracked_fds_list_count; i++) { - int length = snprintf(x, 10 + 1, "%d", tracked_fds_list[i]); - if (length >= 10 + 1) { - /* Integer too big to fit the buffer, skip it. */ +static char *update_environment_buffer_entry(char *x, int fd) { + int length = snprintf(x, 10 + 1, "%d", fd); + if (length >= 10 + 1) { + /* Integer too big to fit the buffer, skip it. */ #ifdef DEBUG - warning("update_environment_buffer_entry(): truncated fd: %d [%d]\n", - tracked_fds_list[i], getpid()); + warning("update_environment_buffer_entry(): truncated fd: %d [%d]\n", + fd, getpid()); #endif - continue; - } + return x; + } - /* Write comma after number. */ - x += length; - *x++ = ','; - /* Make sure the string is always zero terminated. */ - *x = 0; + /* Write comma after number. */ + x += length; + *x++ = ','; + /* Make sure the string is always zero terminated. */ + *x = 0; + + return x; +} +static void update_environment_buffer(char *x) { + size_t i; + for (i = 0; i < TRACKFDS_STATIC_COUNT; i++) { + if (tracked_fds[i]) { + x = update_environment_buffer_entry(x, (int)i); + } + } + for (i = 0; i < tracked_fds_list_count; i++) { + x = update_environment_buffer_entry(x, tracked_fds_list[i]); } } inline static size_t update_environment_buffer_size(void) { - /* An integer (32-bit) has at most 10 digits, + 1 for the comma after each + /* Use the maximum count (TRACKFDS_STATIC_COUNT) of used descriptors + * because it's simple and small enough not to be a problem. + * + * An integer (32-bit) has at most 10 digits, + 1 for the comma after each * number. Bigger file descriptors (which shouldn't occur in reality) are * skipped. */ - return tracked_fds_list_count * (10 + 1) + 1 /* to fit '\0' */; + return (TRACKFDS_STATIC_COUNT + tracked_fds_list_count) + * (10 + 1) + 1 /* to fit '\0' */; } static void update_environment(void) { #ifdef DEBUG @@ -172,6 +222,15 @@ static void update_environment(void) { static void tracked_fds_add(int fd) { + if (fd < TRACKFDS_STATIC_COUNT) { + tracked_fds[fd] = 1; +#if 0 + debug("tracked_fds_add(): %-3d\t\t[%d]\n", fd, getpid()); + tracked_fds_debug(); +#endif + return; + } + if (tracked_fds_list_count >= tracked_fds_list_space) { size_t new_space = tracked_fds_list_space + TRACKFDS_REALLOC_STEP; int *tmp = realloc(tracked_fds_list, @@ -198,6 +257,17 @@ static void tracked_fds_add(int fd) { #endif } static int tracked_fds_remove(int fd) { + if (fd < TRACKFDS_STATIC_COUNT) { + int old_value = tracked_fds[fd]; + tracked_fds[fd] = 0; + +#if 0 + debug("tracked_fds_remove(): %-3d\t[%d]\n", fd, getpid()); + tracked_fds_debug(); +#endif + return old_value; /* Found vs. not found. */ + } + size_t i; for (i = 0; i < tracked_fds_list_count; i++) { if (fd != tracked_fds_list[i]) { @@ -221,6 +291,13 @@ static int tracked_fds_remove(int fd) { return 0; } static int tracked_fds_find(int fd) { + if (fd < TRACKFDS_STATIC_COUNT) { + return tracked_fds[fd]; + } + if (tracked_fds_list_count == 0) { + return 0; + } + size_t i; for (i = 0; i < tracked_fds_list_count; i++) { if (fd == tracked_fds_list[i]) { diff --git a/tests/example.c b/tests/example.c index c615211..b1eac98 100644 --- a/tests/example.c +++ b/tests/example.c @@ -36,6 +36,17 @@ int main(int argc, char **argv) { fprintf(stderr, "\n"); fprintf(stdout, "\n"); + fflush(stdout); + + /* Check usage of tracked_fds_list (at least in parts). No error checking + * here! */ + dup2(STDERR_FILENO, 471); + dup2(471, 42); + write(471, "more on stderr\n", 15); + close(471); + dup2(STDOUT_FILENO, 471); + write(42, "stderr ...\n", 11); + write(471, "more on stdout\n", 15); return EXIT_SUCCESS; } diff --git a/tests/example.expected b/tests/example.expected index e556748..edb9c58 100644 --- a/tests/example.expected +++ b/tests/example.expected @@ -3,3 +3,6 @@ >STDERR>error!: Success STDERR>write to stderr 2STDERR> STDERR>more on stderr +STDERR>stderr ... +