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

[Shaojin Wen]

On Fri, 28 Jun 2024 10:22:58 GMT, Claes Redestad <redestad at openjdk.org> wrote:

>> 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_UP...

@cl4es provided a better idea, which is to change the branch of switch instead of using ForceInline. I have another approach:


            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 -> true;
                default -> c == PERCENT_SIGN;
            };

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

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