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

What is PerformanceBenchmarkTest exactly supposed to do? #34

Closed
wind57 opened this issue Jun 11, 2020 · 2 comments
Closed

What is PerformanceBenchmarkTest exactly supposed to do? #34

wind57 opened this issue Jun 11, 2020 · 2 comments

Comments

@wind57
Copy link

wind57 commented Jun 11, 2020

I am a little confused about PerformanceBenchmarkTest.

Anyway, these are nitpicks, mainly. The idea is that I hoped this test would have some samples against some popular frameworks. I have seen https://github.com/arey/java-object-mapper-benchmark, though. It's just that your results rely on those tests and I am not sure about their correctness (without understanding them, those are meaningless numbers: look how the benchmark presents that "direct" mapping is slower than "map-struct"). I wonder if I should provide a PR against some data mappings libraries inside (or a separate project/child from datus)?

@roookeee
Copy link
Owner

roookeee commented Jun 11, 2020

Hi @wind57,

.jvmArgs("-server") is an ancient setting, that is simply ignored in the JVM.

Sure I can remove that.

Second, what is the point of having a single @benchmark? You are supposed to benchmark against something else, I guess.

The point of having a single benchmark is to collect data of a single use-case. It might seem counter intuitive but it helps me to benchmark two different versions of datus on the same setup. Before releasing a new version of datus I run said benchmark on the previous version and compare potential performance regressions.

it seems your testedConversionShouldBeCorrect, just ensures that the conversion is correct, but that is something JMH already offers via @teardown: https://hg.openjdk.java.net/code-tools/jmh/file/b6f87aa2a687/jmh-samples/src/main/java/org/openjdk/jmh/samples/JMHSample_05_StateFixtures.java

Well I am using assertJ facilities as assert is ancient Java and noone really uses it nowadays (which is a subjective answer).

Anyway, these are nitpicks, mainly. The idea is that I hoped this test would have some samples against some popular frameworks. I have seen https://github.com/arey/java-object-mapper-benchmark, though. It's just that your results rely on those tests and I am not sure about their correctness (without understanding them, those are meaningless numbers: look how the benchmark presents that "direct" mapping is slower than "map-struct")

There's that tricky question :) I wanted to do that in #23 but decided against it as I would have to maintain other mapper implementations in my repository and people would complain about non-idiomatic / non-best-performing usage of other frameworks. That's why I have linked to the repository you linked to in the PerformanceBenchmarkTest. I have highlighted the "weird" performance issues here but it seems like the author did not want to update the numbers. Datus numbers have improved by 50% too compared to the last run so... there's that :)

I wonder if I should provide a PR against some data mappings libraries inside (or a separate project/child from datus)?

To put it bluntly: I still don't want to maintain 3rd-party mapper implementations for 8-XX frameworks for the aforementioned reasons. I think it would be best to go to arey's repo and open issues etc.

Thank you for your issue

@wind57
Copy link
Author

wind57 commented Jun 11, 2020

Hi @wind57,

.jvmArgs("-server") is an ancient setting, that is simply ignored in the JVM.

Sure I can remove that.

Thank you.

Second, what is the point of having a single @benchmark? You are supposed to benchmark against something else, I guess.

The point of having a single benchmark is to collect data of a single use-case. It might seem counter intuitive but it helps me to benchmark two different versions of datus on the same setup. Before releasing a new version of datus I run said benchmark on the previous version and compare potential performance regressions.

I see. Yes, that makes sense.

it seems your testedConversionShouldBeCorrect, just ensures that the conversion is correct, but that is something JMH already offers via @teardown: https://hg.openjdk.java.net/code-tools/jmh/file/b6f87aa2a687/jmh-samples/src/main/java/org/openjdk/jmh/samples/JMHSample_05_StateFixtures.java

Well I am using assertJ facilities as assert is ancient Java and noone really uses it nowadays (which is a subjective answer).

I did not meant to use assert, but @TearDown in general : that would be the place to make tests against the correctness. That could be done with simple if/else tests. Anyway, as said, these are nitpicks.

Anyway, these are nitpicks, mainly. The idea is that I hoped this test would have some samples against some popular frameworks. I have seen https://github.com/arey/java-object-mapper-benchmark, though. It's just that your results rely on those tests and I am not sure about their correctness (without understanding them, those are meaningless numbers: look how the benchmark presents that "direct" mapping is slower than "map-struct")

There's that tricky question :) I wanted to do that in #23 but decided against it as I would have to maintain other mapper implementations in my repository and people would complain about non-idiomatic / non-best-performing usage of other frameworks. That's why I have linked to the repository you linked to in the PerformanceBenchmarkTest. I have highlighted the "weird" performance issues here but it seems like the author did not want to update the numbers. Datus numbers have improved by 50% too compared to the last run so... there's that :)

I wonder if I should provide a PR against some data mappings libraries inside (or a separate project/child from datus)?

To put it bluntly: I still don't want to maintain 3rd-party mapper implementations for 8-XX frameworks for the aforementioned reasons. I think it would be best to go to arey's repo and open issues etc.

I see. Thx for the follow-up. I think it's safe we close this and I understand the reasonings.

Thank you for your issue

@wind57 wind57 closed this as completed Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants