Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[Mandy Chung]

On 3/27/20 11:59 AM, Paul Sandoz wrote:
> Hi Mandy,
>
> Very thorough, bravo!

Thanks.
> Minor suggestions below.
>
> Paul.
>
> MethodHandleNatives.java
> —
>
>   142
>   143         /**
>   144          * Flags for Lookup.ClassOptions
>   145          */
>   146         static final int
>   147             NESTMATE_CLASS            = 0x00000001,
>   148             HIDDEN_CLASS              = 0x00000002,
>   149             STRONG_LOADER_LINK        = 0x00000004,
>   150             ACCESS_VM_ANNOTATIONS     = 0x00000008;
>   151     }
>   
>
> Suggest you add a comment to keep the values in sync with the VM component.

Already in the class spec of this Constants class.  The values of all 
constants defined in this Constants class are verified in sync with VM 
(see verifyConstants).

>
> MethodHandles.java
> —
>
> 1786          * (Given the {@code Lookup} object returned this method, its lookup class
> 1787          * is a {@code Class} object for which {@link Class#getName()} returns a string
> 1788          * that is not a binary name.)
>
> “
> (The {@code Lookup} object returned from this method has a lookup class that is
> a {@code Class} object whose {@link Class#getName()} returns a string
> that is not a binary name.)
> “
>
>
> 1902             Set<ClassOption> opts = options.length > 0 ? Set.of(options) : Set.of();
>
> You can just do:
>
>    Set<ClassOption> opts = Set.of(options)
>
> And/or inline it into the subsequent method call.  The implementation of Set.of checks the array length.

Great to know.  Thanks.
>
> 2001         ClassDefiner makeHiddenClassDefiner(byte[] bytes,
>
> I think you can telescope the methods for non-name and name accepting since IIUC the name is derived from the byte[].  Thereby you can remove some code duplication. i.e. pull ClassDefiner.className out from ClassDefiner and place the logic in the factory methods.  Alternative push the factory methods into ClassDefiner to keep all the logic together.
>
Ok.  I will move the className out.
>
> 3797         public enum ClassOption {
>
> Shuffle up to be closer to the defineHiddenClass

Moved before defineHiddenClass.

>
> 3798             /**
> 3799              * This class option specifies the hidden class be added to
> 3800              * {@linkplain Class#getNestHost nest} of a lookup class as
> 3801              * a nestmate.
>
> Suggest:
>
> "This class option specifies the hidden class “ -> “Specifies that a hidden class
>
> 3812              * This class option specifies the hidden class to have a <em>strong</em>
>
> “Specifies that a hidden class have a …"

Specifies that a hidden class has a...

>
> 3813              * relationship with the class loader marked as its defining loader,
> 3814              * as a normal class or interface has with its own defining loader.
> 3815              * This means that the hidden class may be unloaded if and only if
> 3816              * its defining loader is not reachable and thus may be reclaimed
> 3817              * by a garbage collector (JLS 12.7).
>
>
> StringConcatFactory.java
> —
>
>   861             // use of @ForceInline no longer has any effect
>
> ?

Right, I should have explained this [1].

This @ForceInline is used by BytecodeStringBuilderStrategy that 
generates code to have the same StringBuilder chain javac would emit. It 
uses `@ForceInline` annotation which may probably be for performance.  
It's believed people rarely uses this non-default strategy.  This patch 
changes StringConcatFactory to the standard defineHiddenClass method and 
hence `@ForceInline` has no effect in the generated class for this 
non-default strategy.  If it turns out to be an issue, then we will 
determine if it should enable the access to VM annotations (I doubt this 
is supported strategy).

[1] https://bugs.openjdk.java.net/browse/JDK-8241548