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

split flux_mrpc() out to its own class #1080

Merged
merged 10 commits into from May 25, 2017

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented May 25, 2017

The flux_rpc() interfaces and implementation are encumbered with extra complexity because flux_rpc_multi() doesn't quite fit the design of the interface. flux_rpc_multi() streams multiple results back to the caller, but the "promise/future/continuation" design of flux_rpc() is geared towards a single (though possibly complex) result.

While designing futures (#1053), it seemed obvious that flux_rpc() should return a future once they are available, but futures as a general concept/interface really shouldn't have to support this "streaming mode".

Therefore, to allow flux_rpc() to evolve separately from flux_rpc_multi(), a new class is created, flux_mrpc_t which encompasses the "multi" use case, and otherwise looks quite a bit like flux_rpc_t. The implementation of flux_rpc_t is then simplified.

There is some code duplication, but now there is a separation of concerns, and the heavily used flux_rpc() can be further optimized and changed without this extra baggage.

Users, tests, and man pages are updated to reflect this change.

garlick added some commits May 18, 2017

libflux/mrpc: split flux_mrpc class out of flux_rpc
In preparation for migrating RPC's to futures, split out
the flux_rpc_multi functionality to its own class.
flux_rpc_multi() becomes flux_mrpc(), etc..

flux_mrpc() doesn't quite fit the future abstraction.
Futures provide a callback for one result, while
flux_mrpc() provides multiple, streaming results.
flux_mrpc() could fit if all the results were provided to
the caller in one go, but this would greatly increase
memory utilization usage since all the results would
have to be stored before any could be consumed by the caller.

Although most of the rpc class is copied here, the
decoupling permits flux_rpc (singleton) to change now, and
flux_mrpc, with its comparatively fewer users, to change
independently later.

Subsequent commits will update flux_rpc_multi() users
and drop the flux_rpc_multi() functionality from the
flux_rpc class.
libflux/rpc: drop flux_rpc_multi() et al
Update users of flux_rpc_multi() to use the new
flux_mrpc class, then drop the "multi" specific
functions from the flux_rpc class.
modules/aggregator: avoid using flux_rpc_get_nodeid()
Drop call to flux_rpc_get_nodeid(), which was used in a
flux_log_error message to show who sent the RPC.

I just dropped it from the log message, because flux_log
messages are already stamped with the sending rank,
and in this case, the value returned would have been
FLUX_NODEID_UPSTREAM, which is probably not useful.
libflux/rpc: drop internal dead code
With flux_rpc_get_nodeid() removed, code that caches the
nodeid for singleton rpcs, and that inserts the nodeid
in the group matchtag for group rpcs, can be removed
to simplify the code.

Also, code for conditionally requesting a group matchtag
if more than one response is expected can be removed
as now only zero or one response can ever be expected.
libflux/rpc: simplify rpc state
It's no longer necessary to keep a tally of expected versus
received messages.  Expected is always zero or one
(as determined by flags), and received  is always zero or one
(as determined by whether rx_msg is set NULL or not.

Try clarify some convoluted logic in the continuation
registration.
doc/flux_rpc_raw(3): fix prototype, drop multi refs
Fix outdated flux_rpc_get_raw() function prototype (drop
nodeid paramter).

Drop references to flux_rpc_multi() references.

Reword the description to be a little less like a cut and
paste from flux_rpc(3).
doc/flux_rpc_then(3): drop flux_rpc_multi(3) SEE ALSO
Drop reference to flux_rpc_multi(3) and word the references
to flux_rpc_get() variants more generically since a few
have been added since this man page was written.
doc/flux_mrpc(3): rename class
Since there is no longer overlap between flux_rpc()
and flux_mrpc(), the flux_mrpc(3) man page documents all
the class functions, with new names.

Skip creating stubs for every function, since flux_mrpc()
si not a heavily used class like flux_rpc().

Add and drop words from dictionary.  Rename the example code
and the sharness test that ensures it builds.

@garlick garlick force-pushed the garlick:rpc_multi_split branch from 3f0de49 to ec9d829 May 25, 2017

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 25, 2017

Codecov Report

Merging #1080 into master will decrease coverage by 0.02%.
The diff coverage is 85.57%.

@@            Coverage Diff             @@
##           master    #1080      +/-   ##
==========================================
- Coverage   77.98%   77.95%   -0.03%     
==========================================
  Files         150      151       +1     
  Lines       25963    26109     +146     
==========================================
+ Hits        20246    20353     +107     
- Misses       5717     5756      +39
Impacted Files Coverage Δ
src/modules/aggregator/aggregator.c 76.98% <0%> (-0.2%) ⬇️
src/cmd/flux-module.c 84.45% <100%> (ø) ⬆️
src/common/libflux/rpc.c 91.81% <100%> (-0.19%) ⬇️
src/cmd/builtin/hwloc.c 80.64% <70%> (ø) ⬆️
src/cmd/flux-ping.c 86.86% <75%> (ø) ⬆️
src/common/libflux/mrpc.c 85.31% <85.31%> (ø)
src/broker/module.c 82.15% <0%> (-1.42%) ⬇️
src/common/libflux/request.c 89.04% <0%> (-1.37%) ⬇️
src/broker/content-cache.c 72.54% <0%> (-1.31%) ⬇️
src/common/libutil/dirwalk.c 93.33% <0%> (-0.75%) ⬇️
... and 7 more
@coveralls

This comment has been minimized.

Copy link

coveralls commented May 25, 2017

Coverage Status

Coverage decreased (-0.05%) to 78.175% when pulling 3f0de49 on garlick:rpc_multi_split into fefa109 on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented May 25, 2017

Coverage Status

Coverage decreased (-0.03%) to 78.198% when pulling ec9d829 on garlick:rpc_multi_split into fefa109 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 25, 2017

Dumb question: what is the use case for flux_mrpc() over just using flux_rpc()? Does it use the overlay more efficiently than letting the caller use flux_rpc() on their own? (Sorry I never quite understood this)

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented May 25, 2017

I guess there are two optimizations

  • it uses a group matchtag on the request, thus only one message handler, one dispatcher entry, and one matchtag allocation for the group
  • the amount of state held in a flux_mrpc_t is about the same as one flux_rpc_t.

So if you did say 1000 separate flux_rpc() calls in parallel, you'd have 1000ish times the overhead in those areas.

I'm not necessarily defending flux_mrpc_t as useful or interesting. Probably it needs to get reworked another way someday, for example using event (broadcast) for the request, and reduction for the responses. For now it gets the job done I guess.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 25, 2017

Thanks those are good optimizations, I'd say.

Is this ready for merge?

@grondo grondo merged commit 2bafdad into flux-framework:master May 25, 2017

4 checks passed

codecov/patch 85.57% of diff hit (target 77.98%)
Details
codecov/project Absolute coverage decreased by -0.02% but relative coverage increased by +7.59% compared to fefa109
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.03%) to 78.198%
Details

@grondo grondo referenced this pull request Aug 23, 2017

Closed

0.8.0 Release #1160

@garlick garlick deleted the garlick:rpc_multi_split branch Sep 6, 2017

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.