API: Remove overflow checks in DefaultCounter causing performance issues

[aokolnychyi]

This PR removes the overflow check in DefaultCounter, which causes performance issues when the counter is being accessed from multiple threads.

Prior to this PR:

Benchmark                                               Mode  Cnt  Score   Error  Units
CountersBenchmark.defaultCounterMultipleThreads           ss   25  4.912 ± 0.110   s/op
CountersBenchmark.defaultCounterMultipleThreads:·async    ss         NaN            ---
CountersBenchmark.defaultCounterSingleThread              ss   25  1.136 ± 0.014   s/op
CountersBenchmark.defaultCounterSingleThread:·async       ss         NaN            ---

After this PR:

Benchmark                                               Mode  Cnt  Score   Error  Units
CountersBenchmark.defaultCounterMultipleThreads           ss   25  0.168 ± 0.022   s/op
CountersBenchmark.defaultCounterMultipleThreads:·async    ss         NaN            ---
CountersBenchmark.defaultCounterSingleThread              ss   25  1.134 ± 0.011   s/op
CountersBenchmark.defaultCounterSingleThread:·async       ss         NaN            ---

[aokolnychyi]

Calling longValue() on LongAdd is expensive as it performs an aggregation over cells. It basically removes all the benefits of LongAdd that is supposed to scale well in case of contention. We call it every time the counter is being modified. I am also not sure having an overflow (which is unlikely given that we are using longs) should fail queries, for instance. These are metrics and they should add as little overhead as possible.

[paliwalashish]

ctx.scanMetrics().totalFileSizeInBytes().increment(dataFile.fileSizeInBytes()) is the one that had the potential. With long as datatype, it's like 8192 Petabyte data read, so doesn't seem to be an issue as of now :)
Just curious, since it's a public class and if someone is using it, should we mention this in javadoc?

[aokolnychyi]

since it's a public class and if someone is using it, should we mention this in javadoc?

That's a good idea, will do if we decide to proceed with the change.

[aokolnychyi]

@nastra, could you take a look?

[aokolnychyi]

These counters have been deprecated previously.

[advancedxy]

For IntCounter, I believe that we can still add a cheap overflow check at the value() method.

    public Integer value() {
      long sum = counter.longValue();
      if (sum > Integer.MaxValue) {
        ...
      }
      return (int) sum
    }

[aokolnychyi]

It is the old deprecated counter API that is not used anymore but no harm adding it. Let me do it.

[nastra]

Thanks for finding this @aokolnychyi. I think we're ok not checking for an overflow. I've checked other frameworks (dropwizard/micrometer) and their counter implementations also don't seem to be doing overflow checks, so this change seems reasonable to me

[paliwalashish]

Should we add in DefaultCounter that it doesn't check for overflow? Not sure if it's too much.

[aokolnychyi]

I see that DefaultCounter has no other method-level docs so I'd probably just stick to a note in the public interface for now.

[paliwalashish]

+1 LGTM

Thanks @aokolnychyi

[aokolnychyi]

Thank you, @mystic-lama @advancedxy @nastra!