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

Added benchmarks for common java idioms #1456

Merged
merged 1 commit into from Oct 23, 2016

Conversation

mping
Copy link
Contributor

@mping mping commented Jul 21, 2016

Hopefully will have the following benchmarks:

  • For comprehension
  • Tuple vs Pair class?
  • Try
  • Pattern Matching

@mping
Copy link
Contributor Author

mping commented Jul 21, 2016

@danieldietrich this is just preliminary to see if the general outline of the test looks good.
I've actually never used javaslang in my projects so let me know if it doesn't look good.


@Benchmark
public Object javaslang_for() {
Integer result = For(ELEMENTS_ARRAY).yield(i -> i).reduce((i, j) -> i + j);
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for providing alternative benchmarks :).
It would be nice if the build would be passing before asking for code reviews.
Also, please use the same style as the other benchmarks do (e.g. final local variables, method naming, encapsulation in separate classes with a base class)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will update, didnt notice it was triggering CI.

@mping
Copy link
Contributor Author

mping commented Jul 27, 2016

Tests are failing because of OOM, since For comprehension essentially does a cross product I'll use smaller arrays.

for (Integer i : ELEMENTS_ARRAY) {
for (Integer j : ELEMENTS_ARRAY) {
for (Integer k : ELEMENTS_ARRAY) {
result.add(i * j * k * 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

* 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.

Just needed something to ensure the assert is simple.

ELEMENTS_ARRAY = Arrays.asList(ELEMENTS);
}

protected Object consume(Blackhole bh, Integer i) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Blackhole is from JMH and is designed for the case where you don't need/want to check the result but you do want to prevent DCE, so it's basically something to emulate a side-effect function.

@mping
Copy link
Contributor Author

mping commented Jul 27, 2016

Tests should be simpler now, here's what I have (cannot explain the 2nd result)

Ratios slang / <alternative_impl>
Target        Operation                   Ratio                              10 
ForBenchmark  ForComprehension1Iterator   javaslang_for/java_for          0,88x
ForBenchmark  ForComprehension2Iterators  javaslang_for/java_for         11,89x

@paplorinc
Copy link
Contributor

Will check it out next week (I'm very busy, sorry), until then, please simplify everything as much as possible.
If you're testing the speed of for loops, don't include everything else in it.
Just iterate over the array and either consume it via the blackhole, or rather collect it via a cheap operation like xor so that we can have some assertions that fail if we mess it up (like they failed until now when you made an error, that's exactly why we need them :)

@codecov-io
Copy link

codecov-io commented Jul 27, 2016

Current coverage is 96.84% (diff: 100%)

Merging #1456 into master will not change coverage

@@             master      #1456   diff @@
==========================================
  Files            88         88          
  Lines         10675      10675          
  Methods           0          0          
  Messages          0          0          
  Branches       1818       1818          
==========================================
  Hits          10338      10338          
  Misses          194        194          
  Partials        143        143          

Powered by Codecov. Last update 403226f...cd1b8e0

@mping
Copy link
Contributor Author

mping commented Jul 27, 2016

Will use a xor, thanks.

public int CONTAINER_SIZE;

Integer[] ELEMENTS;
List<Integer> ELEMENTS_ARRAY;
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't seem like an array :)
Do you need both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I just need a list.

@mping
Copy link
Contributor Author

mping commented Jul 31, 2016

Geez, I'm getting horrible results with the XOR trick. @paplorinc can you share you idea with some code? I'm currently doing this:

Optional<Integer> res = For(ELEMENTS, ELEMENTS).yield((i, j) -> i ^ j).collect(XOR);

But the results are horrible in comparison to the nested for. I'm guessing I am doing something wrong, feel free to point me in the right direction.

@paplorinc
Copy link
Contributor

I would be surprised if the simple for loop (which is often optimized by JIT anyway) wouldn't be at least 10x faster ... maybe even 100x.
However, you should get the same results with or without the xor (actually any constant size operation would only bring the values closer together).
Please try to debug it manually, I would do the same :).

@mping
Copy link
Contributor Author

mping commented Aug 1, 2016

Cool, I will proceed then, and add more benchmarks, for the rest of the idioms.
The For().yield implementation is quite simple via flatmap but I was wondering if it wouldn't be better to just nest the loops to squeeze more performance.

@paplorinc
Copy link
Contributor

Try it out and report your findings :)

@mping
Copy link
Contributor Author

mping commented Aug 2, 2016

Added a couple more benchmarks, now only pattern matching is missing.

Ratios <alternative_impl> / slang
Target        Operation                                                                          Ratio           

-- try -- 
TryBenchmark  TryThrowsCatchBenchmark         java_try_actual_exception/javaslang_try_actual_exception     2,20x
TryBenchmark  TryNoThrowsCatchBenchmark               java_try_no_exception/javaslang_try_no_exception     0,87x

-- tuple --
TupleBenchmark  Tuple2Benchmark       java_tuple2_creation/javaslang_tuple2_creation     1,00x


-- for comprehension  --
ForBenchmark  ForComprehensionNestedFor       java_for/javaslang_for    11,47x



```
mvn clean test -Pbenchmark
mvn clean test -pl javaslang-benchmark -Dtest=TryBenchmark
Copy link
Member

Choose a reason for hiding this comment

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

Do we already run a specific microbenchmark here? See line 37 and line 40.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, too much copy/paste. Will fix.

@mping
Copy link
Contributor Author

mping commented Aug 24, 2016

No I don't. Will check it out today. Was an assert that failed :(
@danieldietrich is there a report for the benchmarks generated anywhere? would love to write a blog post on this.

@danieldietrich
Copy link
Member

danieldietrich commented Aug 24, 2016

A blog post would be awesome! I would appreciate to review it before publishing because interpreting benchmarks is a difficult topic. @paplorinc and others may also be interested to take a look!

Actually I don't know how to run the benchmarks :-/ The recent command mvn test -Pbenchmark -pl javaslang does not work any more. I asked @paplorinc for the details and will keep you up-to-date. We will document it, see #1527

@paplorinc
Copy link
Contributor

paplorinc commented Aug 24, 2016

Sure, a blog post would be nice, but a lot of things are very hard to interpret here currently, I really urge you to understand JMH well first, it's a lot more difficult than it seems - and as I tried to point out in these benchmarks, they are still far from being finished.

@mping
Copy link
Contributor Author

mping commented Aug 24, 2016

I appreciate everyone's advice and input, I realize it's very tricky to interpret the results but my goal was never to provide an accurate measurement. I just want to get an idea of what to expect in terms of performance. I know using JMH is hard, but I think this is a good and practical way to learn

@danieldietrich
Copy link
Member

@mping Yes, we need that blog post. I would greatly appreciate if you would write a guest post on the Javaslang blog, including your Bio (/Photo etc) :-) The format is would be markdown.


@State(Scope.Benchmark)
public static class Base {
@Param({ "10" })
Copy link
Contributor

Choose a reason for hiding this comment

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

Please provide more params so that we can observe a trend

@danieldietrich danieldietrich modified the milestones: 2.2.0, 2.1.0 Aug 26, 2016
- For comprehension
- Tuple
- Try
- Pattern Matching
@danieldietrich danieldietrich modified the milestones: 2.1.0, 2.2.0 Oct 23, 2016
@danieldietrich
Copy link
Member

I will pull this PR in now - we need to burn down our issues and PRs in order to get forward. Optimizations of the Benchmarks can occur later.

@mping Many thanks!!

@danieldietrich danieldietrich merged commit 120f85f into vavr-io:master Oct 23, 2016
@mping
Copy link
Contributor Author

mping commented Oct 24, 2016

@danieldietrich yay thanks!

@AnthonyKot
Copy link

AnthonyKot commented Aug 16, 2017

JFYI, looks like the error was mentioned by danieldietrich on 24 Aug 2016 is still here:
"Test set: io.vavr.idiom.ForBenchmark

Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.475 sec <<< FAILURE! - in io.vavr.idiom.ForBenchmark
testAsserts(io.vavr.idiom.ForBenchmark) Time elapsed: 0.475 sec <<< ERROR!
java.lang.RuntimeException: org.openjdk.jmh.runner.RunnerException: Benchmark caught the exception
at io.vavr.idiom.ForBenchmark.testAsserts(ForBenchmark.java:38)
Caused by: org.openjdk.jmh.runner.RunnerException: Benchmark caught the exception
at io.vavr.idiom.ForBenchmark.testAsserts(ForBenchmark.java:38)
Caused by: org.openjdk.jmh.runner.BenchmarkException: Benchmark error during the run
at io.vavr.idiom.ForBenchmark.testAsserts(ForBenchmark.java:38)"

@danieldietrich
Copy link
Member

@AnthonyKot thank you, I will create an issue

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

5 participants