Skip to content

Commit

Permalink
libsubprocess: create write_buffer on demand
Browse files Browse the repository at this point in the history
Problem: Profiling shows that creating the write buffer for
remote subprocesses eats up a healthy amount of cycles.  However,
the buffer is not needed for many circumstances.  It is only needed
when there is an attemp to write data to the subprocess before
the subprocess is running.

Create the write buffer only when it is needed.
  • Loading branch information
chu11 committed May 28, 2024
1 parent 175e741 commit 1bf761c
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 13 deletions.
21 changes: 8 additions & 13 deletions src/common/libsubprocess/remote.c
Original file line number Diff line number Diff line change
Expand Up @@ -226,25 +226,12 @@ static int remote_channel_setup (flux_subprocess_t *p,
struct subprocess_channel *c = NULL;
char *e = NULL;
int save_errno;
int buffer_size;

if (!(c = channel_create (p, output_cb, name, channel_flags))) {
llog_debug (p, "channel_create: %s", strerror (errno));
goto error;
}

if ((buffer_size = cmd_option_bufsize (p, name)) < 0) {
llog_debug (p, "cmd_option_bufsize: %s", strerror (errno));
goto error;
}

if (channel_flags & CHANNEL_WRITE) {
if (!(c->write_buffer = fbuf_create (buffer_size))) {
llog_debug (p, "fbuf_create: %s", strerror (errno));
goto error;
}
}

if (channel_flags & CHANNEL_READ) {
int wflag;

Expand All @@ -257,6 +244,11 @@ static int remote_channel_setup (flux_subprocess_t *p,
c->line_buffered = true;

if (!(p->flags & FLUX_SUBPROCESS_FLAGS_LOCAL_UNBUF)) {
int buffer_size;
if ((buffer_size = cmd_option_bufsize (p, name)) < 0) {
llog_debug (p, "cmd_option_bufsize: %s", strerror (errno));
goto error;

Check warning on line 250 in src/common/libsubprocess/remote.c

View check run for this annotation

Codecov / codecov/patch

src/common/libsubprocess/remote.c#L249-L250

Added lines #L249 - L250 were not covered by tests
}
if (!(c->read_buffer = fbuf_create (buffer_size))) {
llog_debug (p, "fbuf_create: %s", strerror (errno));
goto error;

Check warning on line 254 in src/common/libsubprocess/remote.c

View check run for this annotation

Codecov / codecov/patch

src/common/libsubprocess/remote.c#L253-L254

Added lines #L253 - L254 were not covered by tests
Expand Down Expand Up @@ -525,6 +517,9 @@ static int send_channel_data (flux_subprocess_t *p)
set_failed (p, "internal close error");
return -1;

Check warning on line 518 in src/common/libsubprocess/remote.c

View check run for this annotation

Codecov / codecov/patch

src/common/libsubprocess/remote.c#L517-L518

Added lines #L517 - L518 were not covered by tests
}
/* Don't need this anymore, reclaim the space */
fbuf_destroy (c->write_buffer);
c->write_buffer = NULL;
}
if (c->closed) {
if (subprocess_write (p->h,
Expand Down
11 changes: 11 additions & 0 deletions src/common/libsubprocess/subprocess.c
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,17 @@ int flux_subprocess_write (flux_subprocess_t *p,
return -1;
}
if (p->state == FLUX_SUBPROCESS_INIT) {
if (!c->write_buffer) {
int buffer_size;
if ((buffer_size = cmd_option_bufsize (p, stream)) < 0) {
log_err ("cmd_option_bufsize: %s", strerror (errno));
return -1;

Check warning on line 715 in src/common/libsubprocess/subprocess.c

View check run for this annotation

Codecov / codecov/patch

src/common/libsubprocess/subprocess.c#L714-L715

Added lines #L714 - L715 were not covered by tests
}
if (!(c->write_buffer = fbuf_create (buffer_size))) {
log_err ("fbuf_create");
return -1;

Check warning on line 719 in src/common/libsubprocess/subprocess.c

View check run for this annotation

Codecov / codecov/patch

src/common/libsubprocess/subprocess.c#L718-L719

Added lines #L718 - L719 were not covered by tests
}
}
if (fbuf_space (c->write_buffer) < len) {
errno = ENOSPC;
return -1;

Check warning on line 724 in src/common/libsubprocess/subprocess.c

View check run for this annotation

Codecov / codecov/patch

src/common/libsubprocess/subprocess.c#L723-L724

Added lines #L723 - L724 were not covered by tests
Expand Down

0 comments on commit 1bf761c

Please sign in to comment.