RFR: JDK-8219412: Eager enum class initialization with enum switch
forax at univ-mlv.fr
forax at univ-mlv.fr
Mon Apr 8 14:02:42 UTC 2019
----- Mail original -----
> De: "jan lahoda" <jan.lahoda at oracle.com>
> À: "Remi Forax" <forax at univ-mlv.fr>
> Cc: "compiler-dev" <compiler-dev at openjdk.java.net>, "Liam Miller-Cushon" <cushon at google.com>
> Envoyé: Lundi 8 Avril 2019 10:17:05
> Objet: Re: RFR: JDK-8219412: Eager enum class initialization with enum switch
> Hi Remi,
>
> Even though I understand what you mean, not sure if it is appropriate to
> do this here (i.e. under an ordinary P3 bug). If we only had the
> switch-over-enum, then it might be OK. But we even currently have the
> switch-over-string, and foresee a possibility of more complex switches,
> and this switch-over-enum bootstrap may the outlier among the other
> bootstraps (or maybe not; or maybe that is OK; but without a more
> general consideration, it is difficult to tell).
>
> I'd be much more inclined to either:
> -use a local solution that is not much different from the current state
> (i.e. version .00 or possibly .01 of this patch), and convert to the
> switch bootstrap when that's done
> -work on the complete switch bootstraps effort, and accept it may take
> some time before this bug is fixed.
Hi Jan,
i've spent some time thinking about that, i believe that the solution i've proposed has the weakness of using an int array as return type of the condy,
something that should be abstracted. So i agree with your local solution.
The main issue of this solution is that people that maintain tools like a code coverage, a fuzzer, a profiler etc may have to change their tool because the patch introduce a new synthetic method that their tools may not want to be visible from the user POV. But maybe we can introduce the switch on enum using indy in Java 13 too.
BTW, i don't believe that we should try to jump from solving the enum switch to let's design a complete switch solution given that a complete switch solution should encompass the enum switch solution and we still don't know what an enum switch that uses indy is. Or said differently, the complete switch should offer the same backward compatibility guarantee that an enum switch has if all the cases are enum values but the switch is on Object.
>
> Thanks,
> Jan
regards,
Rémi
>
> On 5.4.2019 23:23, forax at univ-mlv.fr wrote:
>> ----- Mail original -----
>>> De: "jan lahoda" <jan.lahoda at oracle.com>
>>> À: "Remi Forax" <forax at univ-mlv.fr>
>>> Cc: "compiler-dev" <compiler-dev at openjdk.java.net>, "Liam Miller-Cushon"
>>> <cushon at google.com>
>>> Envoyé: Vendredi 5 Avril 2019 19:35:35
>>> Objet: Re: RFR: JDK-8219412: Eager enum class initialization with enum switch
>>
>>> On 5.4.2019 19:14, forax at univ-mlv.fr wrote:
>>>> ----- Mail original -----
>>>>> De: "jan lahoda" <jan.lahoda at oracle.com>
>>>>> À: "Remi Forax" <forax at univ-mlv.fr>
>>>>> Cc: "compiler-dev" <compiler-dev at openjdk.java.net>, "Liam Miller-Cushon"
>>>>> <cushon at google.com>
>>>>> Envoyé: Vendredi 5 Avril 2019 13:56:58
>>>>> Objet: Re: RFR: JDK-8219412: Eager enum class initialization with enum switch
>>>>
>>>>> On 4.4.2019 20:39, Remi Forax wrote:
>>>>>> Hi Jan,
>>>>>> it seems like a perfect case for a constant dynamic,
>>>>>> the array can be loaded using ldc and the bootstrap method takes the
>>>>>> enum names as constant arguments.
>>>>>
>>>>> True, I didn't realize that. javac does not currently (AFAIK) have a way
>>>>> to generate condy, so we first need support for condy in javac. This
>>>>> patch strives to do that:
>>>>> http://cr.openjdk.java.net/~jlahoda/8219412/webrev.condy.01/
>>>>> based on Vicente's:
>>>>> http://hg.openjdk.java.net/amber/amber/rev/8e2f2d280e44
>>>>>
>>>>> The usage to use that to generate the condy where available (and fall
>>>>> back on generating the holder classes where not available):
>>>>> http://cr.openjdk.java.net/~jlahoda/8219412/webrev.01/
>>>>
>>>> The description of an enum (EnumMapping) can be encoded by the enum class and an
>>>> array of String (in the order of the enum values at compile time).
>>>> So instead of generating one specific method per enum that initializes the
>>>> mapping table and using it as a bootstrap method, i believe it's better to have
>>>> one bootstrap method in the JDK (let say in
>>>> java.lang.invoke.ConstantBootstraps) that takes the enum class and an array of
>>>> Strings. This method will use ConstantBootstraps.enumConstant to get the
>>>> corresponding enum value at runtime (which does the security checks) and a call
>>>> ordinal() on each enum value in order to dynamically create the mapping table.
>>>> This will reduce the size of the generated bytecode by removing the need to have
>>>> one bootstrap method per enum.
>>>
>>> Well, this is effectively the "use condy for switch" effort, right?
>>
>> yes !
>>
>>>
>>> (The enumSwitch method from:
>>> http://hg.openjdk.java.net/amber/amber/file/4461b14e6543/src/java.base/share/classes/java/lang/compiler/SwitchBootstraps.java
>>>
>>> is relevant, as is stringSwitch for switch-over-strings.) Not sure if we
>>> have time for that for 13.
>>
>> The switch-over-enum proposed by SwitchBootstraps uses an invokedynamic and
>> understanding the perf of indy is not that simple.
>> The code you are proposing uses condy, i think we should keep it that way.
>>
>> At the same time, your patch generates bytecodes to initialize the array. I
>> think this is an overkill.
>> As shown by SwitchBootstraps.enumSwitch, you can represent the information that
>> the compiler see at compile time with an enum class and an array of enum value
>> names,
>> so what i'm proposing is to still use condy to load the array of enum constants
>> but to only use one bootstrap method that dynamically create the array.
>>
>> For the compiler, it's easier because you generate less bytecodes (it's not
>> fully true because it seems the compiler generate the EnumMapping pretty
>> early),
>> in term of bugs, because you always call the same bootstrap method which is
>> inside the JDK, if there is a bug, you can fix it without changing neither the
>> compiler not the generated code,
>> and it makes the .class slimmer which is usually a win.
>>
>> so the idea is to still use condy but initialize it by sending a call and an
>> array of strings instead of initialize it by a special tailored bootstrap
>> method.
>>
>> to be 100% clear, here is the BSM i think we should use:
>> public static int initEnumMappingArray(Lookup lookup,
>> String name,
>> Class<?> type, // should be int[].class
>> Class<?> enumClass,
>> String... enumNames) {
>> requireNonNull(lookup);
>> requireNonNull(name);
>> requireNonNull(type);
>> validateClassAccess(lookup, enumClass); // does the class that contains the
>> ldc condy can see the array?
>>
>> int length = enumClass.getEnumConstants().length;
>> int[] array = new int[length];
>> for(int i = 0; i < enumNames.length; i++) {
>> try {
>> Enum<?> enumValue = Enum.valueOf(enumClass, enumNames[i]);
>> array[enumValue.ordinal()] = i + 1;
>> } catch (IllegalArgumentException e) {
>> // empty
>> }
>> }
>> return array;
>> }
>>
>> This is the same code as the code which is generated currently by the compiler,
>> it can be optimized using java.lang.Class internals but this can be done later
>>
>>>
>>> Jan
>>
>> Rémi
>>
>>>
>>>>
>>>> Also in Target.java, constant dynamic is available since 11 but if there is a
>>>> need of a new BSM in ConstantBootstraps, it should be since 13 (apart if the
>>>> patch is backported to 11).
>>>>
>>>>
>>>>>
>>>>> (The downside of that is that when indy switch desugaring will be
>>>>> implemented, there will be three distinct desugarings for switches over
>>>>> enums.)
>>>>
>>>> yes, as you said, we may able to share some code in the future, we will see when
>>>> we get there.
>>>>
>>>>>
>>>>>>
>>>>>> You doesn't have to create one class per switch, it's lazy initialized
>>>>>
>>>>> Please note that neither the original patch, nor the current javac
>>>>> state, creates a class per switch. It creates a field (and with the .00
>>>>> version of the patch a class) per enum used as switch selector per
>>>>> top-level class.
>>>>
>>>>
>>>> yes
>>>>
>>>>>
>>>>> Jan
>>>>
>>>> Rémi
>>>>
>>>>>
>>>>>> like any constant pool item and each switch on the same enum type can
>>>>>> reuse the same,
>>>>>> compare to the solution implemented by Eclipse, it's not a field but a
>>>>>> constant pool constant you you can not mess with it using reflection and
>>>>>> you don't need to check if the array was loaded or not in the fastpath.
>>>>>>
>>>>>> regards,
>>>>>> Rémi
>>>>>>
>>>>>> ------------------------------------------------------------------------
>>>>>>
>>>>>> *De: *"Liam Miller-Cushon" <cushon at google.com>
>>>>>> *À: *"jan lahoda" <jan.lahoda at oracle.com>
>>>>>> *Cc: *"compiler-dev" <compiler-dev at openjdk.java.net>
>>>>>> *Envoyé: *Jeudi 4 Avril 2019 20:20:00
>>>>>> *Objet: *Re: RFR: JDK-8219412: Eager enum class initialization with
>>>>>> enum switch
>>>>>>
>>>>>> Hi Jan,
>>>>>> There are a couple of other bugs about enum switch / class
>>>>>> initialization issues that I think are addressed by this approach:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-7176515
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8169526
>>>>>>
>>>>>> Do you have a sense of how many additional auxiliary classes this
>>>>>> will create for typical applications? I'm wondering if the number of
>>>>>> additional classes might be large enough to effect performance (i.e.
>>>>>> binary size, classloading).
>>>>>>
>>>>>> Were other lazy initialization patterns considered? JDK-7176515
>>>>>> mentions that ecj avoids this by caching the switch map in a field
>>>>>> in the same class as the switch.
>>>>>>
>>>>>> On Thu, Apr 4, 2019 at 11:04 AM Jan Lahoda <jan.lahoda at oracle.com
>>>>>> <mailto:jan.lahoda at oracle.com>> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> When there is a switch over enum in the code, and is compiled
>>>>>> into the
>>>>>> bytecode, the meaning of the switch should not change even if
>>>>>> the enum's
>>>>>> constant are reordered. This is achieved by a map that maps the
>>>>>> (current) enum constants to the appropriate case in the switch.
>>>>>> This map
>>>>>> is stored in a field in an auxiliary class, and filled when that
>>>>>> auxiliary class is loaded in.
>>>>>>
>>>>>> But, if there are multiple switches over distinct enums under a
>>>>>> single
>>>>>> top-level class, the mapping fields for all the enums are stored
>>>>>> in a
>>>>>> single class. So, when any of these is needed, the mapping is
>>>>>> constructed for all the enums, which may cause enum class
>>>>>> initialization
>>>>>> even in cases where it shouldn't happen.
>>>>>>
>>>>>> The proposed fix is to create a separate auxiliary classes for each
>>>>>> enum/mapping.
>>>>>>
>>>>>> Proposed patch:
>>>>>> http://cr.openjdk.java.net/~jlahoda/8219412/webrev.00/
>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8219412
>>>>>>
>>>>>> How does this look?
>>>>>>
>>>>>> Thanks,
> >>>>> Jan
More information about the compiler-dev
mailing list