RFR: 8261447: MethodInvocationCounters frequently run into overflow [v8]

[Lutz Schmidt]

On Tue, 2 Mar 2021 20:46:01 GMT, Igor Veresov <iveresov at openjdk.org> wrote:

>> src/hotspot/share/runtime/java.cpp line 100:
>> 
>>> 98: int compare_methods(Method** a, Method** b) {
>>> 99:   return (int32_t)(((uint32_t)(*b)->invocation_count() + (*b)->compiled_invocation_count())
>>> 100:                  - ((uint32_t)(*a)->invocation_count() + (*a)->compiled_invocation_count()));
>> 
>> Is this correct? The arithmetic look to be: (int32_t) (uint64_t - uint64_t). If the 64 values inside don't fit in 32, you'll get a negative number which would break the sorting logic.
>
> I see that you've fixed the types since the last comment, but it think it's still broken (and has been before).
> How about:
> int64_t diff =  ((*b)->compiled_invocation_count() - (*a)->compiled_invocation_count()) + ((*b)->invocation_count() - (*a)->invocation_count());
> if (diff > 0) return 1;
> else if (diff < 0) return -1;
> else return 0;
> It's kind of hacky too, because it assumes that compiled_invocation_count() are positive and didn't overflow. But at least we'd get rid of a possible overflow during summation. What do you think?

Right. As soon as there is overflow, the original formula doesn't do the trick either.
We can fix it as long as either (unsigned int)invocation_count() does not wrap around from 2^32-1 to 0. The entire expression is calculated as int64_t, protecting us from overflow for the next few years. If we then calculate the return value as you propose, we are good.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2511