RFR: JDK-8261447: MethodInvocationCounters frequently run into overflow

classic Classic list List threaded Threaded
39 messages Options
12
Reply | Threaded
Open this post in threaded view
|

RFR: JDK-8261447: MethodInvocationCounters frequently run into overflow

Lutz Schmidt
Dear community,
may I please request reviews for this fix, improving the usefulness of method invocation counters.
- aggregation counters are retyped as uint64_t, shifting the overflow probability way out (185 days in case of a 1 GHz counter update frequency).
- counters for individual methods are interpreted as (unsigned int), in contrast to their declaration as int. This gives us a factor of two before the counters overflow.
- as a special case, "compiled_invocation_counter" is retyped as long, because it has a higher update frequency than other counters.
- before/after sample output is attached to the bug description.

Thank you!
Lutz

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

Commit messages:
 - JDK-8261447: MethodInvocationCounters frequently run into overflow

Changes: https://git.openjdk.java.net/jdk/pull/2511/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2511&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261447
  Stats: 184 lines in 8 files changed: 89 ins; 3 del; 92 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2511.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2511/head:pull/2511

PR: https://git.openjdk.java.net/jdk/pull/2511
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261447: MethodInvocationCounters frequently run into overflow

Tobias Hartmann-3
On Wed, 10 Feb 2021 16:28:29 GMT, Lutz Schmidt <[hidden email]> wrote:

> Dear community,
> may I please request reviews for this fix, improving the usefulness of method invocation counters.
> - aggregation counters are retyped as uint64_t, shifting the overflow probability way out (185 days in case of a 1 GHz counter update frequency).
> - counters for individual methods are interpreted as (unsigned int), in contrast to their declaration as int. This gives us a factor of two before the counters overflow.
> - as a special case, "compiled_invocation_counter" is retyped as long, because it has a higher update frequency than other counters.
> - before/after sample output is attached to the bug description.
>
> Thank you!
> Lutz

Changes requested by thartmann (Reviewer).

src/hotspot/share/oops/method.hpp line 728:

> 726: #ifndef PRODUCT
> 727:   static ByteSize compiled_invocation_counter_offset() { return byte_offset_of(Method, _compiled_invocation_count); }
> 728:   static ByteSize compiled_invocation_counter_offset64() { return byte_offset_of(Method, _compiled_invocation_count64); }

`compiled_invocation_counter_offset()` looks unused.

src/hotspot/share/oops/method.hpp line 459:

> 457: #else
> 458:   // for PrintMethodData in a product build
> 459:   int      compiled_invocation_count() const    { return 0; }

`compiled_invocation_count()` looks unused.

src/hotspot/share/oops/method.hpp line 106:

