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.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
