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

Replaced SoftReference with WeakReference for better performance #151

Merged
merged 5 commits into from Jun 13, 2015
Merged

Replaced SoftReference with WeakReference for better performance #151

merged 5 commits into from Jun 13, 2015

Conversation

orionll
Copy link
Contributor

@orionll orionll commented Jun 8, 2015

Benchmark:

import com.google.common.base.Stopwatch;
import fj.data.Stream;

public class Main {
    public static void main(String[] args) {
        int to = 3000000;
        Stream<Integer> range = Stream.range(1, 3000000);

        Stopwatch sw = Stopwatch.createStarted();
        Integer sum = range.foldLeft(x -> y -> x + y, 0);
        System.out.println("First run: " + sw.stop());

        sw.start();
        sum = range.foldLeft(x -> y -> x + y, 0);
        System.out.println("Second run: " + sw.stop());
    }
}

The result with SoftReference:

First run: 13,31 s
Second run: 32,68 s

WeakReference:

First run: 219,9 ms
Second run: 388,6 ms

SoftReference is about 70 times slower.

I also performed a profiling session with JVisualVM. With SoftReference there are 125 collections total and garbage collection took 23 seconds! With WeakReference - 29 collections and 25 milliseconds.

@jbgi
Copy link
Member

jbgi commented Jun 8, 2015

but with WeakReference there may be no point to memoize at all: depending what you do with it, the memoized result can be garbaged really fast, even before a second iteration on the stream where memorization is useful.
Another benchmark taking advantage of memoization would be useful.

@orionll
Copy link
Contributor Author

orionll commented Jun 8, 2015

We use a cache of parsed expressions in production (com.google.common.cache.Cache<String, ASTNode>). We tested different implementations of cache (WeakReference-based, SoftReference-based) and also no caching at all (each expression is parsed every time). The SoftReference-based cache showed terrible performance, even worse than no caching. The WeakReference-based cache showed the best performance, and we are using it in production now.

@jbgi
Copy link
Member

jbgi commented Jun 8, 2015

I think there is no general rule, it depends of how expensive it is to re-compute cached values and how frequently you need them. Maybe we need the three memo implementation to let the user chose the most appropriate one:

  • hardMemo() (basically com.google.common.base.Suppliers.MemoizingSupplier)
  • softMemo() (current implementation)
  • weakMemo() (your patch)

It is totally possible that for most use cases of streams weakMemo give the best results.

@jbgi
Copy link
Member

jbgi commented Jun 8, 2015

What I meant is that we should not change the current memo() behavior (which would be an alias for softMemo()) and maybe instead change Stream implementation to use a new weakMemo() method on P1.

@orionll
Copy link
Contributor Author

orionll commented Jun 8, 2015

+1 for adding three distinct methods and removing memo().

@orionll
Copy link
Contributor Author

orionll commented Jun 9, 2015

So, I think there should be three methods:

  • memo() - based on strong references
  • weakMemo() - based on weak references
  • softMemo() - based on soft references

See my changes.

@jbgi
Copy link
Member

jbgi commented Jun 9, 2015

@mperry can we change memo() memory behaviour without a full deprecation cycle?

@techtangents
Copy link

How about we keep memo(), but add an overload for it that passes in the
memoization strategy.

e.g.
memo(Memo.hardReference)
memo(Memo.softReference)
memo(Memo.weakReference)

then memo() would be implemented as:

memo(Memo.weakReference)

or whichever behavior you want as default.

JB Giraudeau mailto:notifications@github.com
10 June 2015 2:27 am

@mperry https://github.com/mperry can we change memo() memory
behaviour without a full deprecation cycle?


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

Zheka Kozlov mailto:notifications@github.com
8 June 2015 8:00 pm

Benchmark:

import com.google.common.base.Stopwatch;
import fj.data.Stream;

public class Main {
public static void main(String[] args) {
int to = 3000000;
Stream range = Stream.range(1, 3000000);

Stopwatch sw = Stopwatch.createStarted();
Integer sum = range.foldLeft(x -> y -> x + y, 0);
System.out.println("First run: " + sw.stop());

sw.start();
sum = range.foldLeft(x -> y -> x + y, 0);
System.out.println("Second run: " + sw.stop());
}
}

The result with SoftReference:

|First run: 13,31 s
Second run: 32,68 s
|

WeakReference:

|First run: 219,9 ms
Second run: 388,6 ms
|

SoftReference is about 70 times slower.

I also performed a profiling session with JVisualVM. With
SoftReference there are 125 collections total and garbage collection
took 23 seconds! With WeakReference - 29 collections and 25 milliseconds.


    You can view, comment on, or merge this pull request online at:

#151

    Commit Summary


Reply to this email directly or view it on GitHub
#151.

@orionll
Copy link
Contributor Author

orionll commented Jun 10, 2015

I don't see any benefits of it

… StackOverflow. Added Ordering.reverse(), Ord.reverse().
}
};
}
}, value(List.<A>nil()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This caused StackOverflow

@@ -625,18 +624,18 @@ public final void foreachDoEffect(final Effect1<A> f) {
* @return A new list that has appended the given list.
*/
public final List<A> append(final List<A> as) {
return fromList(this).append(as).toList();
return Buffer.fromList(this).prependToList(as);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

List.append() was O(n+m). I made it O(n)

@mperry
Copy link
Contributor

mperry commented Jun 13, 2015

I agree with @techtangents, I think we should provide a default memo implementation and then other options to choose from a hard, soft and weak implementation. Whether this is separate hard, soft and weak methods or a parameter to memo does not matter too much.

@mperry
Copy link
Contributor

mperry commented Jun 13, 2015

@jbgi we can change the implementation of memo memory implementation without deprecation. We don't provide guarantees on the implementation, so changing the implementation when the type signature remains the same I think is fine.

@mperry
Copy link
Contributor

mperry commented Jun 13, 2015

I think this issue is complete enough to merge. I created #153 to handle hard, soft, weak and default memo implementations.

mperry added a commit that referenced this pull request Jun 13, 2015
Replaced SoftReference with WeakReference for better performance
@mperry mperry merged commit fe5c13c into functionaljava:master Jun 13, 2015
@mperry mperry added this to the v4.4 milestone Jul 5, 2015
@mperry mperry added the internal label Jul 5, 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.

None yet

4 participants