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

Fixes memory leak of grouped iterator #1907

Merged
merged 8 commits into from
Mar 12, 2017
72 changes: 40 additions & 32 deletions javaslang/src/main/java/javaslang/collection/Iterator.java
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import javaslang.Tuple3;
import javaslang.collection.IteratorModule.ConcatIterator;
import javaslang.collection.IteratorModule.DistinctIterator;
import javaslang.collection.IteratorModule.GroupedIterator;
import javaslang.control.Option;

import java.math.BigDecimal;
Expand All @@ -20,6 +21,7 @@
import static java.lang.Double.NEGATIVE_INFINITY;
import static java.lang.Double.POSITIVE_INFINITY;
import static java.math.RoundingMode.HALF_UP;
import static javaslang.API.*;
import static javaslang.collection.IteratorModule.BigDecimalHelper.areEqual;
import static javaslang.collection.IteratorModule.BigDecimalHelper.asDecimal;
import static javaslang.collection.IteratorModule.EmptyIterator;
Expand Down Expand Up @@ -1535,9 +1537,9 @@ default <C> Map<C, Iterator<T>> groupBy(Function<? super T, ? extends C> classif

@Override
default Iterator<Seq<T>> grouped(int size) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Scala's grouped() and sliding() functions return a GroupedIterator<Seq<T>>. That GroupedIterator has the ability of padding and skipping partial results (both not implemented in our version, yet).

It would be nice if we could provide the same functionality. Returning a GroupedIterator<Seq<T>> instead of Iterator<Seq<T>> will not be backward compatible. I will target that change for 3.0.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: I internally based the buffer on our persistent Vector, Scala users a mutable ArrayBuffer. Our version is blazing fast (a little slower than the mutable version). Congrats @paplorinc !!!

Copy link
Contributor

Choose a reason for hiding this comment

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

nice, could you share some benchmarks :)?

return sliding(size, size);
return new GroupedIterator<>(this, size, size);
}

@Override
default boolean hasDefiniteSize() {
return false;
Expand Down Expand Up @@ -1827,37 +1829,9 @@ default Iterator<Seq<T>> sliding(int size) {

@Override
default Iterator<Seq<T>> sliding(int size, int step) {
if (size <= 0 || step <= 0) {
throw new IllegalArgumentException("size: " + size + " or step: " + step + " not positive");
}
if (!hasNext()) {
return empty();
} else {
final Stream<T> source = Stream.ofAll(this);
return new AbstractIterator<Seq<T>>() {
private Stream<T> that = source;
private IndexedSeq<T> next = null;

@Override
public boolean hasNext() {
while (next == null && !that.isEmpty()) {
final Tuple2<Stream<T>, Stream<T>> split = that.splitAt(size);
next = split._1.toVector();
that = split._2.isEmpty() ? Stream.empty() : that.drop(step);
}
return next != null;
}

@Override
public IndexedSeq<T> getNext() {
final IndexedSeq<T> result = next;
next = null;
return result;
}
};
}
return new GroupedIterator<>(this, size, step);
}

@Override
default Tuple2<Iterator<T>, Iterator<T>> span(Predicate<? super T> predicate) {
Objects.requireNonNull(predicate, "predicate is null");
Expand Down Expand Up @@ -2089,6 +2063,40 @@ public String toString() {
}
}

final class GroupedIterator<T> implements Iterator<Seq<T>> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if we should really switch to Iterator (instead of AbstractIterator) in version 2.1.0 for slightly better performance...

In 3.0.0 I think we should use the faster Iterator (without a pretty toString()).


private final Iterator<T> that;
private final int step;
private final int gap;

private List<T> buffer;
Copy link
Contributor

@l0rinc l0rinc Mar 11, 2017

Choose a reason for hiding this comment

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

is List faster than Vector for this scenario?


GroupedIterator(Iterator<T> that, int size, int step) {
if (size < 1 || step < 1) {
throw new IllegalArgumentException("size (" + size + ") and step (" + step + ") must both be positive");
}
this.that = that;
this.step = step;
this.gap = Math.max(step - size, 0);
this.buffer = List.ofAll(that.take(size));
}

@Override
public boolean hasNext() {
return !buffer.isEmpty();
}

@Override
public Seq<T> next() {
if (!hasNext()) {
throw new NoSuchElementException();
}
final Seq<T> result = buffer;
buffer = that.hasNext() ? that.drop(gap).take(step).toList().prependAll(buffer.drop(step)) : buffer.take(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

  • drop(...).take(...) seems like a slice
  • you could use appendAll also, it's more intuitive to start with the prefix (it's implemented via prependAll in List anyway)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did appendAll() before with Vector. List.prependAll() performs better in this use-case. See above. I will add a benchmark.

Copy link
Contributor

Choose a reason for hiding this comment

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

appendAll uses prependAll internally:

@Override
default List<T> appendAll(Iterable<? extends T> elements) {
    Objects.requireNonNull(elements, "elements is null");
    return List.<T> ofAll(elements).prependAll(this);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think about adding appendAll() to Traversable and therefor to Iterator. we could then operate entirely on the Iterator and finally call toList(). Maybe it is simpler to read. However, each operation requires a new Iterator instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmhh, does not seem to be applicable here...

Copy link
Contributor

Choose a reason for hiding this comment

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

If you're lucky it will end up on the stack (i.e. escape analysis), you should measure the impact :)
Seems more readable/maintainable though, that's more important for me personally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't done such measures yet - no clue which tool to use in order to see how things are organized within the JVM 🙄 do you have a hint?

Copy link
Contributor

Choose a reason for hiding this comment

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

JMH has some nice profiler addons and https://github.com/AdoptOpenJDK/jitwatch is also quite good for pure Java (couldn't use it for Scala though).
Let me know if you need help.

return result;
}
}

final class BigDecimalHelper {

@GwtIncompatible("Math::nextDown is not implemented")
Expand Down
18 changes: 12 additions & 6 deletions javaslang/src/main/java/javaslang/collection/List.java
Original file line number Diff line number Diff line change
Expand Up @@ -720,6 +720,12 @@ default <U> List<T> distinctBy(Function<? super T, ? extends U> keyExtractor) {

@Override
default List<T> drop(int n) {
if (n <= 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

return this;
}
if (n >= size()) {
return empty();
}
List<T> list = this;
for (long i = n; i > 0 && !list.isEmpty(); i--) {
list = list.tail();
Expand Down Expand Up @@ -1390,12 +1396,12 @@ default Option<List<T>> tailOption() {

@Override
default List<T> take(int n) {
if (n >= length()) {
return this;
}
if (n <= 0) {
return empty();
}
if (n >= length()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the difference here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we do the length() check second now

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that, but why is it important? It's a constant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I forgot, JIT magic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

every runtime env? GWT resp Javascript?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if I can sleep better we have gained s.th.

Copy link
Contributor

Choose a reason for hiding this comment

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

This could help you even more: https://rainymood.com

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😴

Copy link
Contributor

Choose a reason for hiding this comment

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

I use it sometimes when the environment is noisy, it's very relaxing :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try it at $work :)

return this;
}
List<T> result = Nil.instance();
List<T> list = this;
for (int i = 0; i < n; i++, list = list.tail()) {
Expand All @@ -1406,12 +1412,12 @@ default List<T> take(int n) {

@Override
default List<T> takeRight(int n) {
if (n >= length()) {
return this;
}
if (n <= 0) {
return empty();
}
if (n >= length()) {
return this;
}
return reverse().take(n).reverse();
}

Expand Down