RFR: JDK-8219412: Eager enum class initialization with enum switch
Jan Lahoda
jan.lahoda at oracle.com
Mon Apr 8 08:17:05 UTC 2019
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.
Thanks,
Jan
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