Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

increase efficiency of line buffered I/O #2294

Merged
merged 10 commits into from Aug 8, 2019

Conversation

@grondo
Copy link
Contributor

commented Aug 7, 2019

Fixes #2287

This PR includes the addition of flux_buffer_has_lines() and its use in place of flux_buffer_lines() as discussed in #2287.

Additionally, the "multiple callback" support for flux_buffer_t read callbacks was removed -- while it makes sense for standalone use, in the larger context of use by libsubprocess in a reactor, repeatedly calling the buffer callback until no more data was consumed could result in other parts of the system getting starved out by a heavily used buffer.

Since the multiple callback support was removed, I just removed the tests for that aspect of the code. Hope that was an ok approach.

Here's some timing results, before:

Executing: git log --oneline -n1; make -j 12 >/dev/null 2>&1 && time src/cmd/flux start flux srun t/shell/lptest 79 10000 | wc -l
a972f14 libflux/test: remove multi-callback tests for flux_buffer_t
10000

real	0m56.864s
user	0m56.344s
sys	0m3.962s

After:

Executing: git log --oneline -n1; make -j 12 >/dev/null 2>&1 && time src/cmd/flux start flux srun t/shell/lptest 79 10000 | wc -l
177cd4a libflux/test: test flux_buffer_has_line
10000

real	0m4.796s
user	0m2.259s
sys	0m3.057s
grondo added 2 commits Aug 7, 2019
The mode where the flux_buffer_t read callback is called in a loop
until no more data is being read is going away in flux_buffer_t.
Therefore, remove the multiple callback tests which depend on this
behavior.
Problem: When buffer data or a line is pending in a flux_buffer_t
the implementation calls the provided read callback in a loop until
no more data is being consumed. While this makes logical sense for
a standalone buffer implementation, this isn't a good fit with how
the buffer is being used within libflux, since in a reactor driven
context we want to be sure to re-enter the reactor for fairness to
other watchers and events.

Therefore, instead of calling the provided callback in a loop, just
invoke the provided callback once. When using a flux_buffer_t
standalone, the user's callback will have to read in a loop until
all data is consumed, or the buffer will have to be checked for
data out of bound. However, there should not be much change for
use of flux_buffer_t within a reactor context.
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

@chu11 suggested we should also add tests to ensure flux_buffer_read/read_line can be called multiple times from the read callbacks. That's a good idea, so I'll add that to the buffer unit tests.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

Ok, just pushed a test to ensure that flux_buffer_t read callbacks can call read or read_line multiple times before returning.

@garlick garlick requested a review from chu11 Aug 7, 2019
@garlick

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

This all looks good to me but methinks @chu11 should press the button after he's had a look.

@chu11
chu11 approved these changes Aug 8, 2019
Copy link
Contributor

left a comment

LGTM, pretty much exactly what I was imagining implementing when I created the issue :-)

just one comment on the signature of flux_buffer_has_line(). I used an integer return with -1 on error, 1 on true, 0 on false for flux_buffer_is_readonly(). So for consistency, would we want to copy that function's semantic? Or copy this bool return to flux_buffer_is_readonly()?

Looking at how I ultimately used flux_buffer_is_readonly(), it's probably better to have is_readonly() return bool.

This isn't a deal breaker to merging (approving PR). Just dunno if we want to do it here, or in another PR. I can do in a later PR if we want to get this merged in soon.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

just one comment on the signature of flux_buffer_has_line(). I used an integer return with -1 on error, 1 on true, 0 on false for flux_buffer_is_readonly(). So for consistency, would we want to copy that function's semantic? Or copy this bool return to flux_buffer_is_readonly()?

Good comment. Sorry I didn't look at existing usage. I'll just quickly change flux_buffer_is_readonly() if it is ok with you. It just seems like trying to handle errors with these functions is overkill. They are used in conjunction with other calls where the error can be caught.

@chu11

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

I'll just quickly change flux_buffer_is_readonly() if it is ok with you.

Sounds good!

grondo added 7 commits Aug 7, 2019
Problem: There is no way to efficiently determine if a flux_buffer_t
has at least one unread line pending.

