<i18n dev> RFR: 8335252: ForceInline j.u.Formatter.Conversion#isValid [v2]

[Claes Redestad]

On Thu, 27 Jun 2024 14:12:36 GMT, Shaojin Wen <duke at openjdk.org> wrote:

>> Currently, the java.util.Formatter$Conversion::isValid method is implemented based on switch, which cannot be inlined because codeSize > 325. This problem can be avoided by implementing it with ImmutableBitSetPredicate.
>> 
>> use `-XX:+UnlockDiagnosticVMOptions -XX:+PrintInlining` to see the master branch:
>> 
>> @ 109   java.util.Formatter$Conversion::isValid (358 bytes)   failed to inline: hot method too big
>> 
>> 
>> current version
>> 
>> @ 109   java.util.Formatter$Conversion::isValid (10 bytes)   inline (hot)
>>   @ 4   jdk.internal.util.ImmutableBitSetPredicate$SmallImmutableBitSetPredicate::test (50 bytes)   inline (hot)
>
> Shaojin Wen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   revert & use `@ForceInline`

So the compound issue here is that there's a heuristic which prevents inlining of large methods, and that a tableswitch on inputs spanning from `'%' (37)` to `'x' (120)` become expansive enough (350bytes) to make the JIT enforce that rule.

As an alternative to `@ForceInline` I tested splitting uppercase and lowercase chars into two switches. This looks a bit nasty, but actually reduces the size of the method:

diff --git a/src/java.base/share/classes/java/util/Formatter.java b/src/java.base/share/classes/java/util/Formatter.java
index a7d95ee5780..50419f1ea23 100644
--- a/src/java.base/share/classes/java/util/Formatter.java
+++ b/src/java.base/share/classes/java/util/Formatter.java
@@ -4947,30 +4947,36 @@ static class Conversion {
         static final char PERCENT_SIGN        = '%';

         static boolean isValid(char c) {
-            return switch (c) {
-                case BOOLEAN,
-                     BOOLEAN_UPPER,
-                     STRING,
-                     STRING_UPPER,
-                     HASHCODE,
-                     HASHCODE_UPPER,
-                     CHARACTER,
-                     CHARACTER_UPPER,
-                     DECIMAL_INTEGER,
-                     OCTAL_INTEGER,
-                     HEXADECIMAL_INTEGER,
-                     HEXADECIMAL_INTEGER_UPPER,
-                     SCIENTIFIC,
-                     SCIENTIFIC_UPPER,
-                     GENERAL,
-                     GENERAL_UPPER,
-                     DECIMAL_FLOAT,
-                     HEXADECIMAL_FLOAT,
-                     HEXADECIMAL_FLOAT_UPPER,
-                     LINE_SEPARATOR,
-                     PERCENT_SIGN -> true;
-                default -> false;
-            };
+            if (c >= 'a') {
+                return switch (c) {
+                    case BOOLEAN,
+                         STRING,
+                         HASHCODE,
+                         CHARACTER,
+                         DECIMAL_INTEGER,
+                         OCTAL_INTEGER,
+                         HEXADECIMAL_INTEGER,
+                         SCIENTIFIC,
+                         GENERAL,
+                         DECIMAL_FLOAT,
+                         HEXADECIMAL_FLOAT,
+                         LINE_SEPARATOR -> true;
+                    default -> false;
+                };
+            } else {
+                return switch (c) {
+                    case BOOLEAN_UPPER,
+                         STRING_UPPER,
+                         HASHCODE_UPPER,
+                         CHARACTER_UPPER,
+                         HEXADECIMAL_INTEGER_UPPER,
+                         SCIENTIFIC_UPPER,
+                         GENERAL_UPPER,
+                         HEXADECIMAL_FLOAT_UPPER,
+                         PERCENT_SIGN -> true;
+                    default -> false;
+                };
+            }
         }

         // Returns true iff the Conversion is applicable to all objects.

`javap -v java.lang.Formatter$Conversion`

  static boolean isValid(char);
    descriptor: (C)Z
    flags: (0x0008) ACC_STATIC
    Code:
      stack=2, locals=1, args_size=1
         0: iload_0
         1: bipush        97
         3: if_icmplt     122
         6: iload_0
         7: tableswitch   { // 97 to 120
                      97: 116
                      98: 116
                      99: 116
                     100: 116
                     101: 116
                     102: 116
                     103: 116
                     104: 116
                     105: 120
                     106: 120
                     107: 120
                     108: 120
                     109: 120
                     110: 116
                     111: 116
                     112: 120
                     113: 120
                     114: 120
                     115: 116
                     116: 120
                     117: 120
                     118: 120
                     119: 120
                     120: 116
                 default: 120
            }
       116: iconst_1
       117: goto          121
       120: iconst_0
       121: ireturn
       122: iload_0
       123: lookupswitch  { // 9
                      37: 204
                      65: 204
                      66: 204
                      67: 204
                      69: 204
                      71: 204
                      72: 204
                      83: 204
                      88: 204
                 default: 208
            }
       204: iconst_1
       205: goto          209
       208: iconst_0
       209: ireturn
 ```
 
 And sees an improvement similar to `@ForceInline` on the micro:
 ```
 Name            Cnt   Base   Error    Test   Error  Unit  Change
stringIntFormat  15 92,447 ± 2,497  82,912 ± 3,317 ns/op   1,11x (p = 0,000*)
  * = significant
  ```
Shrinking the size of the .class file by 121 bytes is a fun bonus.

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

PR Comment: https://git.openjdk.org/jdk/pull/19926#issuecomment-2196585088