> 104:     struct {
> 105:       int     _compiled_invocation_count;  // Number of nmethod invocations so far (for perf. debugging)
> 106:                                            // Must preserve this as int. Is used outside the jdk by SA.

Why not update the SA code to access 64 bit?

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

PR: https://git.openjdk.java.net/jdk/pull/2511
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261447: MethodInvocationCounters frequently run into overflow

Lutz Schmidt
On Thu, 11 Feb 2021 08:28:47 GMT, Tobias Hartmann <[hidden email]> wrote:

>> Dear community,
>> may I please request reviews for this fix, improving the usefulness of method invocation counters.
>> - aggregation counters are retyped as uint64_t, shifting the overflow probability way out (185 days in case of a 1 GHz counter update frequency).
>> - counters for individual methods are interpreted as (unsigned int), in contrast to their declaration as int. This gives us a factor of two before the counters overflow.
>> - as a special case, "compiled_invocation_counter" is retyped as long, because it has a higher update frequency than other counters.
>> - before/after sample output is attached to the bug description.
>>
>> Thank you!
>> Lutz
>
> src/hotspot/share/oops/method.hpp line 728:
>
>> 726: #ifndef PRODUCT
>> 727:   static ByteSize compiled_invocation_counter_offset() { return byte_offset_of(Method, _compiled_invocation_count); }
>> 728:   static ByteSize compiled_invocation_counter_offset64() { return byte_offset_of(Method, _compiled_invocation_count64); }
>
> `compiled_invocation_counter_offset()` looks unused.

Correct. Removed.

> src/hotspot/share/oops/method.hpp line 459:
>
>> 457: #else
>> 458:   // for PrintMethodData in a product build
>> 459:   int      compiled_invocation_count() const    { return 0; }
>
> `compiled_invocation_count()` looks unused.

compiled_invocation_count() was used in compare_methods() and collect_invoked_methods(). I have converted those call sites to compiled_invocation_count64().

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

PR: https://git.openjdk.java.net/jdk/pull/2511
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261447: MethodInvocationCounters frequently run into overflow

Lutz Schmidt
In reply to this post by Tobias Hartmann-3
On Thu, 11 Feb 2021 08:33:40 GMT, Tobias Hartmann <[hidden email]> wrote:

>> Dear community,
>> may I please request reviews for this fix, improving the usefulness of method invocation counters.
>> - aggregation counters are retyped as uint64_t, shifting the overflow probability way out (185 days in case of a 1 GHz counter update frequency).
>> - counters for individual methods are interpreted as (unsigned int), in contrast to their declaration as int. This gives us a factor of two before the counters overflow.
>> - as a special case, "compiled_invocation_counter" is retyped as long, because it has a higher update frequency than other counters.
>> - before/after sample output is attached to the bug description.
>>
>> Thank you!
>> Lutz
>
> src/hotspot/share/oops/method.hpp line 106:
>
>> 104:     struct {
>> 105:       int     _compiled_invocation_count;  // Number of nmethod invocations so far (for perf. debugging)
>> 106:                                            // Must preserve this as int. Is used outside the jdk by SA.
>
> Why not update the SA code to access 64 bit?

Two reasons: scope limitation and lack of expertise in SA code. Don't we need the 32bit decl anyway because it is contained in vmstructs.cpp?

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

PR: https://git.openjdk.java.net/jdk/pull/2511
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261447: MethodInvocationCounters frequently run into overflow

Lutz Schmidt
In reply to this post by Tobias Hartmann-3
On Thu, 11 Feb 2021 08:34:09 GMT, Tobias Hartmann <[hidden email]> wrote:

>> Dear community,
>> may I please request reviews for this fix, improving the usefulness of method invocation counters.
>> - aggregation counters are retyped as uint64_t, shifting the overflow probability way out (185 days in case of a 1 GHz counter update frequency).
>> - counters for individual methods are interpreted as (unsigned int), in contrast to their declaration as int. This gives us a factor of two before the counters overflow.
>> - as a special case, "compiled_invocation_counter" is retyped as long, because it has a higher update frequency than other counters.
>> - before/after sample output is attached to the bug description.
>>
>> Thank you!
>> Lutz
>
> Changes requested by thartmann (Reviewer).

Thank you Tobias for having a look. I'll post an update asap.

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

PR: https://git.openjdk.java.net/jdk/pull/2511
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261447: MethodInvocationCounters frequently run into overflow [v2]

Lutz Schmidt
In reply to this post by Lutz Schmidt
> Dear community,
> may I please request reviews for this fix, improving the usefulness of method invocation counters.
> - aggregation counters are retyped as uint64_t, shifting the overflow probability way out (185 days in case of a 1 GHz counter update frequency).
> - counters for individual methods are interpreted as (unsigned int), in contrast to their declaration as int. This gives us a factor of two before the counters overflow.
> - as a special case, "compiled_invocation_counter" is retyped as long, because it has a higher update frequency than other counters.
> - before/after sample output is attached to the bug description.
>
> Thank you!
> Lutz

Lutz Schmidt has updated the pull request incrementally with one additional commit since the last revision:

  8261447: requested changes by TobiHartmann

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2511/files
  - new: https://git.openjdk.java.net/jdk/pull/2511/files/868ff38b..bfd60a3c

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2511&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2511&range=00-01

  Stats: 8 lines in 2 files changed: 1 ins; 3 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2511.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2511/head:pull/2511

PR: https://git.openjdk.java.net/jdk/pull/2511
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261447: MethodInvocationCounters frequently run into overflow [v2]

Vladimir Kozlov-2
In reply to this post by Lutz Schmidt
On Thu, 11 Feb 2021 13:39:15 GMT, Lutz Schmidt <[hidden email]> wrote:

>> Changes requested by thartmann (Reviewer).
>
> Thank you Tobias for having a look. I'll post an update asap.

@veresov please review these changes

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

PR: https://git.openjdk.java.net/jdk/pull/2511
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261447: MethodInvocationCounters frequently run into overflow [v2]

Igor Veresov-3
On Thu, 11 Feb 2021 18:22:38 GMT, Vladimir Kozlov <[hidden email]> wrote:

>> Thank you Tobias for having a look. I'll post an update asap.
>
> @veresov please review these changes

I don't really like these .*64 suffixes. Can we just make the counter 64 bit, update the SA as Tobias suggested and keep the existing method names? Or is there a reason for doing this that eludes me?

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

PR: https://git.openjdk.java.net/jdk/pull/2511
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261447: MethodInvocationCounters frequently run into overflow [v2]

Lutz Schmidt
On Thu, 11 Feb 2021 20:45:30 GMT, Igor Veresov <[hidden email]> wrote:

>> @veresov please review these changes
>
> I don't really like these .*64 suffixes. Can we just make the counter 64 bit, update the SA as Tobias suggested and keep the existing method names? Or is there a reason for doing this that eludes me?

I introduced the *64 suffixes to not break anything that still uses the old calls. As old uses disappear step by step, I'm more than happy to remove the suffixes. I will have a look into SA and try to make it 64bit counter ready. There may be no new version before the weekend is over.

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

PR: https://git.openjdk.java.net/jdk/pull/2511
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261447: MethodInvocationCounters frequently run into overflow [v2]

Lutz Schmidt
On Fri, 12 Feb 2021 08:29:37 GMT, Lutz Schmidt <[hidden email]> wrote:

>> I don't really like these .*64 suffixes. Can we just make the counter 64 bit, update the SA as Tobias suggested and keep the existing method names? Or is there a reason for doing this that eludes me?
>
> I introduced the *64 suffixes to not break anything that still uses the old calls. As old uses disappear step by step, I'm more than happy to remove the suffixes. I will have a look into SA and try to make it 64bit counter ready. There may be no new version before the weekend is over.

This is a request for help. Could someone with SA knowledge please check if my assumption is correct?

In hotspot code, the field Method::_compiled_invocation_count is annotated with a comment that it is used by SA. The field is also exposed via vmStructs.cpp to enable such use. I have scanned SA code in OpenJDK11 and OpenJDK head but found no evidence that this particular field is accessed. Is this finding/assumption correct?

If so, I could just stop exposing the field, making my life easier. Thanks!

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

PR: https://git.openjdk.java.net/jdk/pull/2511
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261447: MethodInvocationCounters frequently run into overflow [v4]

Lutz Schmidt
In reply to this post by Lutz Schmidt
> Dear community,
> may I please request reviews for this fix, improving the usefulness of method invocation counters.
> - aggregation counters are retyped as uint64_t, shifting the overflow probability way out (185 days in case of a 1 GHz counter update frequency).
> - counters for individual methods are interpreted as (unsigned int), in contrast to their declaration as int. This gives us a factor of two before the counters overflow.
> - as a special case, "compiled_invocation_counter" is retyped as long, because it has a higher update frequency than other counters.
> - before/after sample output is attached to the bug description.
>
> Thank you!
> Lutz

Lutz Schmidt has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits:

 - 8261447: requested changes by TobiHartmann
 - JDK-8261447: MethodInvocationCounters frequently run into overflow
 - expand remaining counters to 64-bit, remove 64 duffix
 - 8261447: requested changes by TobiHartmann
 - JDK-8261447: MethodInvocationCounters frequently run into overflow

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

Changes: https://git.openjdk.java.net/jdk/pull/2511/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2511&range=03
  Stats: 5969 lines in 92 files changed: 5856 ins; 4 del; 109 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2511.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2511/head:pull/2511

PR: https://git.openjdk.java.net/jdk/pull/2511
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261447: MethodInvocationCounters frequently run into overflow [v5]

Lutz Schmidt
In reply to this post by Lutz Schmidt
> Dear community,
> may I please request reviews for this fix, improving the usefulness of method invocation counters.
> - aggregation counters are retyped as uint64_t, shifting the overflow probability way out (185 days in case of a 1 GHz counter update frequency).
> - counters for individual methods are interpreted as (unsigned int), in contrast to their declaration as int. This gives us a factor of two before the counters overflow.
> - as a special case, "compiled_invocation_counter" is retyped as long, because it has a higher update frequency than other counters.
> - before/after sample output is attached to the bug description.
>
> Thank you!
> Lutz

Lutz Schmidt has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR.

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2511/files
  - new: https://git.openjdk.java.net/jdk/pull/2511/files/273d55c2..0a99ee4e

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2511&range=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2511&range=03-04

  Stats: 5780 lines in 79 files changed: 0 ins; 5779 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2511.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2511/head:pull/2511

PR: https://git.openjdk.java.net/jdk/pull/2511
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261447: MethodInvocationCounters frequently run into overflow [v6]

Lutz Schmidt
In reply to this post by Lutz Schmidt
> Dear community,
> may I please request reviews for this fix, improving the usefulness of method invocation counters.
> - aggregation counters are retyped as uint64_t, shifting the overflow probability way out (185 days in case of a 1 GHz counter update frequency).
> - counters for individual methods are interpreted as (unsigned int), in contrast to their declaration as int. This gives us a factor of two before the counters overflow.
> - as a special case, "compiled_invocation_counter" is retyped as long, because it has a higher update frequency than other counters.
> - before/after sample output is attached to the bug description.
>
> Thank you!
> Lutz

Lutz Schmidt has updated the pull request incrementally with two additional commits since the last revision:

 - no incrementq for x86_32
 - cleaning up the remaining mess

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2511/files
  - new: https://git.openjdk.java.net/jdk/pull/2511/files/0a99ee4e..faab64b0

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2511&range=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2511&range=04-05

  Stats: 30 lines in 5 files changed: 0 ins; 15 del; 15 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2511.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2511/head:pull/2511

PR: https://git.openjdk.java.net/jdk/pull/2511
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261447: MethodInvocationCounters frequently run into overflow [v7]

Lutz Schmidt
In reply to this post by Lutz Schmidt
On Mon, 15 Feb 2021 18:21:53 GMT, Lutz Schmidt <[hidden email]> wrote:

>> This is a request for help. Could someone with SA knowledge please check if my assumption is correct?
>>
>> In hotspot code, the field Method::_compiled_invocation_count is annotated with a comment that it is used by SA. The field is also exposed via vmStructs.cpp to enable such use. I have scanned SA code in OpenJDK11 and OpenJDK head but found no evidence that this particular field is accessed. Is this finding/assumption correct?
>>
>> If so, I could just stop exposing the field, making my life easier. Thanks!
>
> Looks like I have completely messed up my pull request. Please disregard for now. I'm trying to find a way how to clean up. Maybe I'll just start over.

OK, my pull request is back in a reviewable state. Here is what changed:

1) Honouring review comments from @TobiHartmann and @veresov
Trusting my own code research, I removed _compiled_invocation_count from vmStructs.cpp. Builds are ok and all tests we run in-house (including the jtreg suite) did not show any issue. The updated pull request has _compiled_invocation_count widened to 64-bit and all those *64 suffixes are removed.

2) Dealing with counter updates in {v|i}table stubs
While waiting for a response from SA experts, I took the time and had a closer look at the last remaining 32-bit counter (_nof_megamorphic_calls). It turned out the required changes to code generation were trivial. So I took the opportunity and made it a 64-bit counter. Call stats look even nicer now!

