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

OperatorToObservableList: use LinkedList to buffer the sequence’s items #1208

Merged

Conversation

mttkay
Copy link
Contributor

@mttkay mttkay commented May 17, 2014

LinkedList has guaranteed constant insertion time when appending to the end of the list, whereas ArrayList takes O(1) amortized, since a reallocation might be necessary to insert further items. Since no capacity was specified for the buffer, on Hotspot this would cause the array to reallocate after the first 10 insertions, on Android even after the first insertion (since Android’s ArrayList uses a default capacity of zero.)

Since the buffer is copied to an ArrayList before emission, subscriber performance when working with the list should remain unaffected.

Sorry for the import noise, that's IntelliJ reformatting them every time I look at the file.

Refs #1141

LinkedList has guaranteed constant insertion time when appending to the end of the list, whereas ArrayList takes O(1) amortized, since a reallocation might be necessary to insert further items. Since no capacity was specified for the buffer, on Hotspot this would cause the array to reallocate after the first 10 insertions, on Android after the first insertion (since Android’s ArrayList uses a default capacity of zero.)

Since the buffer is copied to an ArrayList before emission, subscriber performance when working with the list should remain unaffected.

Refs ReactiveX#1141
@cloudbees-pull-request-builder

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

@benjchristensen
Copy link
Member

Why do we copy it to a new ArrayList before emitting?

I agree that this change will be fine because of that, but that seems odd.

Merging, and will let future performance discussions handle that case.

benjchristensen added a commit that referenced this pull request May 20, 2014
OperatorToObservableList: use LinkedList to buffer the sequence’s items
@benjchristensen benjchristensen merged commit 3f03935 into ReactiveX:master May 20, 2014
@benjchristensen
Copy link
Member

Added comments in #1218 to research this further.

@mttkay
Copy link
Contributor Author

mttkay commented May 20, 2014

Good question. It was copied before and I assumed it was done so as to not
leak the internal buffer to the subscriber. Since it won't be modified
after emission, this shouldn't make any difference in practice though?

Copying it makes more sense now that the buffer is a LinkedList, since for
large sequences this would change access times for callers when using
random access to elements. Then again, these are all hunches, so I can't
really say what the right thing to do is here. For us, it's highly unlikely
that list sequences would be longer than a few dozen items, so no harm
either way.
On May 20, 2014 6:15 AM, "Ben Christensen" notifications@github.com wrote:

Added comments in #1218 https://github.com/Netflix/RxJava/issues/1218to research this further.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1208#issuecomment-43584846
.

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.

3 participants