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

Reimplement the 'SkipLast' operator #1050

Merged
merged 4 commits into from
Apr 20, 2014

Conversation

zsxwing
Copy link
Member

@zsxwing zsxwing commented Apr 19, 2014

This PR did the following work:

  • Reimplemented the SkipLast operator to two different classes: OperatorSkipLast and OperatorSkipLastWithTimed.
  • Fixed a bug in OperatorSkipLastWithTimed that when all elements are valid, onCompleted will not be called. The unit test is testSkipLastTimedWhenAllElementsAreValid.

@cloudbees-pull-request-builder

RxJava-pull-requests #966 FAILURE
Looks like there's a problem with this pull request

/**
* Store the last count elements until now.
*/
private final Deque<T> deque = new LinkedList<T>();
Copy link
Member

Choose a reason for hiding this comment

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

How about using a ringbuffer instead of the linkedlist while we are at it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean ArrayDeque? If so, I need to wrap the value with a Notification since ArrayDeque does not support null.

Copy link
Member

Choose a reason for hiding this comment

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

Take a look at NotificationLite for efficient notification wrapping. Purely for internal use.

Copy link
Member Author

Choose a reason for hiding this comment

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

LinkedList:

Iteration   1: 18478.082 ns/op
Iteration   2: 18549.588 ns/op
Iteration   3: 18496.627 ns/op
Iteration   4: 18594.022 ns/op
Iteration   5: 18491.945 ns/op

Iteration   1: 18498.661 ns/op
Iteration   2: 18415.850 ns/op
Iteration   3: 18464.311 ns/op
Iteration   4: 18459.684 ns/op
Iteration   5: 18499.839 ns/op

ArrayDeque+Notification:

Iteration   1: 18277.651 ns/op
Iteration   2: 17660.270 ns/op
Iteration   3: 17575.652 ns/op
Iteration   4: 18055.774 ns/op
Iteration   5: 17477.764 ns/op

Iteration   1: 17488.280 ns/op
Iteration   2: 17413.479 ns/op
Iteration   3: 17503.564 ns/op
Iteration   4: 17376.135 ns/op
Iteration   5: 17450.446 ns/op

Looks ArrayDeque+Notification is better than LinkedList.

Copy link
Member Author

Choose a reason for hiding this comment

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

ArrayDeque+NotificationLite is impressive:

Iteration   1: 14854.581 ns/op
Iteration   2: 14842.878 ns/op
Iteration   3: 14940.360 ns/op
Iteration   4: 15131.214 ns/op
Iteration   5: 14767.790 ns/op

Iteration   1: 14604.962 ns/op
Iteration   2: 14606.918 ns/op
Iteration   3: 14624.361 ns/op
Iteration   4: 14739.717 ns/op
Iteration   5: 14541.750 ns/op

@cloudbees-pull-request-builder

RxJava-pull-requests #967 SUCCESS
This pull request looks good

@zsxwing zsxwing closed this Apr 19, 2014
@zsxwing zsxwing reopened this Apr 19, 2014
@zsxwing
Copy link
Member Author

zsxwing commented Apr 19, 2014

Updated to ArrayDeque+NotificationLite. Thank you, @akarnokd and @benjchristensen

@cloudbees-pull-request-builder

RxJava-pull-requests #968 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

RxJava-pull-requests #969 SUCCESS
This pull request looks good

benjchristensen added a commit that referenced this pull request Apr 20, 2014
Reimplement the 'SkipLast' operator
@benjchristensen benjchristensen merged commit 9f84ecb into ReactiveX:master Apr 20, 2014
@zsxwing zsxwing deleted the skip-last branch April 20, 2014 08:18
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.

None yet

4 participants