Added IgnoreDefaultEqualsAndToString #566

pditommaso · 2017-06-25T18:30:32Z

This annotation allows custom Collection and Map objects to bypass the default Groovy format and equality methods ie. toString and equals.

…Groovy equals and toString methods for Map and Collection objects

jwagenleitner · 2017-07-05T03:53:14Z

Just a minor comment, for checking whether the annotation exists I think isAnnotationPresent(IgnoreDefaultEqualsAndToString.class) reads a little better than getAnnotation(IgnoreDefaultEqualsAndToString.class) != null.

pditommaso · 2017-07-05T07:03:39Z

👍

paulk-asert · 2017-07-24T00:27:47Z

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.

pditommaso · 2017-07-24T13:01:01Z

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.

melix · 2018-02-06T10:19:50Z

src/main/org/codehaus/groovy/runtime/DefaultGroovyMethods.java

@@ -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 ) {


-1 to this until we have a better idea on the impact on performance. Calling getClass().getAnnotation(...) is very expensive.

Could you suggest a possible metrics/benchmark ?

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.

blackdrag · 2018-02-06T11:28:42Z

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)

pditommaso · 2018-02-06T17:18:33Z

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?

pditommaso · 2018-02-06T18:53:57Z

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:

iteration	w/o annotation (ms)	with annotation (ms)	delta (ms)	overhead
10	239	264	25	10.46%
100	206	216	10	4.85%
1000	339	365	26	7.67%
10000	1430	1509	79	5.52%
100000	11370	11434	64	0.56%

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?

blackdrag · 2018-02-07T16:44:50Z

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

pditommaso · 2018-02-07T16:55:04Z

I've never used jmh, but I can try to implement the benchmark with it if required.

You did something for toString()

Not really, I've just copy&pasted the same code in the InvokerHelper.formatCollection method (adjusting the missing constants/methods to equivalent ones)

blackdrag · 2018-02-08T08:14:41Z

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

pditommaso · 2018-02-08T08:15:56Z

👍

…

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

melix · 2018-02-08T08:18:33Z

Testing 2 empty lists is not enough. We should test lists with arbitrary objects, as otherwise the VM will perform optimistic branch optimizations.

blackdrag · 2018-02-08T08:55:57Z

I would not count on that optimization anymore with the reflective call in there

pditommaso · 2018-02-08T09:05:27Z

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:

no extra reflective overhead
no need for a new annotation in the groovy code
user provided strategy could rely on more performant implementation eg (use of marker interface instead of annotation)
would be easier to remove in a future groovy release if this feature would be implemented/handled at core level as a language feature.

What do you think about that?

pditommaso · 2018-02-14T21:38:41Z

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:

# Run complete. Total time: 00:17:43

Benchmark                       (count)  Mode  Cnt     Score     Error  Units
ListBench.equalsDefaultGroovy         1  avgt   15     0.003 ±   0.001  ms/op
ListBench.equalsDefaultGroovy        10  avgt   15     0.034 ±   0.001  ms/op
ListBench.equalsDefaultGroovy       100  avgt   15     0.332 ±   0.003  ms/op
ListBench.equalsDefaultGroovy      1000  avgt   15     3.355 ±   0.027  ms/op
ListBench.equalsDefaultGroovy   1000000  avgt   15  3412.212 ±  36.335  ms/op
ListBench.equalsWithAnnotation        1  avgt   15     0.004 ±   0.001  ms/op
ListBench.equalsWithAnnotation       10  avgt   15     0.035 ±   0.001  ms/op
ListBench.equalsWithAnnotation      100  avgt   15     0.336 ±   0.003  ms/op
ListBench.equalsWithAnnotation     1000  avgt   15     3.350 ±   0.029  ms/op
ListBench.equalsWithAnnotation  1000000  avgt   15  3534.392 ± 246.034  ms/op
ListBench.equalsWithInterface         1  avgt   15     0.003 ±   0.001  ms/op
ListBench.equalsWithInterface        10  avgt   15     0.035 ±   0.001  ms/op
ListBench.equalsWithInterface       100  avgt   15     0.341 ±   0.009  ms/op
ListBench.equalsWithInterface      1000  avgt   15     3.359 ±   0.057  ms/op
ListBench.equalsWithInterface   1000000  avgt   15  3510.889 ± 194.890  ms/op

not sure to understand how to read it.

pditommaso · 2018-02-15T10:27:16Z

I've refined a bit the benchmark adding the a throughput mode that looks more informative. The benchmark test is here.

