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

Flux buffer w/ initial buffer reactors #1518

Merged
merged 2 commits into from May 30, 2018

Conversation

Projects
None yet
5 participants
@chu11
Copy link
Contributor

chu11 commented May 11, 2018

Initial flux buffer + read/write buffer reactor implementation for #1052. Not ready for commiting, but wanted to get initial comments.

@chu11 chu11 requested a review from grondo May 11, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented May 11, 2018

Coverage Status

Coverage increased (+0.2%) to 79.332% when pulling 4f3e00b on chu11:issue1052-flux-buffer-and-buffer-reactor into ba1f064 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 11, 2018

Codecov Report

Merging #1518 into master will increase coverage by 0.18%.
The diff coverage is 91.94%.

@@            Coverage Diff             @@
##           master    #1518      +/-   ##
==========================================
+ Coverage    78.8%   78.99%   +0.18%     
==========================================
  Files         164      167       +3     
  Lines       30333    30718     +385     
==========================================
+ Hits        23905    24266     +361     
- Misses       6428     6452      +24
Impacted Files Coverage Δ
src/common/libflux/reactor.c 91.84% <80.3%> (-1.89%) ⬇️
src/common/libflux/ev_buffer_write.c 89.47% <89.47%> (ø)
src/common/libflux/ev_buffer_read.c 94.93% <94.93%> (ø)
src/common/libflux/buffer.c 95.04% <95.04%> (ø)
src/common/libflux/keepalive.c 86.66% <0%> (-6.67%) ⬇️
src/common/libflux/rpc.c 92.37% <0%> (-1.7%) ⬇️
src/common/libflux/msg_handler.c 87.36% <0%> (-0.37%) ⬇️
... and 4 more
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 11, 2018

Thanks! This is looking pretty good. I will try to make a high-level pass soon and report any comments along the way.

@chu11 chu11 force-pushed the chu11:issue1052-flux-buffer-and-buffer-reactor branch 2 times, most recently from 8c58fc4 to 2a4e668 May 18, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented May 18, 2018

Updated push of my tree, Reactors now handle many corner cases I didn't see at first. Mini fixes to buffer API.

As an experiment (patch is not in this PR), I replaced the pmi zio read/write reactors in wrexecd with these reactors and everything passed.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 18, 2018

Nice work!

@chu11 chu11 force-pushed the chu11:issue1052-flux-buffer-and-buffer-reactor branch from 2a4e668 to cb5cbef May 18, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented May 19, 2018

restarted a builder that hit a "write error", haven't seen one of these in awhile

@garlick

This comment has been minimized.

Copy link
Member

garlick commented May 22, 2018

Other flux watchers were first implemented using composite libev watcher, then wrapped in reactor.[ch].
For example, see ev_flux.[ch], or ev_zlist removed in 5f5468c. Was there a reason not to do that also here? This approach might be equivalent - I need to look more closely to see whether not using the prepare/check/idle pattern to implement this watcher presents any actual problems.

In buffer_read_cb() there are some calls to the internal libev_to_events() function that pass a FLUX_POLLIN as a parameter. That function converts libev flags to flux flags, so I think in this case you just want to pass the flux flags directly to the flux watcher callback.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented May 22, 2018

Other flux watchers were first implemented using composite libev watcher, then wrapped in reactor.[ch].

The main reason was that I was only using a "native" libev io watcher, so I didn't initially see the need to do a composite one.

However, looking at your old ev_zlist, it gave me an alternate viewpoint on how to write this reactor. I could do the prepare/check/idle pattern which would allow me to remove the while loops in the middle of the buffer_read_cb. Let me see if this can be done and/or would be better.

That function converts libev flags to flux flags, so I think in this case you just want to pass the flux flags directly to the flux watcher callback.

Ahh I see what you're saying. Yeah, that can be simplified.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented May 22, 2018

The main reason was that I was only using a "native" libev io watcher, so I didn't initially see the need to do a composite one.

Got it.

It may be beneficial to let the reactor run in between iterations of that loop. For example, maybe another pending event will have a higher priority (like say a ctrl-C to end the program) and spinning in the read handler starves it out.

Ahh I see what you're saying. Yeah, that can be simplified.

I think right now the flux bits are the same as the EV ones but if they were different, that would set the wrong bits.

@chu11 chu11 force-pushed the chu11:issue1052-flux-buffer-and-buffer-reactor branch from cb5cbef to 3f1e207 May 23, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented May 23, 2018

re-pushed with a new implementation. The read reactors are now in a composite prepare/check/idle file ev_buffer_read. The write reactor is simple enough it doesn't need prepare/check/idle, but I placed it in a ev_buffer_write file for consistency.

A few minor fixes/tweaks along the way and some additional tests.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented May 23, 2018

Just a thought, but could the read_line variant of the watcher be eliminated if a flags argument were added to the regular read watcher, e.g. to select line buffered?

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented May 23, 2018

yeah, it definitely could. I went back and forth on whether I should do that. I honestly can't remember why I settled on it the way I did, but could go either way.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 24, 2018

Now that @garlick mentions it, it might be better for extensibility+simplicity if we use a flags argument to indicate if the buffering is normal, line, or fully unbuffered. The flags field could additionally be used for other tunables in the future.

@chu11 chu11 force-pushed the chu11:issue1052-flux-buffer-and-buffer-reactor branch from 3f1e207 to 841e003 May 24, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented May 24, 2018

Sounds good. I just pushed a change that collapses the read watcher & read line watcher into one with a new flags argument. I also added a flags argument to the write watcher for future support. I went ahead and just squashed the changes b/c its pretty tiny.

chu11 added some commits Apr 20, 2018

common/libflux: Add flux_buffer data structure
Add basic buffer data structure.  This data structure is mostly
a wrapper around liblsd/cbuf, providing a public flux buffer data
structure for use for buffer related APIs in the future.

Privately, support some callback functions to be used by flux
internally.
common/libflux: Add read/write buffer watchers
Add new read/write watchers that are tied to a flux buffer.

@garlick garlick force-pushed the chu11:issue1052-flux-buffer-and-buffer-reactor branch from 841e003 to 4f3e00b May 30, 2018

@garlick

This comment has been minimized.

Copy link
Member

garlick commented May 30, 2018

Rebased on current master and pushed to @chu11's branch.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented May 30, 2018

@grondo and I thought we'd go ahead and merge this as is, and if there are any additional issues, we can address them in separate pull requests. Nice work!

@garlick garlick merged commit bf59bee into flux-framework:master May 30, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 79.332%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.