Skip to content

Suggest not requesting one-by-one in rule 2.1 #435

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

Merged
merged 4 commits into from
Jul 20, 2018

Conversation

ktoso
Copy link
Contributor

@ktoso ktoso commented Jul 4, 2018

Follow up from #433 (comment)

Improves intent wording to explain that implementations should try to request as many elements as it makes sense, and not only request(1) as it defeats the pipelining potential of reactive streams.

README.md Outdated
@@ -131,7 +131,7 @@ public interface Subscriber<T> {
| ID | Rule |
| ------------------------- | ------------------------------------------------------------------------------------------------------ |
| <a name="2.1">1</a> | A `Subscriber` MUST signal demand via `Subscription.request(long n)` to receive `onNext` signals. |
| [:bulb:](#2.1 "2.1 explained") | *The intent of this rule is to establish that it is the responsibility of the Subscriber to signal when, and how many, elements it is able and willing to receive.* |
| [:bulb:](#2.1 "2.1 explained") | *The intent of this rule is to establish that it is the responsibility of the Subscriber to signal when, and how many, elements it is able and willing to receive. It is RECOMMENDED to request as many elements as a Subscriber is prepared to receive (e.g. has preallocated buffer space for), rather than requesting "one by one".* |
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, the buffer space part only applies for asynchronous Subscribers.

Would this be more general?

For performance reasons it is RECOMMENDED to request as many elements as a Subscriber is currently able to receive, as requesting elements one at a time will be the slowest possible option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I don't like starting with "for performance reasons", always feels hand wavy right away, but good rewording; how about:

It is RECOMMENDED to request as many elements as a Subscriber is currently able to receive, as the most conservative strategy to signal demand by requesting elements one-at-a-time is the inherently inefficient.

?

@DougLea
Copy link
Contributor

DougLea commented Jul 5, 2018

Or maybe:

It is RECOMMENDED that Subscribers request as many elements as can be processed without loss, within their resource limits. Requesting only one item at a time results in an inherently inefficient "stop-and-wait" protocol.

Maybe we could add a link to something comparing stop-and-wait to windowing. All the ones that come to mind assume people know about TCP protocols etc, which might not be helpful.

@ktoso
Copy link
Contributor Author

ktoso commented Jul 6, 2018

I like that wording a lot @DougLea, thanks!
I think it is good even without linking to more stop-and-wait definitions (e.g. the https://en.wikipedia.org/wiki/Stop-and-wait_ARQ derails quickly into cases with "ack being lost" and other scenarios that could/would confuse people in the context of RS) – the "inherently inefficient stop-and-wait" already invokes the right image in mind I think..

Would it be fine if I use your proposed wording in this PR?

@viktorklang
Copy link
Contributor

viktorklang commented Jul 6, 2018

@DougLea @ktoso Great feedback!

Could it perhaps be distilled further, something along the lines of:

It is RECOMMENDED that Subscribers request the upper limit of what they are able to process, as requesting only one element at a time results in an inherently inefficient "stop-and-wait" protocol.

@ktoso
Copy link
Contributor Author

ktoso commented Jul 6, 2018

Sounds find to me. The "without loss" phrasing is not strictly necessary I think (I mean it could be a dropping / filtering operation after all)

@viktorklang
Copy link
Contributor

@ktoso Right, and some operators do not want too trigger much look-ahead (a headOption / findFirst).

Copy link
Contributor

@viktorklang viktorklang left a comment

Choose a reason for hiding this comment

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

Excellent!

@DougLea
Copy link
Contributor

DougLea commented Jul 6, 2018

lgtm

@viktorklang
Copy link
Contributor

@ktoso Rebase and resolve conflict and I'll merge. :)

@ktoso
Copy link
Contributor Author

ktoso commented Jul 20, 2018

resolved

@viktorklang
Copy link
Contributor

Perfect, thanks @ktoso!

@viktorklang viktorklang merged commit 9464664 into reactive-streams:master Jul 20, 2018
@viktorklang viktorklang added this to the 1.0.3 milestone Jul 20, 2018
@ktoso ktoso deleted the patch-4 branch July 20, 2018 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants