IntIterator and LongIterator implement OfInt,OfLong #591
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This makes it easier to use in standard library situations that expect and OfInt or OfLong. The downside is that while practicaly compatible due to autoboxing existing users of the next() method might be slowed down if they do not migrate to nextLong().
|
Relates with #461 I may require the same API change, hence package change. |
Is that true? Certainly, that may happen if 'scalarisation' fails, but I am interested in examples where the iterator would be such that scalarisation would not happen. Could someone volunteer to benchmark this? |
|
My naive expectation is that if you take an integer, you box it, return the box, and then immediately unbox it to an integer... then the JIT will just optimize away the boxing. At least this ought to be true for 'some' versions of Java. This presentation makes me hopeful : https://www.youtube.com/watch?v=p1MhRBYS-k0 In any case, it is an empirical issue. We can test it out. |
|
@lemire I assumed as much in most cases. But the comment on IntIterator suggests that there are cases where scalarization used to fail and
With basically identical text in the IntIterator. |
Yes, I am sure that 'on some tests' this was true. We should not assume that it is still true today. Again, I am calling for folks to benchmark this. It may very well be that your PR has no practical performance impact. In other words, I am very supportive of this PR. My assumption is that it is all good. (Of course, we should test it out.) |
|
Well I ran into an issue :( when adapting the existing
this might lead to more issues downstream than I expected. |
|
Why would you need to adapt it? |
|
Oh to see the difference of running the test with |
|
@JervenBolleman Right but I think care must be needed not to extend it to |
|
Yes. That's an example where this PR would break existing code. :-( |
|
Here comes a very large table with benchmark results from my desktop with JDK1.8 on a cheap (AMD Ryzen 3 3200G). Which on a quick tired eyeballing seems that Next up JDk 17.0.4 |
|
JDK 8 is old at this point. |
|
Here comes the JDK-17 batch. Which I find surprising and very interesting. Just leaving the results here now that they are ready. Analysis to follow. |
blacelle
reviewed
Nov 15, 2022
| /** | ||
| * @return whether there is another value | ||
| */ | ||
| boolean hasNext(); |
There was a problem hiding this comment.
What about keeping these methods, adding .nextInt with a default implementation? it would enable 0.X compatibility.
There was a problem hiding this comment.
Ok. Both interface has a conflict over .next() (int or Integer). This was discussed in
-> is the need motivating by this PR covered by wrapping getIntIterator() in a RoaringOfInt as done in
|
I am marking this PR as 'work needed'. Before we adopt this, we need some more analysis. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes it easier to use in standard library situations that expect and OfInt or OfLong. The downside is that while practicaly compatible due to autoboxing existing users of the next() method might be slowed down if they do not migrate to nextLong().
SUMMARY
Since java 8 there has been an interface OfInt and OfLong that is similar to the IntIterator and LongIterator in ideas.
However instead of next returning the primitive, next returns an autoboxed one, with new methods nextInt/Long to return a primitive.
This pull request changes uses the two primitive Iterators to OfInt and OfLong. This makes it easier to interoperate with other code that expects these two interfaces. But with a significant downside that existing of
next()may have a significant regression in performance.I personally don't think this should be merged before the 0.10.x release series but wanted to open this pull request for discussion if and when such a patch should be merged.
Procedure to make this patch is trivial. Use rename re-factoring of the existing next to nextInt/Long methods then add the OfInt/Long to the existing Int/LongInterface.
Automated Checks
./gradlew testand made sure that my PR does not break any unit test../gradlew checkstyleMainor the equivalent and corrected the formatting warnings reported.