Review Request: 8242012: Drop the uniqueness guarantee from the suffix of the name of a hidden class

[John Rose]

Thank you for fixing that.  It looks good to me.

> On Apr 7, 2020, at 10:25 AM, Mandy Chung <mandy.chung at oracle.com> wrote:
> 
> On 4/1/20 7:34 PM, John Rose wrote:
>> On Apr 1, 2020, at 6:19 PM, Mandy Chung <mandy.chung at oracle.com <mailto:mandy.chung at oracle.com>> wrote:
>>> 
>>> I file https://bugs.openjdk.java.net/browse/JDK-8242012 <https://bugs.openjdk.java.net/browse/JDK-8242012> to track this issue.
>> 
>> I would prefer to say that the suffix is simply an unspecified
>> simple name.  Then leave it open for implementations to make it more
>> or less unpredictable, as a matter of QoS.  
>> The standard of quality we
>> should aim at is that of System.identityHashCode—nothing much more
>> than that is desirable, and it’s easy to get at least that much quality.
>> 
>> The java doc for Object.hashCode says this about the QoS I want here:
>> 
>>>      * As far as is reasonably practical, the {@code hashCode} method defined
>>>      * by class {@code Object} returns distinct integers for distinct objects.
>> 
>> So, the behavior I would like to keep open is something that cannot
>> readily be distinguished from functions like these…
>> 
>> static String nextSuffix1() { return new Integer.toHexString(Object().hashCode()); }
>> static String nextSuffix2() { return new Integer.toUnsignedString(Object().hashCode(), 36); }
>> static String nextSuffix3() { return nextSuffix1()+nextSuffix2(); }
>> 
>> …and many others.
>> 
>> And I would settle for nextSuffix1, which (after all) is also a suffix of
>> the already specified value of new Object().toString().  To me, the result
>> of HC.toString() (where HC is a hidden class) should not be more deeply
>> informative than new Object().toString().  Also, the behavior currently
>> coded (in mangle_hidden_class_name) is consistent with nextSuffix1.
>> 
>> (Observation:  The JVM has no reason to choose this suffix, beyond the
>> simple reason that it is the first party to peek into the byte[] array of
>> the classfile image.  It would be nice if the JDK platform code could
>> take responsibility for generating that extra suffix; it would make QoS
>> adjustments easier down the road.  We could even go all the way and
>> guarantee that all such suffixes are truly unique; it would be a page of
>> Java code but several pages of HotSpot code.  But, that’s inconvenient,
>> so I don’t recommend that now.)
>> 
>> A parting shot:  Yes, technically the implementation of the suffix
>> choosing (mangle_hidden_class_name) probably upholds almost the full
>> guarantee.  (Although, it can fail if the JVM unloads and reloads a HC
>> of the same name at the same metadata address.)  But if we make the
>> full guarantee part of the API, we set ourselves up to maintain that strong
>> behavior even if we move to another implementation in which metadata
>> pointers are allowed to change.  I know we don’t want to do that, but
>> this is an example of how an overly strong guarantee that seems easy
>> to do in *one* JVM implementation can create headaches for *other*
>> JVM implementations.  What’s easy to do in our implementation is
>> *often* close to the right primitive to specify, but we have a responsibility
>> to remove unnecessary promises and other implementation artifacts
>> from our specification.
>> 
>> — John
> 
> Thanks John for this.
> 
> This is the spec update.   I leave the mangle_hidden_class_name as is.  We can move out the suffix assignment from the VM to the library code as a future cleanup.
> 
> diff --git a/src/java.base/share/classes/java/lang/Class.java b/src/java.base/share/classes/java/lang/Class.java
> --- a/src/java.base/share/classes/java/lang/Class.java
> +++ b/src/java.base/share/classes/java/lang/Class.java
> @@ -799,8 +799,7 @@
>       *      where {@code N} is the <a href="ClassLoader.html#binary-name">binary name</a>
>       *      indicated by the {@code class} file passed to
>       *      {@link java.lang.invoke.MethodHandles.Lookup#defineHiddenClass(byte[], boolean, MethodHandles.Lookup.ClassOption...)
> -     *      Lookup::defineHiddenClass} and {@code <suffix>} is an unqualified name
> -     *      that is guaranteed to be unique during this execution of the JVM.
> +     *      Lookup::defineHiddenClass}, and {@code <suffix>} is an unqualified name.
>       * </ul>
>       *
>       * <p> If this {@code Class} object represents an array class, then
> diff --git a/src/java.base/share/classes/java/lang/invoke/MethodHandles.java b/src/java.base/share/classes/java/lang/invoke/MethodHandles.java
> --- a/src/java.base/share/classes/java/lang/invoke/MethodHandles.java
> +++ b/src/java.base/share/classes/java/lang/invoke/MethodHandles.java
> @@ -1781,8 +1781,7 @@
>           * denote a class or interface in the same package as the lookup class.</li>
>           *
>           * <li> Let {@code CN} be the string {@code N + "." + <suffix>},
> -         * where {@code <suffix>} is an unqualified name that is guaranteed to be unique
> -         * during this execution of the JVM.
> +         * where {@code <suffix>} is an unqualified name.
>           *
>           * <p> Let {@code newBytes} be the {@code ClassFile} structure given by
>           * {@code bytes} with an additional entry in the {@code constant_pool} table,
> 
> thanks
> Mandy