Use cbuf_peek_line() to (more) efficiently determine if at least one
line is pending to be read in the buffer.
Problem: Using flux_buffer_lines() to determine if there is at least
one line to read in a flux_buffer_t is a very heavyweight operation,
since the internal cbuf implementation needs to read through the
whole buffer to count lines.

Instead, use flux_buffer_has_line() which at most reads through the
buffer until it finds the first newline.
Problem: flux_buffer_lines() is a very heavyweight operation, and
is used everytime there is activity in a line buffered subprocess
stream to determine if callbacks should be activated.

Use the lighter weight flux_buffer_has_line() instead in the associated
libsubprocess code.
Add sanity check for flux_buffer_has_line() to the libflux/buffer
unit tests.
Add a test for reading from flux_buffer_t in a loop in read
and read_line callbacks.
For simplicity and consistency with flux_buffer_has_line(), change
the return type of flux_buffer_is_readonly() to bool.
@grondo grondo force-pushed the grondo:issue#2287 branch from d5fbe17 to f8b8efc Aug 8, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Ok, I did a quick change to flux_buffer_is_readonly(). It still sets errno on an invalid buffer, so an error condition can be checked by setting errno = 0 before the call and checking the value of errno after the call (though not sure that is needed, it does allow unit testing)

I changed flux_buffer_has_line() to use this same scheme (and squashed since the change was very small)

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Well, unfortunately travis seems to be misbehaving at the moment, will check again in the morning.

@chu11

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

With the update to flux_buffer_is_readonly(), I think an update to subprocess_read() in libsubprocess needs to be updated so the readonly variable is a bool instead of an int. Then there is a caller in flux_subprocess_getline().

I think the other calls to is_readonly are fine as is.

Update internal libsubprocess callers of flux_buffer_is_readonly()
to use bool instead of integer for the return type.
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Thanks @chu11! I made the suggested changes. I was being overcareful about changing the libsubprocess api and didn't even look long enough to realize those users were internal.

Just to verify, it is fine to leave return type of flux_subprocess_read_stream_closed() as int?

@codecov-io

This comment has been minimized.

Copy link

commented Aug 8, 2019

Codecov Report

Merging #2294 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2294      +/-   ##
==========================================
+ Coverage   80.86%   80.87%   +<.01%     
==========================================
  Files         213      213              
  Lines       33558    33553       -5     
==========================================
- Hits        27136    27135       -1     
+ Misses       6422     6418       -4
Impacted Files Coverage Δ
src/common/libsubprocess/remote.c 69.6% <100%> (ø) ⬆️
src/common/libflux/buffer.c 95.43% <100%> (-0.1%) ⬇️
src/common/libsubprocess/subprocess.c 89.2% <100%> (ø) ⬆️
src/common/libflux/ev_buffer_read.c 93.67% <100%> (ø) ⬆️
src/common/libutil/aux.c 90.74% <0%> (-3.71%) ⬇️
src/common/libflux/message.c 80.62% <0%> (-0.38%) ⬇️
src/shell/shell.c 81.02% <0%> (+0.51%) ⬆️
src/modules/connector-local/local.c 74.42% <0%> (+1.15%) ⬆️
@chu11

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

Just to verify, it is fine to leave return type of flux_subprocess_read_stream_closed() as int?

I had debated if we should change that function signature too, but since it's not inconsistent to any other functions in libsubprocess, I figure we could leave it. If you have time and want to update, that'd be fine too. Just LMK if I should hit the button or if you wanna sneak in that one signature change too.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

@chu11

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

I was hesitant to change the public libsubprocess API, I think we can leave it for now.

Sounds good, merging. Thanks!

@chu11 chu11 merged commit 75e38e9 into flux-framework:master Aug 8, 2019
4 checks passed
4 checks passed
Summary 1 potential rule
Details
codecov/patch 100% of diff hit (target 80.86%)
Details
codecov/project 80.87% (+<.01%) compared to b8a49a7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Thanks!

@grondo grondo deleted the grondo:issue#2287 branch Aug 8, 2019
@garlick garlick referenced this pull request Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.