Added IgnoreDefaultEqualsAndToString #566
Conversation
…Groovy equals and toString methods for Map and Collection objects
pditommaso
force-pushed
the
ignore-default-groovy-methods
branch
from
8a9f43b to
0b0682a
Compare
|
Just a minor comment, for checking whether the annotation exists I think |
|
👍 |
|
I totally understand the need for this but it does feel a bit like a hack to fix a slightly imperfect 'feature' of Groovy. I'd prefer to fix the underlying issue for upcoming releases of Groovy - and make a more generic way for Groovy to determine when to apply special formatting. But having said that, perhaps a short-term fix is needed for older versions anyway, so I am probably -0 on the concept as a whole but have no issue at all with your implementation as far as it goes as a short-term fix. And I realise that much time has gone by with the current situation without any kind of workaround. |
|
I completely agree that a more generic mechanism would be better but that would require to wait for Groovy 3.0 (or later) that realistically wouldn't be released before 2019. For me it is perfectly fine to have this annotation declared as a temporary workaround that won't be supported in a future major release. |
| @@ -12026,6 +12026,9 @@ public static boolean equals(List left, List right) { | |||
| if (left == right) { | |||
| return true; | |||
| } | |||
| if( left.getClass().getAnnotation(IgnoreDefaultEqualsAndToString.class)!=null && right.getClass().getAnnotation(IgnoreDefaultEqualsAndToString.class)!=null ) { | |||
There was a problem hiding this comment.
-1 to this until we have a better idea on the impact on performance. Calling getClass().getAnnotation(...) is very expensive.
There was a problem hiding this comment.
Could you suggest a possible metrics/benchmark ?
There was a problem hiding this comment.
I have nothing particular in mind at this point, but it involves this method being called with different kind of objects, and comparing with the baseline without the change.
|
my suggestion: make a JMH based microbenchmark in which you compare two empty lists. One time with the old implementation and one time with the new implementation. This benchmark can be done fully in Java and just copy&paste the methods in. This benchmark should hit the annotation path every time. And for comparison I would also add different sized lists (1, 10, 100, 1000, 1000000)... for example a List of Integer. They can both have the same content, just should not be the same reference. That would then also give an idea of the overhead in relation to the number of elements (which will be decreasing of course) |
|
Not sure to understand with the length of the list should have an impact on the annotation overhead. That cost it's constant, isn't it? |
|
OK, I've some numbers. I've made a poor man currentTimeMillis delta for different iterations and using a CompileStatic annotation. The results are these:
The source code is here. The launcher script is this. It was compiled and executed once in the master branch and then in the proposed change branch. Thoughts? |
|
that cost is constant yes I know. But that constant cost has to be seen in comparison with actual lists. But it is not the benchmark I would like to see actually. You did something for toString(), not for compare. Also you did not do a warmup, which is why I suggested using jmh |
|
I've never used jmh, but I can try to implement the benchmark with it if required.
Not really, I've just copy&pasted the same code in the |
|
I think you misunderstood me a bit. toString is not a real problem. The performance of equals is though. That is why we need a benchmark for that |
|
👍
…On Feb 8, 2018 09:14, "Jochen Theodorou" ***@***.***> wrote:
I think you misunderstood me a bit. toString is not a real problem. The
performance of equals is though. That is why we need a test for that
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#566 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAx3SDgjhTtQPFmY50TsBwDYUDx59gHgks5tSqz5gaJpZM4OErmR>
.
|
|
Testing 2 empty lists is not enough. We should test lists with arbitrary objects, as otherwise the VM will perform optimistic branch optimizations. |
|
I would not count on that optimization anymore with the reflective call in there |
|
Indeed. I'm starting to become e bit skeptic about this proposal. Surely it will add some overhead and since this is a critical code I agree that it should be avoid as much at possible. For this reason I was thinking about a possible alternative. What about instead to make the default toString and equals strategy configurable via service loader (or even a system property). This would have several benefits:
What do you think about that? |
|
Some progress here, I've made a JHM benchmark using the current groovy master. The benchmark class is this, comparing default groovy equals, one using the proposed annotation and another using a marker interface (all changes here pditommaso@d37d1ae). These are the results: not sure to understand how to read it. |
|
I've refined a bit the benchmark adding the a throughput mode that looks more informative. The benchmark test is here. The complete commit is at this link pditommaso@7a25dfc. |
|
Hi, what about the benchmark result? any chance to move this PR forward? |
This annotation allows custom
CollectionandMapobjects to bypass the default Groovy format and equality methods ie.toStringandequals.