[core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control

[forax at univ-mlv.fr]

----- Mail original -----
> De: "David Holmes" <david.holmes at oracle.com>
> À: "Remi Forax" <forax at univ-mlv.fr>, "Alan Bateman" <Alan.Bateman at oracle.com>
> Cc: "core-libs-dev" <core-libs-dev at openjdk.java.net>
> Envoyé: Jeudi 17 Mai 2018 10:37:46
> Objet: Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control

> Hi Remi,

Hi David

> 
> On 17/05/2018 6:16 PM, Remi Forax wrote:
>> Hi all,
>> 
>> ----- Mail original -----
>>> De: "Alan Bateman" <Alan.Bateman at oracle.com>
>>> À: "David Holmes" <david.holmes at oracle.com>, "core-libs-dev"
>>> <core-libs-dev at openjdk.java.net>
>>> Envoyé: Mardi 15 Mai 2018 15:53:44
>>> Objet: Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based
>>> Access Control
>> 
>>> On 15/05/2018 01:52, David Holmes wrote:
>>>> This review is being spread across four groups: langtools, core-libs,
>>>> hotspot and serviceability. This is the specific review thread for
>>>> core-libs - webrev:
>>>>
>>>> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
>> 
>> [...]
>> 
>>> Maybe a question for Kumar but are we planning to pull in any ASM
>>> updates for JDK 11? NestMembers extends Attribute looks okay, I'm less
>>> sure about the change to ClassReader as I don't know if there is
>>> somewhere else in ASM that has the list of attributes to always parse.
>> 
>> With my ASM hat,
>> the current master of ASM (the release of ASM 6.2 is scheduled for the next
>> week-end) already supports nestmates (and constant dynamic and preview feature)
>> so i suppose that at some point in the future Kumar will merge it to the JDK.
> 
> Unfortunately Kumar is no longer with us.

I will miss him.

> 
>> We have recently changed the way we implement features in ASM, instead of having
>> features lingering in different branches, we now integrate them directly in the
>> master under an experimental flag (ASM7_EXPERIMENTAL), which means for the JDK
>> that it is no longer necessary to wait until the release of ASM 7 because it
>> can use the experimental support of ASM 6.2.
>> (note that experimental doesn't mean full of bugs, or half baked or anything
>> like this, it means that the feature is not yet integrated in a released JDK).
>> 
>> I've taking a look to the code in this patch, i've two comments,
>> - in Attributes, it seems that the code store the bytecode slice corresponding
>> to the attribute only to use its length as argument of the ByteVector which is
>> like an ArrayList of byte, it grows automatically so the initial capacity is a
>> perf optimization. Perhaps the byte array is used somewhere else ?
>> - patching the ClassReader.accept is really a quick hack because the method
>> accept with 3 arguments is not patched so if this method is called somewhere in
>> the JDK it will behave as it should.
> 
> I'll take a look at this. To be honest I don't even remember who
> provided those changes ... I thought you had provided feedback at some
> point in the past :) 

yes, i may have, i do not remember.

> There's a valhalla-dev email with a link that's no
> longer valid:
> 
> https://gitlab.ow2.org/asm/asm/tree/NEST_MATES

as i said above, the support has been integrated in the master of ASM,
see https://gitlab.ow2.org/asm/asm/blob/master/asm/src/main/java/org/objectweb/asm/ClassVisitor.java#L156
and https://gitlab.ow2.org/asm/asm/blob/master/asm/src/main/java/org/objectweb/asm/ClassVisitor.java#L246
 
the current code is fine, just not optimal, but given that when the JDK will use ASM 6.2, this code will have to be removed, i do not think you should spend some time one this.

> 
> Thanks,
> David

regards,
Rémi