Review Request: 8238358: Implementation of JEP 371: Hidden Classes
Chris Plummer
chris.plummer at oracle.com
Fri Apr 17 22:51:41 UTC 2020
On 4/16/20 9:45 AM, Mandy Chung wrote:
> On 4/14/20 11:51 AM, Paul Sandoz wrote:
>> Looks good to me (not familiar with all the code areas.
>>
>> Minor suggestion:
>>
>> MethodHandles.java
>>
>> 1811 * ASCII periods. For the instance of {@link
>> java.lang.Class} representing {@code C}:
>> 1812 * <ul>
>> 1813 * <li> {@link Class#getName()} returns the string
>> {@code GN + "/" + <suffix>},
>> 1814 * even though this is not a valid binary class or
>> interface name.</li>
>> 1815 * <li> {@link Class#descriptorString()} returns the string
>> 1816 * {@code "L" + N + ";" + "/" + <suffix> },
>> 1817 * even though this is not a valid type descriptor
>> name.</li>
>> 1818 * </ul>
>>
>> Add another bullet:
>>
>> “
>> even though this is not a valid type descriptor name; and
>>
>> - therefore {@link Class#describeConstable} returns an empty {@code
>> Optional}.
>> “
>>
>> ?
>
> OK. I add this bullet:
>
> - Class.describeConstable() returns an empty optional as C cannot be
> described in nominal form.
>
> The webrev and spec was updated [1] for descriptor string to be of the
> form "Lfoo/Foo.1234;" to mitigate the compatibility risk. Th
>
> Specdiff with serviceability spec change:
> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff-inc/
>
> Specdiff without svc spec change:
> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff-descriptor-string/overview-summary.html
>
>
> Webrev:
> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-descriptor-string/
>
>
> Svc spec change webrev:
> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes/
>
>
> thanks
> Mandy
> [1]
> https://mail.openjdk.java.net/pipermail/valhalla-dev/2020-April/007155.html
Hi Mandy,
Thanks for updating the svc specs. Some comments below:
In the JDWP spec update, you changed "JNI signature" to "type signature"
in one place, but left it as "JNI signature" everywhere else. Should
they all be changed?
In the JDWP spec for ClassLoaderReference.VisibleClasses:
"That is, this class loader has been recorded as an <i>initiating</i>
loader of the returned classes." -> "That is, all classes for which this
class loader has been recorded as an <i>initiating</i> loader."
This seems like too much detail to be put here. Basically the term
"initiating ClassLoader" has turned into a short essay. Is it possible
that all this detail could be put elsewhere and referenced? Aren't there
other places in other specs where a similar clarification of "initiating
ClassLoader" is needed (I see now that ClassLoaderClasses in the JVMTI
spec, ClassLoaderReference,visibleClasses in the JDI spec, and
Instrumentation.getInitiatedClasses are all dealing with this, but not
all in the exact same way).
In the JVMTI spec for GetLoadedClasses:
This suffers in a way similar to ClassLoaderReference.VisibleClasses in
the JDWP spec, although not as badly. A simple concept ends up with a
complex description, and it seems that description should really be in a
more centralized place. I would also suggest a bit of cleanup of these
lines:
6866 An array class is created directly by Java virtual
machine. An array class
6867 creation can be triggered by using class loaders or by
invoking methods in certain
6868 Java SE Platform API such as reflection.
"Created by [the] Java virtual machine" (add "the")
Change "An array class creation" to "The creation" since your are
repeating "An array class" from the previous sentence.
In the JVMTI spec ClassLoaderClasses section:
"That is, initiating_loader has been recorded as an initiating loader of
the returned classes." -> "That is, all classes for which
initiating_loader has been recorded as an initiating loader."
In the JVMTI spec GetClassSignature section:
"If the class indicated by klass is ..." -> "If the class ..." (you just
finished the previous sentence with "class indicated by Klass").
"the returned name is [the] JNI type signature" (add "the"). Also, is
"JNI type signature" formally defined somewhere? This relates to my JDWP
spec comment above.
" where N is the binary name encoded in internal form indicated by the
class file". Is "binary name encoded in internal form" explained
somewhere? Also, can you add an example of a returned hidden class
signature?
In the JVMTI spec ClassLoad section:
"representation using [a] class loader" (add "a")
"By invoking Lookup::defineHiddenClass, that creates ..." -> "By
invoking Lookup::defineHiddenClass to create ..."
"certain Java SE Platform API" -> Should be "APIs"
In JDI ClassLoaderReference.definedClasses()
"loaded at least to the point of preparation and types ..." -> "loaded
at least to the point of preparation, and types ..." (Note, this not a
new issue with your edits)
In Instrumentation.getAllLoadedClasses:
The reference to `class` did not format properly.
"by invoking Lookup::defineHiddenClass that creates" -> "by invoking
Lookup::defineHiddenClass, which creates"
"An array class is created directly by Java virtual machine. An array
class creation can be triggered ..." ->"An array class is created
directly by the Java virtual machine. Array class creation can be
triggered ..."
In Instrumentation.getInitiatedClasses:
"That is, loader has been recorded as an initiating loader of these
classes." -> "That is, all classes for which loader has been recorded as
an initiating loader."
thanks,
Chris
>
>
>>
>> Paul.
>>
>>> On Apr 11, 2020, at 8:13 PM, Mandy Chung <mandy.chung at oracle.com>
>>> wrote:
>>>
>>> Please review the delta patch:
>>> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-delta/
>>>
>>>
>>> Incremental specdiff:
>>> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff-inc/overview-summary.html
>>>
>>> This is to follow a discussion [1] on Class::descriptorString and
>>> MethodType::descriptorString for hidden classes. The proposal is
>>> to define the descriptor string for a hidden class of this form:
>>> "L" + N + ";" + "/" + <suffix>
>>>
>>> The spec of `Lookup::defineHiddenClass`, `Class::descriptorString`
>>> and `MethodType::descriptorString` are updated to return the
>>> descriptor of this form for a hidden class. To support hidden
>>> class, `java.lang.invoke.TypeDescriptor` spec is revised such that a
>>> `TypeDescriptor` object can represent an entity that may not be
>>> described in nominal form. This change affects JVM TI, JDWP and
>>> JDI. The spec change includes a couple other JVM TI and
>>> java.instrument spec clarification w.r.t. hidden classes that
>>> Serguei has been working on.
>>>
>>>
>>>
>>> Mandy
>>> [1]
>>> https://mail.openjdk.java.net/pipermail/valhalla-dev/2020-April/007093.html
>>>
>>> ----------------
>>> As a record, the above patch is applied on the top:
>>> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06/
>>>
>>>
>>> webrev.06 includes the following changes that have been reviewed:
>>> 1. rename "weak hidden" and Klass::is_hidden_weak to
>>> is_non_strong_hidden
>>> 2. remove unused `setImplMethod` method from lambda proxy class
>>> 3. include Serguei's patch to fix JDK-8242166: regression in JDI
>>> ClassUnload events
>>> 4. drop the uniqueness guarantee of the suffix of the hidden class's
>>> name
>>>
>>> On 3/31/20 8:01 PM, Mandy Chung wrote:
>>>> Thanks to the review feedbacks.
>>>>
>>>> Updated webrev:
>>>> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.04/
>>>>
>>>> Delta between webrev.03 and webrev.04:
>>>> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03-delta/
>>>>
>>>>
>>>> Summary of changes is:
>>>> 1. Update javac to retain the previous behavior when compiling to
>>>> target release <= 14 where lambda proxy class is not a nestmate
>>>> 2. Rename Class::isHiddenClass to Class::isHidden
>>>> 3. Address Joe's feedback on the CSR to document the behavior of
>>>> the relevant methods in java.lang.Class for hidden classes
>>>> 4. Add test case for unloadable class with nest host error
>>>> 5. Add test cases for hidden classes with different kinds of class
>>>> or interface
>>>> 6. Update dcmd to drop "weak hidden class" and refer it as "hidden
>>>> class"
>>>> 7. Small changes in response to Remi, Coleen, Paul's review comments
>>>> 8. Fix copyright headers
>>>> 9. Fix @modules in tests
>>>>
>>>> Most of the changes above have also been reviewed as separate patches.
>>>>
>>>> Thanks
>>>> Mandy
>>>>
>>>> On 3/26/20 4:57 PM, Mandy Chung wrote:
>>>>> Please review the implementation of JEP 371: Hidden Classes. The
>>>>> main changes are in core-libs and hotspot runtime area. Small
>>>>> changes are made in javac, VM compiler (intrinsification of
>>>>> Class::isHiddenClass), JFR, JDI, and jcmd. CSR [1]has been
>>>>> reviewed and is in the finalized state (see specdiff and javadoc
>>>>> below for reference).
>>>>>
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03
>>>>>
>>>>>
>>>>> javadoc/specdiff
>>>>> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/api/
>>>>>
>>>>> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff/
>>>>>
>>>>>
>>>>> JVMS 5.4.4 change:
>>>>> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/Draft-JVMS-HiddenClasses.pdf
>>>>>
>>>>>
>>>>> CSR:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8238359
>>>>>
>>>>> Thanks
>>>>> Mandy
>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8238359
>>>>> [2] https://bugs.openjdk.java.net/browse/JDK-8240338
>>>>> [3] https://bugs.openjdk.java.net/browse/JDK-8230502
>
More information about the valhalla-dev
mailing list