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

feat(zip): zip now supports never-ending iterables #399

Merged
merged 1 commit into from
Sep 25, 2015
Merged

feat(zip): zip now supports never-ending iterables #399

merged 1 commit into from
Sep 25, 2015

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented Sep 24, 2015

  • adds support for never-ending iterables
  • adds fast-track for arrays
  • adds additional tests for zip
  • fixes issues where completion does not happen at the appropriate moment

closes #397

@benlesh
Copy link
Member Author

benlesh commented Sep 24, 2015

The controversial thing here is that it reduces performance slightly from what is in master...

from 98,000 ops/sec
to 78,000 ops/sec

Still substantially faster than the older versions of RxJS.

However... this does include fixes to how the returned observable completes. It's hard to compare apples to apples until those fixes are in what's in master. So that will be next.

@benlesh
Copy link
Member Author

benlesh commented Sep 24, 2015

With (one possible set of) fixes to zip in #400, the perf drops anyway to 89,000...

So really the comparison for this feature is likely: 89,000 vs 78,000 ops/sec.

The upside is it enables zipping with an "infinite" resource without locking a thread or creating a huge buffer.

@benlesh
Copy link
Member Author

benlesh commented Sep 24, 2015

@benlesh
Copy link
Member Author

benlesh commented Sep 24, 2015

@steveorsomethin ... since you're more in-tune with TVUI/Netflix perf needs for these operators, do you view the perf differences mentioned above as okay vs the value of this PR?

One reason I feel like this is okay is it makes zip slightly less dangerous.

- adds support for never-ending iterables
- adds fast-track for arrays
- adds additional tests for zip
- fixes issues where completion does not happen at the appropriate moment

closes #397
@benlesh benlesh merged commit a5684ba into ReactiveX:master Sep 25, 2015
@benlesh
Copy link
Member Author

benlesh commented Sep 25, 2015

Per a conversation with @steveorsomethin and @ktrott: The small change in performance isn't enough to outway the benefit of the new functionality and shouldn't effect anything in the lower-end hardware or non-JIT'ed runtimes Netflix needs to support.

Merged.

@benlesh benlesh deleted the zip_changes branch December 16, 2016 03:08
@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add special handling for Iterables passed to zip operators
1 participant