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

adding node::Queue; refactoring SinkChannel for code reuse #1

Merged
5 commits merged into from
Jun 17, 2022

Conversation

ryanolson
Copy link
Contributor

In our original design, we had three entities required to form an edge: a Source, Channel and Sink.

This required constructing three object and making two calls to form an edge, pseudo code

auto source = s.make_source<T>(...);
auto channel = s.make_channel<T>(...);
auto sink = s.make_sink<T>(...);

s.make_edge(source, channel);
s.make_edge(channel, sink);

To reduce complexity for common patterns, it was decided that Sinks would own Channels and provide them to Sources. This had the effect of reducing complexity for edge construction to the creation of a source and a sink and the formation of an edge.

auto source = s.make_source<T>(...);
auto sink = s.make_sink<T>(...);

s.make_edge(source, sink);

While this reduction in complexity is nice, it also creates a limitation in that each Sink owns its own Channel. This makes load-balancing work across multiple downstream Sinks more difficult, because work is being pushed to each Sink. Pushing has several problems:

  • Any write to a Sink's channel which is full, fiber yields the writer.
    • This means if we have N Sinks to which we want to write, we need to have a unique writer per Sink or risk stalling a single writer by blocking on write.
  • Sinks operating at different speeds, either from heterogeneous hardware or by the fact the operation being performed may have variable length wall time based on inputs, means push based load-balancing will be problematic.

This MR adds the ability for pull based Sinks using a design similar to the original allowing for standalone Queue (Channel holder), but with the default being the current behavior.

// Let:
// - SourceProperties<T> be an IngressAcceptor
// - SinkProperties<T> be an IngressProvider

// If:
// - SourceChannel<T> is an IngressAcceptor
// - SinkChannel<T> can be either an IngressProvider or a ChannelAcceptor
// - Queue<T> is an IngressProivder and a ChannelProvider

// Then, Edges can be from:
// - make_edge(IngressAcceptor<T>, IngressProivder<T>)
// - make_edge(ChannelProvider<T>, ChannelAcceptor<T>)

This enabled the following example's Sink to be a pull based Sink:

        auto source = s.make_object("source", test::nodes::infinite_int_rx_source());
        auto queue  = s.make_object("queue", std::make_unique<node::Queue<int>>());
        auto sink   = s.make_object("sink", test::nodes::int_sink());
        s.make_edge(source, queue);
        s.make_edge(queue, sink);

There is no change to the API or default behavior of node/segment objects.

@ryanolson ryanolson added improvement Improvement to existing functionality non-breaking Non-breaking change labels Jun 8, 2022
Copy link
Contributor

@mdemoret-nv mdemoret-nv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design is growing more on me. Just had a couple of comments and the branch needs to be merged with the latest changes.

include/srf/node/sink_channel_base.hpp Outdated Show resolved Hide resolved
include/srf/segment/resource.hpp Outdated Show resolved Hide resolved
@ryanolson ryanolson changed the base branch from main to branch-22.06 June 17, 2022 19:15
@ryanolson ryanolson requested a review from a team as a code owner June 17, 2022 19:15
@ryanolson ryanolson added improvement and removed improvement Improvement to existing functionality labels Jun 17, 2022
@ryanolson
Copy link
Contributor Author

@gpucibot merge

@ghost ghost merged commit 866b454 into nv-morpheus:branch-22.06 Jun 17, 2022
@ryanolson ryanolson deleted the queue branch June 17, 2022 22:12
ghost pushed a commit that referenced this pull request Jul 29, 2022
ghost pushed a commit that referenced this pull request Oct 28, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants