Hi Mandy,
I have a couple of minor comments on the Serviceability spec update.
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes/src/jdk.jdi/share/classes/com/sun/jdi/ReferenceType.java.udiff.html
Replace: "The returned name is the same form as ..." => "The returned name is of the same form as ..."
Example is:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes/src/jdk.jdi/share/classes/com/sun/jdi/Type.java.udiff.html
+ * Returns the name of this type. The result is of the same form as
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes/src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java.udiff.html
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes/src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java.udiff.html
Both files above have this: "a `class` file representation".
It feels like something is wrong with the `class` part.
Should it say `class file` or maybe use some other formatting?
Otherwise, the Serviceability spec update looks great.
I'll look at the non-Serviceability spec changes too.
Thanks,
Serguei
On 4/16/20 09:45, Mandy Chung wrote:
I have a couple of minor comments on the Serviceability spec update.
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes/src/jdk.jdi/share/classes/com/sun/jdi/ReferenceType.java.udiff.html
Replace: "The returned name is the same form as ..." => "The returned name is of the same form as ..."
Example is:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes/src/jdk.jdi/share/classes/com/sun/jdi/Type.java.udiff.html
+ * Returns the name of this type. The result is of the same form as
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes/src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java.udiff.html
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes/src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java.udiff.html
Both files above have this: "a `class` file representation".
It feels like something is wrong with the `class` part.
Should it say `class file` or maybe use some other formatting?
Otherwise, the Serviceability spec update looks great.
I'll look at the non-Serviceability spec changes too.
Thanks,
Serguei
On 4/16/20 09:45, 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
Paul.
On Apr 11, 2020, at 8:13 PM, Mandy Chung <mandy.ch...@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