Benchmark                        (size)   Mode  Cnt        Score        Error   Units
ListBench.equalsDefaultGroovy         1  thrpt   15  2320508.042 ±  84277.415  ops/ms
ListBench.equalsDefaultGroovy        10  thrpt   15  2329172.399 ±  69070.211  ops/ms
ListBench.equalsDefaultGroovy       100  thrpt   15  2354738.681 ±  64525.890  ops/ms
ListBench.equalsDefaultGroovy      1000  thrpt   15  2358917.549 ±  46171.873  ops/ms
ListBench.equalsDefaultGroovy   1000000  thrpt   15  2377102.274 ±  48227.814  ops/ms
ListBench.equalsWithAnnotation        1  thrpt   15  2213530.048 ±  93671.364  ops/ms
ListBench.equalsWithAnnotation       10  thrpt   15  2277729.772 ±  89214.824  ops/ms
ListBench.equalsWithAnnotation      100  thrpt   15  2256582.572 ±  85712.999  ops/ms
ListBench.equalsWithAnnotation     1000  thrpt   15  2266409.619 ±  45408.683  ops/ms
ListBench.equalsWithAnnotation  1000000  thrpt   15  2197481.973 ± 127202.807  ops/ms
ListBench.equalsWithInterface         1  thrpt   15  2155416.804 ±  80496.840  ops/ms
ListBench.equalsWithInterface        10  thrpt   15  2061873.464 ± 211658.982  ops/ms
ListBench.equalsWithInterface       100  thrpt   15  2106467.218 ±  89659.046  ops/ms
ListBench.equalsWithInterface      1000  thrpt   15  2219947.677 ±  87623.149  ops/ms
ListBench.equalsWithInterface   1000000  thrpt   15  2092834.055 ± 165819.668  ops/ms
ListBench.equalsDefaultGroovy         1   avgt   15       ≈ 10⁻⁶                ms/op
ListBench.equalsDefaultGroovy        10   avgt   15       ≈ 10⁻⁶                ms/op
ListBench.equalsDefaultGroovy       100   avgt   15       ≈ 10⁻⁶                ms/op
ListBench.equalsDefaultGroovy      1000   avgt   15       ≈ 10⁻⁶                ms/op
ListBench.equalsDefaultGroovy   1000000   avgt   15       ≈ 10⁻⁶                ms/op
ListBench.equalsWithAnnotation        1   avgt   15       ≈ 10⁻⁶                ms/op
ListBench.equalsWithAnnotation       10   avgt   15       ≈ 10⁻⁶                ms/op
ListBench.equalsWithAnnotation      100   avgt   15       ≈ 10⁻⁶                ms/op
ListBench.equalsWithAnnotation     1000   avgt   15       ≈ 10⁻⁶                ms/op
ListBench.equalsWithAnnotation  1000000   avgt   15       ≈ 10⁻⁶                ms/op
ListBench.equalsWithInterface         1   avgt   15       ≈ 10⁻⁶                ms/op
ListBench.equalsWithInterface        10   avgt   15       ≈ 10⁻⁶                ms/op
ListBench.equalsWithInterface       100   avgt   15       ≈ 10⁻⁶                ms/op
ListBench.equalsWithInterface      1000   avgt   15       ≈ 10⁻⁶                ms/op
ListBench.equalsWithInterface   1000000   avgt   15       ≈ 10⁻⁶                ms/op

The complete commit is at this link pditommaso@7a25dfc.

pditommaso · 2018-03-07T08:32:48Z

Hi, what about the benchmark result? any chance to move this PR forward?

Added @IgnoreDefaultEqualsAndToString annotation that allows by-pass …

0b0682a

…Groovy equals and toString methods for Map and Collection objects

pditommaso force-pushed the ignore-default-groovy-methods branch from 8a9f43b to 0b0682a Compare June 26, 2017 01:03

melix reviewed Feb 6, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added IgnoreDefaultEqualsAndToString #566

Added IgnoreDefaultEqualsAndToString #566

pditommaso commented Jun 25, 2017

jwagenleitner commented Jul 5, 2017

pditommaso commented Jul 5, 2017

paulk-asert commented Jul 24, 2017 •

edited

pditommaso commented Jul 24, 2017

melix Feb 6, 2018

pditommaso Feb 6, 2018

melix Feb 6, 2018

blackdrag commented Feb 6, 2018

pditommaso commented Feb 6, 2018

pditommaso commented Feb 6, 2018 •

edited

blackdrag commented Feb 7, 2018

pditommaso commented Feb 7, 2018 •

edited

blackdrag commented Feb 8, 2018 •

edited

pditommaso commented Feb 8, 2018 via email

melix commented Feb 8, 2018

blackdrag commented Feb 8, 2018

pditommaso commented Feb 8, 2018 •

edited

pditommaso commented Feb 14, 2018 •

edited

pditommaso commented Feb 15, 2018

pditommaso commented Mar 7, 2018

Added IgnoreDefaultEqualsAndToString #566

Are you sure you want to change the base?

Added IgnoreDefaultEqualsAndToString #566

Conversation

pditommaso commented Jun 25, 2017

jwagenleitner commented Jul 5, 2017

pditommaso commented Jul 5, 2017

paulk-asert commented Jul 24, 2017 • edited

pditommaso commented Jul 24, 2017

melix Feb 6, 2018

Choose a reason for hiding this comment

pditommaso Feb 6, 2018

Choose a reason for hiding this comment

melix Feb 6, 2018

Choose a reason for hiding this comment

blackdrag commented Feb 6, 2018

pditommaso commented Feb 6, 2018

pditommaso commented Feb 6, 2018 • edited

blackdrag commented Feb 7, 2018

pditommaso commented Feb 7, 2018 • edited

blackdrag commented Feb 8, 2018 • edited

pditommaso commented Feb 8, 2018 via email

melix commented Feb 8, 2018

blackdrag commented Feb 8, 2018

pditommaso commented Feb 8, 2018 • edited

pditommaso commented Feb 14, 2018 • edited

pditommaso commented Feb 15, 2018

pditommaso commented Mar 7, 2018

paulk-asert commented Jul 24, 2017 •

edited

pditommaso commented Feb 6, 2018 •

edited

pditommaso commented Feb 7, 2018 •

edited

blackdrag commented Feb 8, 2018 •

edited

pditommaso commented Feb 8, 2018 •

edited

pditommaso commented Feb 14, 2018 •

edited