Optimize containsAll on RichIterable. #1344 #1353
Conversation
| @@ -295,17 +295,38 @@ else if (inside.size() > 32 && !(inside instanceof SetIterable)) | |||
| /** | |||
| * Returns true if all elements in source are contained in this collection. | |||
| * | |||
| * @since 1.0 | |||
| * @since 11.1 | |||
There was a problem hiding this comment.
We shouldn't change since tags. This method really was here since version 1.0.
| if (this.size() < inside.size()) | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
On line 306, we've already called size() on both collections. If the sizes are unequal, we can return false. In other words, replace < with !=.
There was a problem hiding this comment.
Would this be correct? A.containsAll(B) should return true iff all the elements of B are contained in A, so A and B don't necessarily have the same size.
There was a problem hiding this comment.
If A and B don't have the same size then they are definitely not equal.
| { | ||
| if (source instanceof RichIterable) | ||
| { | ||
| RichIterable<?> outside = this; |
There was a problem hiding this comment.
We already know that this is a RichIterable, because we're inside the interface. No need to assign it to another variable.
| if (outside.size() > 32 && !(outside instanceof SetIterable)) | ||
| { | ||
| outside = outside.toSet(); | ||
| } |
There was a problem hiding this comment.
To make the implementation consistent with RichIterable#containsNoneIterable.
| { | ||
| outside = outside.toSet(); | ||
| } | ||
| return inside.allSatisfy(outside::contains); |
There was a problem hiding this comment.
If you use Iterate.allSatisfy() then you don't need to check whether source is a RichIterable.
There was a problem hiding this comment.
@motlin Iterate.allSatisfy() can't be used in RichIterable as it is an implementation class.
There was a problem hiding this comment.
I'm skeptical that this is an optimization, and suspect it's slower in some cases. There are no JMH tests or other performance tests or commentary explaining why it's faster. The purpose of this change seems to be purely to optimize code since it is more verbose than the previous implementation. I don't think we ought to merge this until we can be confident it's an optimization.
| { | ||
| RichIterable<?> outside = this.toSet(); | ||
| return StreamSupport.stream(source.spliterator(), false).allMatch(outside::contains); | ||
| } |
There was a problem hiding this comment.
If this is a set, then calling toSet() is unnecessary, and this implementation is slower than necessary. I think we should leave this abstract on RichIterable.
| { | ||
| return source instanceof RichIterable ? this.containsAllIterable(source) | ||
| : source.stream().allMatch(this::contains); | ||
| } |
There was a problem hiding this comment.
I don't understand the conditional here. What does it matter if source is a RichIterable? It's being passed to a method that takes Iterable.
In addition, containsAllIterable() may convert this to a set, but if the collection is a plain one, not a RichIterable, then we don't convert anything to a set. Why the difference? If it's for performance, this inconsistency doesn't make sense to me.
Signed-off-by: YoussefAbdallah [email protected]