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

refactor(throttle): remove tryCatch/errorObject #1286

Merged
merged 3 commits into from
Feb 10, 2016
Merged

refactor(throttle): remove tryCatch/errorObject #1286

merged 3 commits into from
Feb 10, 2016

Conversation

tetsuharuohzeki
Copy link
Contributor

  • This change is same kind of Perf or no perf, there is no try... #1251.
  • I have not benchmarked this yet. So this is purely refactoring to unify code style.
    • The current codebase does not have any benchmark about throttle().
    • throttle() suppress the value traffic. How do we benchmark this without a throughput benchmark?

@benlesh
Copy link
Member

benlesh commented Feb 2, 2016

Can you flatten the first two commits? There's no reason for them to be separate, really. They're just TypeScript fixes.

@benlesh benlesh added the blocked label Feb 2, 2016
@tetsuharuohzeki
Copy link
Contributor Author

I added the micro perf benchmars for throttle() ( tetsuharuohzeki@8963abb ), but RxJS v4 does not accept Observable<T> as a durationSelector (see also #1321). So we should only watch the v5's result.
(Should we really add this benchmark to the tree?)

These are results of benchmarks (this table drops v4's results):

Before this pull request

                                         |             RxJS 5.0.0-beta.1 |
----------------------------------------------------------------------------
                    throttle - immediate |                38,030 (±2.18%) |
                                throttle |                32,392 (±5.58%) |

My first patch

                                         |             RxJS 5.0.0-beta.1 |  
----------------------------------------------------------------------------
                    throttle - immediate |                45,254 (±1.21%) |
                                throttle |                33,630 (±3.33%) |

@Blesh's proposal

                                         |             RxJS 5.0.0-beta.1 |  
----------------------------------------------------------------------------
                    throttle - immediate |                58,980 (±5.69%) |
                                throttle |                41,971 (±5.14%) |

By these results, @Blesh's suggestion is right!

@benlesh benlesh added PR: lgtm and removed blocked labels Feb 10, 2016
@tetsuharuohzeki
Copy link
Contributor Author

@kwonoj

updated :)
And, I add some comments which explain why we cannot compare with v4's throttle(durationSelector) to tetsuharuohzeki@6060977.

@kwonoj kwonoj merged commit 7bae860 into ReactiveX:master Feb 10, 2016
@kwonoj
Copy link
Member

kwonoj commented Feb 10, 2016

Merged with 7bae860, thanks @saneyuki !

@tetsuharuohzeki tetsuharuohzeki deleted the throttle branch February 12, 2016 14:11
@lock
Copy link

lock bot commented Jun 7, 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 7, 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.

None yet

3 participants