In summary: All global invocation counters are 64-bit now. From those counters that register method-individual calls, only _compiled_invocation_count and _nof_megamorphic_calls were widened to 64-bit. The three remaining method-individual counters (invocation_count, interpreter_invocation_count. and backedge_count) remain untouched.

I appreciate your feedback!

Here is how stats look like now:

Invocations summary for 28214 methods:
         41055191904 (100%)  total
          4818528940 (11.7%) |- interpreted
         36236662964 (88.3%) |- compiled
          9065026571 (22.1%) |- special methods (interpreted and compiled)
           607128840 ( 1.5%)    |- synchronized
          2107652419 ( 5.1%)    |- final
          6347934023 (15.5%)    |- static
             1122857 ( 0.0%)    |- native
             1188432 ( 0.0%)    |- accessor

Calls from compiled code:
         27011733837 (100%)  total non-inlined
         14500960686 (53.7%) |- virtual calls
        124325246564 ( 857%) |  |- inlined
                   0 (   0%) |  |- optimized
          8890453008 (  61%) |  |- monomorphic
          5610507678 (  39%) |  |- megamorphic
          4529160753 (16.8%) |- interface calls
          8905052200 ( 197%) |  |- inlined
                   0 (   0%) |  |- optimized
          4529160753 ( 100%) |  |- monomorphic
                   0 (   0%) |  |- megamorphic
          7981612398 (29.5%) |- static/special calls
         73886243527 ( 926%) |  |- inlined

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

PR: https://git.openjdk.java.net/jdk/pull/2511
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261447: MethodInvocationCounters frequently run into overflow [v7]

David Holmes-2
In reply to this post by Lutz Schmidt
On Wed, 17 Feb 2021 18:22:01 GMT, Lutz Schmidt <[hidden email]> wrote:

>> Dear community,
>> may I please request reviews for this fix, improving the usefulness of method invocation counters.
>> - aggregation counters are retyped as uint64_t, shifting the overflow probability way out (185 days in case of a 1 GHz counter update frequency).
>> - counters for individual methods are interpreted as (unsigned int), in contrast to their declaration as int. This gives us a factor of two before the counters overflow.
>> - as a special case, "compiled_invocation_counter" is retyped as long, because it has a higher update frequency than other counters.
>> - before/after sample output is attached to the bug description.
>>
>> Thank you!
>> Lutz
>
> Lutz Schmidt has updated the pull request incrementally with one additional commit since the last revision:
>
>   update copyright year

src/hotspot/share/oops/method.cpp line 511:

> 509:   tty->cr();
> 510:
> 511:   // Internal counting is based on signed int counters. They tend to

Is there a good reason to not simply make them unsigned int?

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

PR: https://git.openjdk.java.net/jdk/pull/2511
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261447: MethodInvocationCounters frequently run into overflow [v7]

Lutz Schmidt
On Thu, 18 Feb 2021 05:12:08 GMT, David Holmes <[hidden email]> wrote:

>> Lutz Schmidt has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   update copyright year
>
> src/hotspot/share/oops/method.cpp line 511:
>
>> 509:   tty->cr();
>> 510:
>> 511:   // Internal counting is based on signed int counters. They tend to
>
> Is there a good reason to not simply make them unsigned int?

Well, depends on what you accept as a good reason. :-)

I decided to keep the counters as they are to limit the scope of the change. A grep for backedge_counter returns 94 lines, for example. Deep down, these counters are InvocationCounters, declared as uint. On their way up to the surface, they are treated signed or unsigned. Pretty inconsistent, yes. But a huge task to get it all straight, including checking/fixing assembly code.

Is that reason enough?

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

PR: https://git.openjdk.java.net/jdk/pull/2511
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261447: MethodInvocationCounters frequently run into overflow [v7]

David Holmes
On 18/02/2021 9:25 pm, Lutz Schmidt wrote:

> On Thu, 18 Feb 2021 05:12:08 GMT, David Holmes <[hidden email]> wrote:
>
>>> Lutz Schmidt has updated the pull request incrementally with one additional commit since the last revision:
>>>
>>>    update copyright year
>>
>> src/hotspot/share/oops/method.cpp line 511:
>>
>>> 509:   tty->cr();
>>> 510:
>>> 511:   // Internal counting is based on signed int counters. They tend to
>>
>> Is there a good reason to not simply make them unsigned int?
>
> Well, depends on what you accept as a good reason. :-)
>
> I decided to keep the counters as they are to limit the scope of the change. A grep for backedge_counter returns 94 lines, for example. Deep down, these counters are InvocationCounters, declared as uint. On their way up to the surface, they are treated signed or unsigned. Pretty inconsistent, yes. But a huge task to get it all straight, including checking/fixing assembly code.
>
> Is that reason enough?

