RFR: JDK-8219412: Eager enum class initialization with enum switch

[Jan Lahoda]

On 9.4.2019 12:16, Maurizio Cimadamore wrote:
> I'm sorry Remi - in terms of complexity, I just don't see v1 being the
> sweet spot. First, it needs condy support, which, while we have in
> prototype form, we don't have it available in full in vanilla javac.
> Secondly, I'm very skeptical that any of that code could be reused when
> we go to the pattern-match-based implementation. So, overall, I'm not
> convinced.
>
> As mentioned yesterday, I'd like to know, with real code bases (like
> e.g. the JDK itself), how much extra static footprint are we looking at
> exactly.

For OpenJDK, there are ~150 additional classes (from 30267 to 30415) 
with the .00 version of the patch.

Jan

>
> Maurizio
>
> On 09/04/2019 08:08, Remi Forax wrote:
>> Hi Maurizio,
>>
>> I'm fine with
>>    http://cr.openjdk.java.net/~jlahoda/8219412/webrev.01/
>>
>> It generates one method per enum instead of one class per enum which
>> is a net win compared to the version 00.
>>
>> There is another reason to go that way, the solution proposed by Jan
>> is very close to the translation we want for the lazy static final
>> (without the retconing of getstatic)
>>
>> Roughly in Java-ish, the translation scheme is
>>    switch(anEnum) {
>>      case FOO: ...
>>      case BAR: ...
>>    }
>>
>> is translated to
>>    private lazy static final int[] TRANSLATION_TABLE = buildTable();
>>
>>    switch(ldc TRANSLATION_TABLE[anEnum.ordinal]) {
>>      case FOO_ORDINAL: ...
>>      case BAR_ORDINAL: ...
>>    }
>>
>> with the field TRANSLATION_TABLE not needed because all access are
>> inside the class so ldc is enough.
>>
>> cheers,
>> Rémi
>>
>> ----- Mail original -----
>>> De: "Maurizio Cimadamore" <maurizio.cimadamore at oracle.com>
>>> À: "jan lahoda" <jan.lahoda at oracle.com>, "Brian Goetz"
>>> <brian.goetz at oracle.com>, "compiler-dev"
>>> <compiler-dev at openjdk.java.net>
>>> Envoyé: Lundi 8 Avril 2019 17:41:28
>>> Objet: Re: RFR: JDK-8219412: Eager enum class initialization with
>>> enum switch
>>> It seems to me that it's not worth spending cycles in fixing what is
>>> essentially a dead end, translation-strategy wise.
>>>
>>> Yes, it might take sometime for new pattern translation to settle in,
>>> but this issue has been there since Java 5 - is there any reason as to
>>> why it is now become a 'must fix' (other than it has been discovered
>>> recently) ?
>>>
>>> On the other hand, the fix is so trivial that it is tempting to 'just go
>>> with it', given that it fixes the underlying correctness issue. But, as
>>> pointed out by Liam and others, generating a class per enum switch (of
>>> given enum type), will now generate more classes than before, so you
>>> could make people unhappy by increasing static footprint.
>>>
>>> So I see two possibilities here - either we wait for better backend
>>> story (e.g. switch translation using indy/condy), or we go for the
>>> simple fix you proposed (after having done some basic validation
>>> exercises that the number of new classes isn't too big). Adding condy
>>> support and/or BSM to back up the existing translation story is IMHO a
>>> waste of  resources.
>>>
>>> Maurizio
>>>
>>> On 05/04/2019 12:58, Jan Lahoda wrote:
>>>> On 4.4.2019 20:50, Brian Goetz wrote:
>>>>> Note that when we switch to translating with indy, this can be
>>>>> cached at
>>>>> the indy call site.  (And, in the case where the compile-time and
>>>>> run-time numberings are the same, we can even, if we like,
>>>>> eliminate the
>>>>> indirection through the array.)
>>>> Yes, with the indy desugaring this all goes away. But I suspect it
>>>> will probably take some time before we switch to that, so seemed
>>>> prudent to fix using the approach we currently have.
>>>>
>>>> Jan
>>>>
>>>>> On 4/4/2019 1:59 PM, Jan Lahoda 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