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

Use singleton reduction functions in count and countLong #2880

Merged
merged 1 commit into from
Apr 21, 2015

Conversation

davidmoten
Copy link
Collaborator

Every call to Observable.count() and countLong() instantiated a new Func2 which I have now made a singleton in the interests of minimizing GC pressure.

Can anyone in the know confirm for me that there is GC benefit from this one because I assume a sophisticated enough compiler could replace the new Func2 call with a singleton during its optimizations.

If this one is worthwhile then there are heaps more of these in Observable.java and possibly elsewhere.

@akarnokd
Copy link
Member

Since we are Java 6+, we can't assume a bunch of optimizations happen so what's left is manual optimization/inlining.

@akarnokd
Copy link
Member

I'd instead do this directly with a long counting Subscriber and for int, I'd map it to min(count, Integer.MAX_VALUE) at the end saving the duplications.

@davidmoten
Copy link
Collaborator Author

I guess this is an opportunity to decide what overflow behaviour we desire. I see three options:

  1. be consistent with Integer/Long overflow behaviour
  2. return Integer.MAX_VALUE/Long.MAX_VALUE on overflow
  3. throw an error on overflow

My preference is 3.

@zsxwing
Copy link
Member

zsxwing commented Apr 17, 2015

I think both 2 and 3 will be a breaking change so it's not acceptable. 1 is not a big deal since it's consistent with Java, which all Java users should be familiar with it.

@zsxwing
Copy link
Member

zsxwing commented Apr 21, 2015

@davidmoten thoughts?

@davidmoten
Copy link
Collaborator Author

I'm happy to maintain the existing overflow behaviour so it's not a
breaking change. Given that I think the PR could go in as is.

On Tue, 21 Apr 2015 18:57 Shixiong Zhu [email protected] wrote:

@davidmoten https://github.com/davidmoten thoughts?


Reply to this email directly or view it on GitHub
#2880 (comment).

zsxwing added a commit that referenced this pull request Apr 21, 2015
Use singleton reduction functions in count and countLong
@zsxwing zsxwing merged commit 9fb5614 into ReactiveX:1.x Apr 21, 2015
@zsxwing
Copy link
Member

zsxwing commented Apr 21, 2015

Thanks.

@benjchristensen benjchristensen mentioned this pull request Apr 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants