RFR: 8331253: 16 bits is not enough for nmethod::_skipped_instructions_size field

[Vladimir Kozlov]

On Thu, 2 May 2024 12:35:45 GMT, Roberto Castañeda Lozano <rcastanedalo at openjdk.org> wrote:

>> In [JDK-8329433](https://bugs.openjdk.org/browse/JDK-8329433) I changed `nmethod::_skipped_instructions_size` field type to `uint16_t` assuming that it only count NOP instructions and GC barriers. I did not take into account that Generational ZGC also incudes barrier stubs into this size (original ZGC missed that). It is correct to include them because these stubs are generated in instructions section and not in stubs section:
>> 
>> 
>> Statistics for 1330 bytecoded nmethods for C2:
>> ...
>> ZGC:
>>    main code = 3237080 (75.567032%)
>>      stubs code = 810577 (25.040375%)
>>      skipped insts = 44432 (1.372595%)
>> 
>> GenZGC:
>>    main code = 4034704 (78.238518%)
>>      stubs code = 1356703 (33.625839%)
>>      skipped insts = 1074611 (26.634197%)
>> 
>> 
>> Note, GenZGC has bigger code because it has store barriers. It generates a separate stub for each barrier, no sharing.
>> 
>> After looking on how `_skipped_instructions_size` is used (only in one place when calculated inlinining size of compiled code) I decided replace it with `int _inline_insts_size;`. It is calculated the same way as before.
>> 
>> And instead of including instructions stubs into `_skipped_instructions_size` I recorded size of instructions in code section before stubs are generated. This allow to get more accurate size of main instructions and no need for `InlineSkippedInstructionsCounter` in GC barriers stubs.
>> 
>> I also fixed code in C2 which estimates size of code and stubs sections.
>> 
>> Tested tier1-4,tier8,stress,xcomp
>
> Thanks for working on this, Vladimir! I tried out this changeset on a simple example ([example-and-instrumentation.zip](https://github.com/openjdk/jdk/files/15188249/example-and-instrumentation.zip)) using a JVM instrumented with the attached patch to observe the output of `ciMethod::inline_instructions_size()` and this seems to differ before and after the changeset:
> 
> Before:
> 
> 
> caller: Test foo (LTest$MyObject;)Ljava/lang/Object; inline instructions size: 0
> callee: Test bar (LTest$MyObject;)V inline instructions size: 219
> 
> 
> after:
> 
> 
> caller: Test foo (LTest$MyObject;)Ljava/lang/Object; inline instructions size: 0
> callee: Test bar (LTest$MyObject;)V inline instructions size: 183
> 
> Is this deviation expected? If so, I suggest to split this changeset into a simple bug fix that only widens the type of `nmethod::_skipped_instructions_size` without affecting the inlining heuristic, and a RFE with the remaining changes.

@robcasloz, thank you for looking on PR.  Yes, 183 is more accurate number. I don't think I need to split it. Splitting is needed if you need to backport. Which is not my case.

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

PR Comment: https://git.openjdk.org/jdk/pull/19029#issuecomment-2090670068