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

👍

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.

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?

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.

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)

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

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:

1. no extra reflective overhead
2. no need for a new annotation in the groovy code
3. user provided strategy could rely on more performant implementation eg (use of marker interface instead of annotation)
4. 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?

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.

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−6 ms/op
ListBench.equalsDefaultGroovy 10 avgt 15 ≈ 10−6 ms/op
ListBench.equalsDefaultGroovy 100 avgt 15 ≈ 10−6 ms/op
ListBench.equalsDefaultGroovy 1000 avgt 15 ≈ 10−6 ms/op
ListBench.equalsDefaultGroovy 1000000 avgt 15 ≈ 10−6 ms/op
ListBench.equalsWithAnnotation 1 avgt 15 ≈ 10−6 ms/op
ListBench.equalsWithAnnotation 10 avgt 15 ≈ 10−6 ms/op
ListBench.equalsWithAnnotation 100 avgt 15 ≈ 10−6 ms/op
ListBench.equalsWithAnnotation 1000 avgt 15 ≈ 10−6 ms/op
ListBench.equalsWithAnnotation 1000000 avgt 15 ≈ 10−6 ms/op
ListBench.equalsWithInterface 1 avgt 15 ≈ 10−6 ms/op
ListBench.equalsWithInterface 10 avgt 15 ≈ 10−6 ms/op
ListBench.equalsWithInterface 100 avgt 15 ≈ 10−6 ms/op
ListBench.equalsWithInterface 1000 avgt 15 ≈ 10−6 ms/op
ListBench.equalsWithInterface 1000000 avgt 15 ≈ 10−6 ms/op

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

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