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

[Remi Forax]

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