I guess so :) It sounds terribly messy and confused.

Thanks,
David

> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/2511
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261447: MethodInvocationCounters frequently run into overflow [v7]

Martin Doerr
In reply to this post by Lutz Schmidt
On Wed, 17 Feb 2021 18:22:01 GMT, Lutz Schmidt <[hidden email]> wrote:

>> Dear community,
>> may I please request reviews for this fix, improving the usefulness of method invocation counters.
>> - aggregation counters are retyped as uint64_t, shifting the overflow probability way out (185 days in case of a 1 GHz counter update frequency).
>> - counters for individual methods are interpreted as (unsigned int), in contrast to their declaration as int. This gives us a factor of two before the counters overflow.
>> - as a special case, "compiled_invocation_counter" is retyped as long, because it has a higher update frequency than other counters.
>> - before/after sample output is attached to the bug description.
>>
>> Thank you!
>> Lutz
>
> Lutz Schmidt has updated the pull request incrementally with one additional commit since the last revision:
>
>   update copyright year

This version looks ok. I understand that you don't want to clean up the whole singed / unsigned mess. That's fine with me. I'd only like to see one confusing comment removed or replaced.
You may also want to check your 64 bit overflow time in the description: I guess 185 days matches a 1 THz counter update frequency. With 1 GHz it should be above 500 years.

src/hotspot/share/runtime/java.cpp line 100:

> 98: int compare_methods(Method** a, Method** b) {
> 99:   // invocation_count() may have overflowed already. Interpret it's result as
> 100:   // unsigned int to shift the limit of meaningless results by a factor of 2.

Code is fine, but this comment doesn't make sense to me. The result is the same with your version. But it has the advantage that it avoids signed integer overflow (undefined behavior).

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

Changes requested by mdoerr (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2511
Reply | Threaded
Open this post in threaded view
|

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

Lutz Schmidt
In reply to this post by Lutz Schmidt
> Dear community,
> may I please request reviews for this fix, improving the usefulness of method invocation counters.
> - aggregation counters are retyped as uint64_t, shifting the overflow probability way out (> 500 years in case of a 1 GHz counter update frequency).
> - counters for individual methods are interpreted as (unsigned int), in contrast to their declaration as int. This gives us a factor of two before the counters overflow.
> - as a special case, "compiled_invocation_counter" is retyped as long, because it has a higher update frequency than other counters.
> - before/after sample output is attached to the bug description.
>
> Thank you!
> Lutz

Lutz Schmidt has updated the pull request incrementally with one additional commit since the last revision:

  comment changes requested by TheRealMDoerr

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2511/files
  - new: https://git.openjdk.java.net/jdk/pull/2511/files/0f220ee3..e8af119b

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2511&range=07
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2511&range=06-07

  Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2511.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2511/head:pull/2511

PR: https://git.openjdk.java.net/jdk/pull/2511
Reply | Threaded
Open this post in threaded view
|

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

Martin Doerr
On Thu, 25 Feb 2021 09:01:10 GMT, Lutz Schmidt <[hidden email]> wrote:

>> Dear community,
>> may I please request reviews for this fix, improving the usefulness of method invocation counters.
>> - aggregation counters are retyped as uint64_t, shifting the overflow probability way out (> 500 years in case of a 1 GHz counter update frequency).
>> - counters for individual methods are interpreted as (unsigned int), in contrast to their declaration as int. This gives us a factor of two before the counters overflow.
>> - as a special case, "compiled_invocation_counter" is retyped as long, because it has a higher update frequency than other counters.
>> - before/after sample output is attached to the bug description.
>>
>> Thank you!
>> Lutz
>
> Lutz Schmidt has updated the pull request incrementally with one additional commit since the last revision:
>
>   comment changes requested by TheRealMDoerr

Marked as reviewed by mdoerr (Reviewer).

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

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