Re: RFR 8246034: Remove java.base/share/classes/jdk/internal/jrtfs/jrtfsviewer.js and java.base/share/classes/jdk/internal/jrtfs/jrtls.js

[James Laskey]

+1



> On May 28, 2020, at 3:16 AM, sundararajan.athijegannat...@oracle.com wrote:
> 
> Please review.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8246034
> 
> Webrev: http://cr.openjdk.java.net/~sundar/8246034/webrev.00/
> 
> Thanks
> 
> -Sundar
> 

Re: RFR 8246034: Remove java.base/share/classes/jdk/internal/jrtfs/jrtfsviewer.js and java.base/share/classes/jdk/internal/jrtfs/jrtls.js

[Alan Bateman]

On 28/05/2020 07:14, sundararajan.athijegannat...@oracle.com wrote:

Please review.

Bug: https://bugs.openjdk.java.net/browse/JDK-8246034

Webrev: http://cr.openjdk.java.net/~sundar/8246034/webrev.00/
Have you checked if there are any make file configured to copy .js 
files? Otherwise looks good.


-Alan

RFR 8246034: Remove java.base/share/classes/jdk/internal/jrtfs/jrtfsviewer.js and java.base/share/classes/jdk/internal/jrtfs/jrtls.js

[sundararajan . athijegannathan]

Please review.

Bug: https://bugs.openjdk.java.net/browse/JDK-8246034

Webrev: http://cr.openjdk.java.net/~sundar/8246034/webrev.00/

Thanks

-Sundar

Re: RFR: JDK-8225056 VM support for sealed classes

[David Holmes]

Hi Harold,

On 28/05/2020 6:35 am, Harold Seigel wrote:

Hi David,

Please review this updated webrev:

Incremental webrev: 
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/


Changes look good - thanks!

One minor comment:

src/hotspot/share/prims/jvm.cpp

2159 if (length != 0) {
2160   for (int i = 0; i < length; i++) {

The if statement is not needed as the loop will be skipped if length is 0.

On testing:

test/hotspot/jtreg/runtime/modules/SealedModuleTest.java
test/hotspot/jtreg/runtime/sealedClasses/SealedUnnamedModuleTest.java
test/hotspot/jtreg/runtime/sealedClasses/SealedUnnamedModuleIntfTest.java

You don't seem to have coverage of the full test matrix. For the 
combination of "same module or not" x "same package or not" x "public or 
not", there should be 8 test cases: 3 failures and 5 successes. Then you 
also have "listed in permits clause" versus "not listed in permits clause".


Then you have all that for classes and interfaces.

---

On the question of whether to use ReflectionData or not that is really 
question for the core-libs folk to decide.


Thanks,
David
-

full webrev: 
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/


It includes the following changes:

  * Indentation and simplification changes as suggested below
  * If a class is not a valid permitted subclass of its sealed super
then an IncompatibleClassChangeError exception is thrown (as
specified in the JVM Spec) instead of VerifyError.
  * Added a check that a non-public subclass must be in the same package
as its sealed super.  And added appropriate testing.
  * Method Class.permittedSubtypes() was changed.

See also inline comments.


On 5/24/2020 10:28 PM, David Holmes wrote:

Hi Harold,

On 22/05/2020 4:33 am, Harold Seigel wrote:

Hi David,

Thanks for looking at this!  Please review this new webrev:

http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/


I'll list all relevant commens here rather than interspersing below so 
that it is easier to track. Mostly nits below, other than the 
is_permitted_subclass check in the VM, and the use of ReflectionData 
in java.lang.Class.


--

src/hotspot/share/classfile/classFileParser.cpp

+ bool ClassFileParser::supports_sealed_types() {
+   return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
+ _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
+ Arguments::enable_preview();
+ }

Nowe there is too little indentation - the subclauses of the 
conjunction expression should align[1]


+ bool ClassFileParser::supports_sealed_types() {
+   return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
+  _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
+  Arguments::enable_preview();
+ }

Fixed. Along with indentation of supports_records().


3791 if (parsed_permitted_subclasses_attribute) {
3792   classfile_parse_error("Multiple 
PermittedSubclasses attributes in class file %s", CHECK);
3793 // Classes marked ACC_FINAL cannot have a 
PermittedSubclasses attribute.

3794 } else if (_access_flags.is_final()) {
3795   classfile_parse_error("PermittedSubclasses 
attribute in final class file %s", CHECK);

3796 } else {
3797   parsed_permitted_subclasses_attribute = true;
3798 }

The indent of the comment at L3793 is wrong, and its placement is 
awkward because it relates to the next condition. But we don't have to 
use if-else here as any parse error results in immediate return due to 
the CHECK macro. So the above can be reformatted as:


3791 if (parsed_permitted_subclasses_attribute) {
3792   classfile_parse_error("Multiple 
PermittedSubclasses attributes in class file %s", CHECK);

3793 }
3794 // Classes marked ACC_FINAL cannot have a 
PermittedSubclasses attribute.

3795 if (_access_flags.is_final()) {
3796   classfile_parse_error("PermittedSubclasses 
attribute in final class file %s", CHECK);

3797 }
3798 parsed_permitted_subclasses_attribute = true;

Done.


---

src/hotspot/share/oops/instanceKlass.cpp

The logic in InstanceKlass::has_as_permitted_subclass still does not 
implement the rules specified in the JVMS. It only implements a "same 
module" check, whereas the JVMS specifies an accessibility requirement 
as well.

Done.


 730 bool InstanceKlass::is_sealed() const {
 731   return _permitted_subclasses != NULL &&
 732 _permitted_subclasses != 
Universe::the_empty_short_array() &&

 733 _permitted_subclasses->length() > 0;
 734 }

Please align subclauses.

Done.


---

src/hotspot/share/prims/jvm.cpp

2159   objArrayHandle result (THREAD, r);

Please remove space after "result".

Done.


As we will always create and return an arry, if you reverse these two 
statements:


2156 if (length != 0) {
2157   objArrayOop 

Re: RFR 8246034: Remove java.base/share/classes/jdk/internal/jrtfs/jrtfsviewer.js and java.base/share/classes/jdk/internal/jrtfs/jrtls.js

[sundararajan . athijegannathan]

Yes, checked. No config file refers to these .js files.

-Sundar

On 28/05/20 12:28 pm, Alan Bateman wrote:

On 28/05/2020 07:14, sundararajan.athijegannat...@oracle.com wrote:

Please review.

Bug: https://bugs.openjdk.java.net/browse/JDK-8246034

Webrev: http://cr.openjdk.java.net/~sundar/8246034/webrev.00/
Have you checked if there are any make file configured to copy .js 
files? Otherwise looks good.


-Alan

Re: contributing to JDK-8171407: Port fdlibm to Java, part 2

[Raffaello Giulietti]

Hi Joe,

I kind of understand your point of being more at ease with the porting 
itself than with the review.


However, wouldn't a review be needed anyway, regardless how 
knowledgeable the porter might be? That is, even if an expert like 
yourself does the port, somebody else would still have to invest time to 
do the review, I guess, so the overall combined effort would be almost 
the same. At least, that's my understanding on how development in the 
OpenJDK works.


Anyway, although I'm not formally a reviewer, just a contributor, I'll 
be glad to review your code on an informal basis if you need an 
additional opinion.



Greetings
Raffaello


On 2020-05-27 23:44, Joe Darcy wrote:

Hi Raffaello,

Finishing the fdlibm port remains on my to-do list. Given the review 
investment needed, I would not find it helpful for someone else to 
contribute a port.


Thanks,

-Joe

On 5/25/2020 9:24 AM, Raffaello Giulietti wrote:

Hi,

[1] enumerates 14 StrictMath native functions that still need a Java 
porting from the original C code.


As of changeset b84f122ea855 all of them are still native, so I wonder 
if it makes sense to contribute or if they have already been ported 
and on the launch pad for integration.


Let me know.


Greetings
Raffaello



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

Re: RFR(S) 8242504: Enhance the system clock to nanosecond precision

[David Holmes]

Thanks Dan!

David

On 28/05/2020 12:52 pm, Daniel D. Daugherty wrote:
I'll wait for your thumbs up on the explanation. 


I'm good with the explanation. Thanks!

Dan


On 5/27/20 10:08 PM, David Holmes wrote:

Hi Dan,

Thanks for taking a look.

On 28/05/2020 1:09 am, Daniel D. Daugherty wrote:

On 5/26/20 12:59 AM, David Holmes wrote:

bug: https://bugs.openjdk.java.net/browse/JDK-8242504
webrev: http://cr.openjdk.java.net/~dholmes/8242504/webrev/


src/hotspot/os/posix/os_posix.hpp
 No comments.

src/hotspot/os/posix/os_posix.inline.hpp
 No comments.

src/hotspot/os/posix/os_posix.cpp
 old L1686: #ifdef NEEDS_LIBRT
 old L1687   // Close librt if there is no monotonic clock.
 old L1688   if (handle != RTLD_DEFAULT) {
 old L1689 dlclose(handle);
 old L1690   }
 old L1691 #endif
 I don't see any explanation in the bug or in the CR invite
 about why this code is deleted when this preceding code
 remains:

 L1658:   // For older linux we need librt, for other OS we 
can find

 L1659:   // this function in regular libc.
 L1660: #ifdef NEEDS_LIBRT
 L1661:   // We do dlopen's in this particular order due to 
bug in linux
 L1662:   // dynamic loader (see 6348968) leading to crash on 
exit.

 L1663:   handle = dlopen("librt.so.1", RTLD_LAZY);
 L1664:   if (handle == NULL) {
 L1665:     handle = dlopen("librt.so", RTLD_LAZY);
 L1666:   }
 L1667: #endif


Sorry I should have mentioned this. In the existing code if we don't 
have CLOCK_MONOTONIC support we will never use the dynamic handles to 
clock_gettime and so we can close librt again (if we loaded it for 
clock_gettime).


With the new code we will always use clock_gettime so we can't unload 
librt (if we needed it for clock_gettime) just because there is no 
CLOCK_MONOTONIC.


The existing code (and thus the new code) is technically missing 
something here:


1675   if (clock_getres_func != NULL && clock_gettime_func != NULL) {
...
1693   }

There could be an else clause that closes librt if it was loaded. But 
the reality is that these functions are present in librt so we should 
never reach such an else clause. Closing the handle to librt is a 
minor "good citizen" act. This will all be moot in the not too distant 
future when we rely on these functions being in libc on all platforms.



src/hotspot/os/linux/os_linux.cpp
 old L1382:   return jlong(time.tv_sec) * 1000  + 
jlong(time.tv_usec / 1000);

 new L1390:     return jlong(time.tv_sec) * MILLIUNITS  +
 new L1391:    jlong(time.tv_usec) / (MICROUNITS / 
MILLIUNITS);


 old L1390:   nanos = jlong(time.tv_usec) * 1000;
 new L1407:     nanos = jlong(time.tv_usec) * (NANOUNITS / 
MICROUNITS);

 Thanks for replacing the literal 1000s in the old code.

test/jdk/java/time/test/java/time/TestClock_System.java
 L207:    + " millisecond precision: 
"+countBetterThanMillisPrecision+"/"+1000);
 L209:    + " microsecond precision: 
"+countBetterThanMicrosPrecision+"/"+1000);

    nit - need spaces around some of the '+' operators.


Fixed.

test/micro/org/openjdk/bench/java/lang/SystemTime.java 
test/micro/org/openjdk/bench/java/lang/Systems.java

 No comments.

My only "concern" is the deletion of closing the librt handle.
If you have a good explanation for that, then I'm good with this patch.


Thanks again. I'll wait for your thumbs up on the explanation.

David
-


Dan





This work was contributed by Mark Kralj-Taylor:

https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-April/038975.html 



On the hotspot side we change the Linux implementations of 
javaTimeMillis() and javaTimeSystemUTC() so that they use 
clock_gettime(CLOCK_REALTIME) instead of gettimeofday(). In keeping 
with our conditional use of this API I added a new guard


os::Posix::supports_clock_gettime()

and refined the existing supports_monotonic_clock() so that we can 
still use CLOCK_REALTIME if CLOCK_MONOTONIC does not exist. In the 
future (hopefully very near future) we will simply assume these APIs 
always exist.


On the core-libs side the existing test:

test/jdk/java/time/test/java/time/TestClock_System.java

is adjusted to track the precision in more detail.

Finally Mark has added instantNowAsEpochNanos() to the benchmark 
previously known as


test/micro/org/openjdk/bench/java/lang/Systems.java

which I've renamed to SystemTime.java as Mark suggested. I agree 
having these three functions measured together makes sense.


Testing:
  - test/jdk/java/time/test/java/time/TestClock_System.java
  - test/micro/org/openjdk/bench/java/lang/SystemTime.java
  - Tiers 1-3

Thanks,
David

Re: RFR: JDK-8245432: Lookup::defineHiddenClass should throw UnsupportedClassVersionError if the given bytes are of an unsupported major or minor version

[David Holmes]

Hi Mandy,

On 28/05/2020 2:13 pm, Mandy Chung wrote:

Updated webrev:
http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8245432/webrev.01/

I modify this patch to check the class file version and throws CFE if 
unsupported before creating ClassReader.  This also fixes JDK-8245061 
that it reads the value of `this_class` as a constant (as ASM will throw 
an exception if it's not a constant) and also ensures that `this_class` 
is a CONSTANT_Class_info by checking the descriptor string from Type.


I couldn't quite follow all the details so just a couple of comments:

 src/java.base/share/classes/jdk/internal/misc/VM.java

+ public static boolean isSupportedClassFileVersion(int major, int 
minor) {

+ if (major < 45 || major > classFileMajorVersion) return false;
+ if (major < 56) return minor == 0;
+ return minor == 0 || minor == PREVIEW_MINOR_VERSION;
+ }

that is only a partial validity test for preview versions - the major 
version must match the current version as well.


+ s = props.get("java.class.version");
+ int index = s.indexOf('.');
+ try {
+ classFileMajorVersion = Integer.valueOf(s.substring(0, 
index));
+ classFileMinorVersion = 
Integer.valueOf(s.substring(index+1, s.length()));

+ } catch (NumberFormatException e) {
+ throw new InternalError(e);
+ }

Can you not access java.lang.VersionProps to get the version info?

Cheers,
David


Mandy

On 5/27/20 10:57 AM, Mandy Chung wrote:
I'm reconsidering this fix along with JDK-8245061 that may require to 
do its own checking (a similar issue w.r.t. ASM validation but in this 
case the constant pool entry of `this_class` item is not validated).


thanks
Mandy

On 5/27/20 10:39 AM, fo...@univ-mlv.fr wrote:

Hi Alan,
We (the ASM team) recommend to our users to check the byte 6 (and 
perhaps 7) instead of relying on ASM throwing an exception,
because you may update the version of ASM but not the visitors your 
are using in your code.


It's less brittle than catching the IAE thrown by ASM.

Rémi

- Mail original -

De: "Alan Bateman" 
À: "mandy chung" , "core-libs-dev" 
, "Remi Forax"


Envoyé: Mercredi 27 Mai 2020 18:16:33
Objet: Re: RFR: JDK-8245432: Lookup::defineHiddenClass should throw 
UnsupportedClassVersionError if the given bytes are

of an unsupported major or minor version
On 26/05/2020 22:46, Mandy Chung wrote:

Lookup::defineHiddenClass currently throws IAE by ASM if the given
bytes are of unsupported class file version.  The implementation
should catch and throw UnsupportedClassVersionError instead.

webrev:
http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8245432/webrev.00/

This patch also includes a spec clarification of @throws IAE if the
the bytes has ACC_MODULE flag set to fix JDK-8245596.

Rémi - has there ever been any discussion in ASM about throwing more
specific exceptions? Only asking to see if we could avoid needing to
depend on the exception message here.

-Alan

8245867: Logger/bundleLeak/BundleTest.java fails due to "OutOfMemoryError: Java heap space"

[Daniel Fuchs]

Hi,

Please find an almost trivial fix for:

8245867: Logger/bundleLeak/BundleTest.java fails due
 to "OutOfMemoryError: Java heap space"
https://bugs.openjdk.java.net/browse/JDK-8245867

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8245867/webrev.00/

My new test for JDK-8239013 has been observed failing
intermittently in OutOfMemory. The test needs to trigger
the clearing of SoftReferences, and does so by eating
up heap memory in order to trigger a GC that will
clear them.
The issue is that the test didn't release the memory
when it no longer needed it, which caused trouble for
the test harness when it tried to clean up after the
test.

The fix is to use SoftReference to retain the eaten-up
memory (instead of strong references) so that it can
be reclaimed at the time the full GC that clear soft
references is triggered, and also to release the heap
memory as soon as it is no longer needed.

best regards,

-- daniel

Re: 8245867: Logger/bundleLeak/BundleTest.java fails due to "OutOfMemoryError: Java heap space"

[Lance Andersen]

Hi Daniel,

The change looks reasonable.  Thank you for addressing so quickly.

Best
Lance

> On May 28, 2020, at 4:50 AM, Daniel Fuchs  wrote:
> 
> Hi,
> 
> Please find an almost trivial fix for:
> 
> 8245867: Logger/bundleLeak/BundleTest.java fails due
> to "OutOfMemoryError: Java heap space"
> https://bugs.openjdk.java.net/browse/JDK-8245867
> 
> webrev:
> http://cr.openjdk.java.net/~dfuchs/webrev_8245867/webrev.00/
> 
> My new test for JDK-8239013 has been observed failing
> intermittently in OutOfMemory. The test needs to trigger
> the clearing of SoftReferences, and does so by eating
> up heap memory in order to trigger a GC that will
> clear them.
> The issue is that the test didn't release the memory
> when it no longer needed it, which caused trouble for
> the test harness when it tried to clean up after the
> test.
> 
> The fix is to use SoftReference to retain the eaten-up
> memory (instead of strong references) so that it can
> be reclaimed at the time the full GC that clear soft
> references is triggered, and also to release the heap
> memory as soon as it is no longer needed.
> 
> best regards,
> 
> -- daniel

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

RFR 8246050: Improve scalability of MemoryScope

[Maurizio Cimadamore]

Hi,
during the review of [1] it emerged that the implementation of the 
memory scope abstraction (which is used to keep track of temporal scope 
of a memory segment) does not scale well in situations where there is a 
lot of contention on the acquire() method due to many threads working 
simultaneously on different chunks of the segment.


Peter has proposed an alternate implementation [2] which, instead of 
using CAS, it cleverly uses LongAdders.


While that implementation worked correctly, we managed to simplify it 
further, by realizing that what we needed here was an instance of a 
read-write lock: a thread that acquires a segment does a "read", while a 
thread closing a segment does a "write". By using optimistic reads with 
a StampedLock we were able to gain back scalability and maintain the 
code relatively readable.


Webrev:

http://cr.openjdk.java.net/~mcimadamore/8246050/webrev/

Cheers
Maurizio

[1] - 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-April/066136.html

[2] - https://git.openjdk.java.net/panama-foreign/pull/142

Re: RFR: JDK-8245432: Lookup::defineHiddenClass should throw UnsupportedClassVersionError if the given bytes are of an unsupported major or minor version

[Mandy Chung]

On 5/28/20 6:55 AM, Alan Bateman wrote:

On 28/05/2020 05:13, Mandy Chung wrote:

Updated webrev:
http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8245432/webrev.01/

I modify this patch to check the class file version and throws CFE if 
unsupported before creating ClassReader.  This also fixes JDK-8245061 
that it reads the value of `this_class` as a constant (as ASM will 
throw an exception if it's not a constant) and also ensures that 
`this_class` is a CONSTANT_Class_info by checking the descriptor 
string from Type.
I don't want to complicate this further but the --enable-preview 
handling in the VM doesn't seem to expose an internal property that 
allows code in the libraries know if preview feature are enabled or 
not. I was assuming that isSupportedClassFileVersion would need to 
check that.




The runtime will ensure if --enable-preview is set if a class file with 
minor is loaded.   I will prefer to keep VM::isSupportedClassFileVersion 
to validate the given major/minor version.  At runtime, it will fail 
when such class file is loaded if preview is not enabled.


I'll add a comment to describe that.

Will readUnsignedShort throw AIOBE if the offset is beyond the length? 


Good catch.  I add the check to throw CFE.
+    private static int readUnsignedShort(byte[] bytes, int 
offset) {

+    if (offset >= bytes.length) {
+    throw new ClassFormatError("Invalid ClassFile 
structure");

+    }
+    return ((bytes[offset] & 0xFF) << 8) | (bytes[offset + 
1] & 0xFF);

+    }

Also are you sure about "& 0xCAFEBABE", I assumed it would just check 
the magic number is equal to that.


It should check ==.  Fixed.

thanks
Mandy

Re: RFR: JDK-8245432: Lookup::defineHiddenClass should throw UnsupportedClassVersionError if the given bytes are of an unsupported major or minor version

[Alan Bateman]

On 28/05/2020 05:13, Mandy Chung wrote:

Updated webrev:
http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8245432/webrev.01/

I modify this patch to check the class file version and throws CFE if 
unsupported before creating ClassReader.  This also fixes JDK-8245061 
that it reads the value of `this_class` as a constant (as ASM will 
throw an exception if it's not a constant) and also ensures that 
`this_class` is a CONSTANT_Class_info by checking the descriptor 
string from Type.
I don't want to complicate this further but the --enable-preview 
handling in the VM doesn't seem to expose an internal property that 
allows code in the libraries know if preview feature are enabled or not. 
I was assuming that isSupportedClassFileVersion would need to check that.


Will readUnsignedShort throw AIOBE if the offset is beyond the length? 
Also are you sure about "& 0xCAFEBABE", I assumed it would just check 
the magic number is equal to that.


-Alan.

Re: RFR: JDK-8225056 VM support for sealed classes

[Mandy Chung]

Hi Harold,

On 5/27/20 1:35 PM, Harold Seigel wrote:


Incremental webrev: 
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/


full webrev: 
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/



Class.java

4406 ReflectionData rd = reflectionData();
4407 ClassDesc[] tmp = rd.permittedSubclasses;
4408 if (tmp != null) {
4409 return tmp;
4410 }
4411
4412 if (isArray() || isPrimitive()) {
4413 rd.permittedSubclasses = new ClassDesc[0];
4414 return rd.permittedSubclasses;
4415 }


This causes an array class or primitive type to create a 
ReflectionData.   It should first check if this is non-sealed class and 
returns a constant empty array.


In fact, ReflectionData caches the derived names and reflected members 
for performance and also they may be invalidated when the class is 
redefined.   It might be okay to add ReflectionData::permittedSubclasses 
while `PermittedSubclasses` attribute can't be redefined and getting 
this attribute is not performance sensitive.   For example, the result 
of `getNestMembers` is not cached in ReflectionData.  It may be better 
not to add it in ReflectionData for modifiable and performance-sensitive 
data.



4421 tmp = new ClassDesc[subclassNames.length];
4422 int i = 0;
4423 for (String subclassName : subclassNames) {
4424 try {
4425 tmp[i++] = ClassDesc.of(subclassName.replace('/', '.'));
4426 } catch (IllegalArgumentException iae) {
4427 throw new InternalError("Invalid type in permitted subclasses 
information: " + subclassName, iae);

4428 }
4429 }

Nit: rename tmp to some other name e.g. descs

I read the JVMS but it isn't clear to me that the VM will validate the 
names in `PermittedSubclasses`attribute are valid class descriptors.   I 
see ConstantPool::is_klass_or_reference check but can't find where it 
validates the name is a valid class descriptor - can you point me 
there?   (otherwise, maybe define it to be unspecified?)



W.r.t. the APIs. I don't want to delay this review.  I see that you 
renamed the method to new API style as I brought up.  OTOH,  I expect 
more discussion is needed. Below is a recent comment from John on this 
topic [1]


One comment, really for the future, regarding the shape of the Java 
API here: It uses Optional and omits the "get" prefix on accessors. 
This is the New Style, as opposed to the Classic Style using null (for 
"empty" results) and a "get" prefix ("getComponentType") to get a 
related type. We may choose to to use the New Style for new reflection 
API points, and if so let's not choose N different New Styles, but one 
New Style. Personally I like removing "get"; I accept Optional instead 
of null; and I also suggest that arrays (if any) be replaced by 
(immutable) Lists in the New Style


There are a few existing Class APIs that use the new API style:
Class arrayClass();
Optional describeConstable();
String descriptorString();

This will set up a precedence of the new API style in this class. Should 
this new permittedSubclasses method return a List instead of an array?  
It's okay with me if you prefer to revert back to the old API style and 
follow this up after integration.


4442 * Returns true if this {@linkplain Class} is sealed.
4443 *
 * @return returns true if this class is sealed


NIt: {@code true} instead of true -  consistent with the style this 
class uses (in most methods)


test/jdk/java/lang/reflect/sealed_classes/SealedClassesReflectionTest.java

Nit: s/sealed_classes/sealedClasses/
- the test directory/file naming convention use camel case or java 
variable name convention.


Thanks
[1] https://github.com/openjdk/valhalla/pull/53#issuecomment-633116043

Re: RFR: JDK-8245432: Lookup::defineHiddenClass should throw UnsupportedClassVersionError if the given bytes are of an unsupported major or minor version

[Alan Bateman]

On 28/05/2020 18:25, Mandy Chung wrote:

:

The runtime will ensure if --enable-preview is set if a class file 
with minor is loaded.   I will prefer to keep 
VM::isSupportedClassFileVersion to validate the given major/minor 
version.  At runtime, it will fail when such class file is loaded if 
preview is not enabled.


I'll add a comment to describe that.

Will readUnsignedShort throw AIOBE if the offset is beyond the length? 


Good catch.  I add the check to throw CFE.
+    private static int readUnsignedShort(byte[] bytes, int 
offset) {

+    if (offset >= bytes.length) {
+    throw new ClassFormatError("Invalid ClassFile 
structure");

+    }
+    return ((bytes[offset] & 0xFF) << 8) | (bytes[offset 
+ 1] & 0xFF);

+    }

Also are you sure about "& 0xCAFEBABE", I assumed it would just check 
the magic number is equal to that.


It should check ==.  Fixed.

Thanks, this is okay with me.

-Alan.

RFR 8246095: Tweaks to memory access API

[Maurizio Cimadamore]

Hi,
this followup change includes a number of tweaks that have been added to 
the memory access API while we were in the process of integrating it. 
Most of them have been contributed by Chris (thanks!), and are all 
listed in great details in the CSR below:


https://bugs.openjdk.java.net/browse/JDK-8246096

Implementation-wise it should be all relatively straightforward - apart 
for the bit of surgery on lambda forms which was required to add a new 
kind of lambda forms to 'collect' return values. For now this method 
handle adaptation is package private, and can only be used by 
MemoryHandles::filterValues - but, if people find it useful, we might 
consider adding it to the MethodHandle standard combinator toolbox.


Cheers
Maurizio


Webrev:

http://cr.openjdk.java.net/~mcimadamore/8246095_v1/webrev/

Javadoc:

http://cr.openjdk.java.net/~mcimadamore/8246095_v1/javadoc/jdk/incubator/foreign/package-summary.html

Specdiff:

http://cr.openjdk.java.net/~mcimadamore/8246095_v1/specdiff/overview-summary.html

Re: RFR: JDK-8245432: Lookup::defineHiddenClass should throw UnsupportedClassVersionError if the given bytes are of an unsupported major or minor version

[Mandy Chung]

On 5/28/20 12:44 AM, David Holmes wrote:

Hi Mandy,

On 28/05/2020 2:13 pm, Mandy Chung wrote:

Updated webrev:
http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8245432/webrev.01/

I modify this patch to check the class file version and throws CFE if 
unsupported before creating ClassReader.  This also fixes JDK-8245061 
that it reads the value of `this_class` as a constant (as ASM will 
throw an exception if it's not a constant) and also ensures that 
`this_class` is a CONSTANT_Class_info by checking the descriptor 
string from Type.


I couldn't quite follow all the details so just a couple of comments:

 src/java.base/share/classes/jdk/internal/misc/VM.java

+ public static boolean isSupportedClassFileVersion(int major, int 
minor) {

+ if (major < 45 || major > classFileMajorVersion) return false;
+ if (major < 56) return minor == 0;
+ return minor == 0 || minor == PREVIEW_MINOR_VERSION;
+ }

that is only a partial validity test for preview versions - the major 
version must match the current version as well.


This is to validate the given version.  The runtime will check if 
preview feature is enabled when such class file is loaded.   I will make 
a comment to make it clear.


+ s = props.get("java.class.version");
+ int index = s.indexOf('.');
+ try {
+ classFileMajorVersion = Integer.valueOf(s.substring(0, 
index));
+ classFileMinorVersion = 
Integer.valueOf(s.substring(index+1, s.length()));

+ } catch (NumberFormatException e) {
+ throw new InternalError(e);
+ }

Can you not access java.lang.VersionProps to get the version info?


VersionProps is package-private.

Mandy

Cheers,
David


Mandy

On 5/27/20 10:57 AM, Mandy Chung wrote:
I'm reconsidering this fix along with JDK-8245061 that may require 
to do its own checking (a similar issue w.r.t. ASM validation but in 
this case the constant pool entry of `this_class` item is not 
validated).


thanks
Mandy

On 5/27/20 10:39 AM, fo...@univ-mlv.fr wrote:

Hi Alan,
We (the ASM team) recommend to our users to check the byte 6 (and 
perhaps 7) instead of relying on ASM throwing an exception,
because you may update the version of ASM but not the visitors your 
are using in your code.


It's less brittle than catching the IAE thrown by ASM.

Rémi

- Mail original -

De: "Alan Bateman" 
À: "mandy chung" , "core-libs-dev" 
, "Remi Forax"


Envoyé: Mercredi 27 Mai 2020 18:16:33
Objet: Re: RFR: JDK-8245432: Lookup::defineHiddenClass should 
throw UnsupportedClassVersionError if the given bytes are

of an unsupported major or minor version
On 26/05/2020 22:46, Mandy Chung wrote:

Lookup::defineHiddenClass currently throws IAE by ASM if the given
bytes are of unsupported class file version.  The implementation
should catch and throw UnsupportedClassVersionError instead.

webrev:
http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8245432/webrev.00/

This patch also includes a spec clarification of @throws IAE if the
the bytes has ACC_MODULE flag set to fix JDK-8245596.

Rémi - has there ever been any discussion in ASM about throwing more
specific exceptions? Only asking to see if we could avoid needing to
depend on the exception message here.

-Alan

Re: RFR 15 (S): 8245068: Implement Deprecation of RMI Activation

[Stuart Marks]

On 5/28/20 12:13 PM, Lance Andersen wrote:

Thinking about:

-
@deprecated The RMI Activation mechanism has been deprecated, and it may
+ * be removed from a future version.


Perhaps it might be a bit clearer to say “… from a future Java SE version”?  I 
realize that is different from what is at: 
https://docs.oracle.com/en/java/javase/14/docs/api/java.base/java/lang/Compiler.html, 
but I thought it makes it clearer as to what “version” is referring to.


Though that being said,  and now looking back at what I did for the CORBA and 
Java EE module removal, I used the same wording as you are proposing,  so all 
good… :-)


Yeah, it does seem like it's missing a noun, doesn't it? Using "Java SE" could 
work, but I note that Mark R edited the JEP 385 text and changed the places 
where I had written "Java SE" to "the Java Platform", so I think I'll run with 
that wording.


Thanks for reviewing the CSR and the release note.

s'marks





I've also drafted a CSR request and a release note. Please review:

CSR:

https://bugs.openjdk.java.net/browse/JDK-8245860


I think the CSR is fine.  I might suggest adding a comment to justify “low” 
for the compatibility impact for the JCK folks.


I added myself as a reviewer.


Release Note:

https://bugs.openjdk.java.net/browse/JDK-8246021


Looks fine.

Best,

 Lance


Thanks,

s'marks


On 5/26/20 12:35 PM, Stuart Marks wrote:

Hi all,
Here's the implementation of the recently-posted JEP 385, deprecation of RMI 
Activation for removal.
I'm listing this as S ("small") since conceptually it's fairly small, though 
there are rather a large number of files changed. Essentially the changes are:

 - java.rmi.activation package spec: add deprecation warning
 - java.rmi module spec: link to activation package spec
 - java.rmi.activation public classes and interfaces: deprecate
 - various other files: suppress warnings
 - Activation.java: have the rmid tool emit a deprecation warning
 - rmid.properties: localized warning message
Webrev:
http://cr.openjdk.java.net/~smarks/reviews/8245068/webrev.0/
Bug ID:
https://bugs.openjdk.java.net/browse/JDK-8245068
JEP:
http://openjdk.java.net/jeps/385
Thanks,
s'marks




Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR 8246050: Improve scalability of MemoryScope

[Paul Sandoz]

+1 (previously reviewed on the panama-dev list)

It’s very pleasing to see this get simplified through some good collaboration. 
StampedLock is quite powerful, and likely an under utilized resource.

Paul.

> On May 28, 2020, at 4:20 AM, Maurizio Cimadamore 
>  wrote:
> 
> Hi,
> during the review of [1] it emerged that the implementation of the memory 
> scope abstraction (which is used to keep track of temporal scope of a memory 
> segment) does not scale well in situations where there is a lot of contention 
> on the acquire() method due to many threads working simultaneously on 
> different chunks of the segment.
> 
> Peter has proposed an alternate implementation [2] which, instead of using 
> CAS, it cleverly uses LongAdders.
> 
> While that implementation worked correctly, we managed to simplify it 
> further, by realizing that what we needed here was an instance of a 
> read-write lock: a thread that acquires a segment does a "read", while a 
> thread closing a segment does a "write". By using optimistic reads with a 
> StampedLock we were able to gain back scalability and maintain the code 
> relatively readable.
> 
> Webrev:
> 
> http://cr.openjdk.java.net/~mcimadamore/8246050/webrev/
> 
> Cheers
> Maurizio
> 
> [1] - 
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-April/066136.html
> [2] - https://git.openjdk.java.net/panama-foreign/pull/142
> 

Re: RFR 15 (S): 8245068: Implement Deprecation of RMI Activation

[Stuart Marks]

I've updated the webrev a little bit in response to previous comments:

http://cr.openjdk.java.net/~smarks/reviews/8245068/webrev.1/

I've also drafted a CSR request and a release note. Please review:

CSR:

https://bugs.openjdk.java.net/browse/JDK-8245860

Release Note:

https://bugs.openjdk.java.net/browse/JDK-8246021

Thanks,

s'marks


On 5/26/20 12:35 PM, Stuart Marks wrote:

Hi all,

Here's the implementation of the recently-posted JEP 385, deprecation of RMI 
Activation for removal.


I'm listing this as S ("small") since conceptually it's fairly small, though 
there are rather a large number of files changed. Essentially the changes are:


  - java.rmi.activation package spec: add deprecation warning
  - java.rmi module spec: link to activation package spec
  - java.rmi.activation public classes and interfaces: deprecate
  - various other files: suppress warnings
  - Activation.java: have the rmid tool emit a deprecation warning
  - rmid.properties: localized warning message

Webrev:

     http://cr.openjdk.java.net/~smarks/reviews/8245068/webrev.0/

Bug ID:

     https://bugs.openjdk.java.net/browse/JDK-8245068

JEP:

     http://openjdk.java.net/jeps/385

Thanks,

s'marks

Re: RFR 8246050: Improve scalability of MemoryScope

[Martin Buchholz]

StampedLock iis a sharp knife, hard to use well, but an excellent tool
for low-level performance work.

On Thu, May 28, 2020 at 10:56 AM Paul Sandoz  wrote:
>
> +1 (previously reviewed on the panama-dev list)
>
> It’s very pleasing to see this get simplified through some good 
> collaboration. StampedLock is quite powerful, and likely an under utilized 
> resource.
>
> Paul.
>
> > On May 28, 2020, at 4:20 AM, Maurizio Cimadamore 
> >  wrote:
> >
> > Hi,
> > during the review of [1] it emerged that the implementation of the memory 
> > scope abstraction (which is used to keep track of temporal scope of a 
> > memory segment) does not scale well in situations where there is a lot of 
> > contention on the acquire() method due to many threads working 
> > simultaneously on different chunks of the segment.
> >
> > Peter has proposed an alternate implementation [2] which, instead of using 
> > CAS, it cleverly uses LongAdders.
> >
> > While that implementation worked correctly, we managed to simplify it 
> > further, by realizing that what we needed here was an instance of a 
> > read-write lock: a thread that acquires a segment does a "read", while a 
> > thread closing a segment does a "write". By using optimistic reads with a 
> > StampedLock we were able to gain back scalability and maintain the code 
> > relatively readable.
> >
> > Webrev:
> >
> > http://cr.openjdk.java.net/~mcimadamore/8246050/webrev/
> >
> > Cheers
> > Maurizio
> >
> > [1] - 
> > https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-April/066136.html
> > [2] - https://git.openjdk.java.net/panama-foreign/pull/142
> >
>

Re: RFR 15 (S): 8245068: Implement Deprecation of RMI Activation

[Lance Andersen]

Hi Stuart

I think your changes read fine.


> On May 28, 2020, at 1:34 PM, Stuart Marks  wrote:
> 
> I've updated the webrev a little bit in response to previous comments:
> 
>http://cr.openjdk.java.net/~smarks/reviews/8245068/webrev.1/

Thinking about:

-
@deprecated The RMI Activation mechanism has been deprecated, and it may
+ * be removed from a future version.


Perhaps it might be a bit clearer to say “… from a future Java SE version”?  I 
realize that is different from what is at: 
https://docs.oracle.com/en/java/javase/14/docs/api/java.base/java/lang/Compiler.html
 
,
 but I thought it makes it clearer as to what “version” is referring to.

Though that being said,  and now looking back at what I did for the CORBA and 
Java EE module removal, I used the same wording as you are proposing,  so all 
good… :-)

> 
> I've also drafted a CSR request and a release note. Please review:
> 
> CSR:
> 
>https://bugs.openjdk.java.net/browse/JDK-8245860

I think the CSR is fine.  I might suggest adding a comment to justify “low” for 
the compatibility impact for the JCK folks.

I added myself as a reviewer.
> 
> Release Note:
> 
>https://bugs.openjdk.java.net/browse/JDK-8246021

Looks fine.

Best,

 Lance
> 
> Thanks,
> 
> s'marks
> 
> 
> On 5/26/20 12:35 PM, Stuart Marks wrote:
>> Hi all,
>> Here's the implementation of the recently-posted JEP 385, deprecation of RMI 
>> Activation for removal.
>> I'm listing this as S ("small") since conceptually it's fairly small, though 
>> there are rather a large number of files changed. Essentially the changes 
>> are:
>>  - java.rmi.activation package spec: add deprecation warning
>>  - java.rmi module spec: link to activation package spec
>>  - java.rmi.activation public classes and interfaces: deprecate
>>  - various other files: suppress warnings
>>  - Activation.java: have the rmid tool emit a deprecation warning
>>  - rmid.properties: localized warning message
>> Webrev:
>> http://cr.openjdk.java.net/~smarks/reviews/8245068/webrev.0/
>> Bug ID:
>> https://bugs.openjdk.java.net/browse/JDK-8245068
>> JEP:
>> http://openjdk.java.net/jeps/385
>> Thanks,
>> s'marks

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR: JDK-8245432: Lookup::defineHiddenClass should throw UnsupportedClassVersionError if the given bytes are of an unsupported major or minor version

[David Holmes]

Hi Mandy,

On 29/05/2020 3:28 am, Mandy Chung wrote:



On 5/28/20 12:44 AM, David Holmes wrote:

Hi Mandy,

On 28/05/2020 2:13 pm, Mandy Chung wrote:

Updated webrev:
http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8245432/webrev.01/

I modify this patch to check the class file version and throws CFE if 
unsupported before creating ClassReader.  This also fixes JDK-8245061 
that it reads the value of `this_class` as a constant (as ASM will 
throw an exception if it's not a constant) and also ensures that 
`this_class` is a CONSTANT_Class_info by checking the descriptor 
string from Type.


I couldn't quite follow all the details so just a couple of comments:

 src/java.base/share/classes/jdk/internal/misc/VM.java

+ public static boolean isSupportedClassFileVersion(int major, int 
minor) {

+ if (major < 45 || major > classFileMajorVersion) return false;
+ if (major < 56) return minor == 0;
+ return minor == 0 || minor == PREVIEW_MINOR_VERSION;
+ }

that is only a partial validity test for preview versions - the major 
version must match the current version as well.


This is to validate the given version.  The runtime will check if 
preview feature is enabled when such class file is loaded.   I will make 
a comment to make it clear.


Okay but I thought the intent here was to pre-validate the version 
information so that when these bytes get passed to ASM you don't have to 
worry about the IAE that will be thrown by ASM if there is actually a 
problem.


Maybe the only real solution here is for ASM to be more specific with 
the exceptions it throws. :(




+ s = props.get("java.class.version");
+ int index = s.indexOf('.');
+ try {
+ classFileMajorVersion = Integer.valueOf(s.substring(0, 
index));
+ classFileMinorVersion = 
Integer.valueOf(s.substring(index+1, s.length()));

+ } catch (NumberFormatException e) {
+ throw new InternalError(e);
+ }

Can you not access java.lang.VersionProps to get the version info?


VersionProps is package-private.


Sure but we provide that kind of cross-package access all the time. We 
also have JAVA_MAX_SUPPORTED_VERSION in the ModuleInfo class. Seems 
messy to add yet a third place where we need to determine what the 
current major version number is.


That aside isn't the minor version, as set in java.class.version 
guaranteed to be zero?


David
-


Mandy

Cheers,
David


Mandy

On 5/27/20 10:57 AM, Mandy Chung wrote:
I'm reconsidering this fix along with JDK-8245061 that may require 
to do its own checking (a similar issue w.r.t. ASM validation but in 
this case the constant pool entry of `this_class` item is not 
validated).


thanks
Mandy

On 5/27/20 10:39 AM, fo...@univ-mlv.fr wrote:

Hi Alan,
We (the ASM team) recommend to our users to check the byte 6 (and 
perhaps 7) instead of relying on ASM throwing an exception,
because you may update the version of ASM but not the visitors your 
are using in your code.


It's less brittle than catching the IAE thrown by ASM.

Rémi

- Mail original -

De: "Alan Bateman" 
À: "mandy chung" , "core-libs-dev" 
, "Remi Forax"


Envoyé: Mercredi 27 Mai 2020 18:16:33
Objet: Re: RFR: JDK-8245432: Lookup::defineHiddenClass should 
throw UnsupportedClassVersionError if the given bytes are

of an unsupported major or minor version
On 26/05/2020 22:46, Mandy Chung wrote:

Lookup::defineHiddenClass currently throws IAE by ASM if the given
bytes are of unsupported class file version.  The implementation
should catch and throw UnsupportedClassVersionError instead.

webrev:
http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8245432/webrev.00/

This patch also includes a spec clarification of @throws IAE if the
the bytes has ACC_MODULE flag set to fix JDK-8245596.

Rémi - has there ever been any discussion in ASM about throwing more
specific exceptions? Only asking to see if we could avoid needing to
depend on the exception message here.

-Alan

Re: RFR: JDK-8245432: Lookup::defineHiddenClass should throw UnsupportedClassVersionError if the given bytes are of an unsupported major or minor version

[Johannes Kuhn]

Hi,

just noticed a small thing:
The magic is 4 bytes, but readUnsignedShort reads two bytes.

- Johannes

On 28-May-20 19:25, Mandy Chung wrote:



On 5/28/20 6:55 AM, Alan Bateman wrote:

On 28/05/2020 05:13, Mandy Chung wrote:

Updated webrev:
http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8245432/webrev.01/

I modify this patch to check the class file version and throws CFE 
if unsupported before creating ClassReader.  This also fixes 
JDK-8245061 that it reads the value of `this_class` as a constant 
(as ASM will throw an exception if it's not a constant) and also 
ensures that `this_class` is a CONSTANT_Class_info by checking the 
descriptor string from Type.
I don't want to complicate this further but the --enable-preview 
handling in the VM doesn't seem to expose an internal property that 
allows code in the libraries know if preview feature are enabled or 
not. I was assuming that isSupportedClassFileVersion would need to 
check that.




The runtime will ensure if --enable-preview is set if a class file 
with minor is loaded.   I will prefer to keep 
VM::isSupportedClassFileVersion to validate the given major/minor 
version.  At runtime, it will fail when such class file is loaded if 
preview is not enabled.


I'll add a comment to describe that.

Will readUnsignedShort throw AIOBE if the offset is beyond the length? 


Good catch.  I add the check to throw CFE.
+    private static int readUnsignedShort(byte[] bytes, int 
offset) {

+    if (offset >= bytes.length) {
+    throw new ClassFormatError("Invalid ClassFile 
structure");

+    }
+    return ((bytes[offset] & 0xFF) << 8) | (bytes[offset 
+ 1] & 0xFF);

+    }

Also are you sure about "& 0xCAFEBABE", I assumed it would just check 
the magic number is equal to that.


It should check ==.  Fixed.

thanks
Mandy

Re: RFR: JDK-8225056 VM support for sealed classes

[David Holmes]

Hi Harold,

Sorry Mandy's comment raised a couple of issues ...

On 29/05/2020 7:12 am, Mandy Chung wrote:

Hi Harold,

On 5/27/20 1:35 PM, Harold Seigel wrote:


Incremental webrev: 
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/


full webrev: 
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/



Class.java

4406 ReflectionData rd = reflectionData();
4407 ClassDesc[] tmp = rd.permittedSubclasses;
4408 if (tmp != null) {
4409 return tmp;
4410 }
4411
4412 if (isArray() || isPrimitive()) {
4413 rd.permittedSubclasses = new ClassDesc[0];
4414 return rd.permittedSubclasses;
4415 }

This causes an array class or primitive type to create a 
ReflectionData.   It should first check if this is non-sealed class 
and returns a constant empty array.


It can't check if this is a non-sealed class as the isSealed() check 
calls the above code! But for arrays and primitives which can't be 
sealed we should just do:


4412 if (isArray() || isPrimitive()) {
4413 return new ClassDesc[0];
4414 }

But this then made me realize that we need to be creating defensive 
copies of the returned arrays, as happens with other APIs that use 
ReflectionData.


Backing up a bit I complained that:

public boolean isSealed() {
return permittedSubclasses().length != 0;
}

is a very inefficient way to answer the question as to whether a class 
is sealed, so I suggested that the result of permittedSubclasses() be 
cached. Caching is not without its own issues as we are discovering, and 
when you add in defensive copies this seems to be trading one 
inefficiency for another. For nestmates we don't cache getNestMembers() 
because we don;t think it will be called often - it is there to complete 
the introspection API of Class rather than being anticipated as used in 
a regular programmatic sense. I expect the same is true for 
permittedSubclasses(). Do we expect isSealed() to be used often or is it 
too just there for completeness? If just for completeness then perhaps a 
VM query would be a better compromise on the efficiency front? Otherwise 
I can accept the current implementation of isSealed(), and a non-caching 
permittedClasses() for this initial implementation of sealed classes. If 
efficiency turns out to be a problem for isSealed() then we can revisit 
it then.


Thanks,
David


In fact, ReflectionData caches the derived names and reflected members 
for performance and also they may be invalidated when the class is 
redefined.   It might be okay to add 
ReflectionData::permittedSubclasses while `PermittedSubclasses` 
attribute can't be redefined and getting this attribute is not 
performance sensitive.   For example, the result of `getNestMembers` 
is not cached in ReflectionData.  It may be better not to add it in 
ReflectionData for modifiable and performance-sensitive data.



4421 tmp = new ClassDesc[subclassNames.length];
4422 int i = 0;
4423 for (String subclassName : subclassNames) {
4424 try {
4425 tmp[i++] = ClassDesc.of(subclassName.replace('/', '.'));
4426 } catch (IllegalArgumentException iae) {
4427 throw new InternalError("Invalid type in permitted subclasses 
information: " + subclassName, iae);

4428 }
4429 }
Nit: rename tmp to some other name e.g. descs

I read the JVMS but it isn't clear to me that the VM will validate the 
names in `PermittedSubclasses`attribute are valid class descriptors.   
I see ConstantPool::is_klass_or_reference check but can't find where 
it validates the name is a valid class descriptor - can you point me 
there?   (otherwise, maybe define it to be unspecified?)



W.r.t. the APIs. I don't want to delay this review.  I see that you 
renamed the method to new API style as I brought up.  OTOH,  I expect 
more discussion is needed. Below is a recent comment from John on this 
topic [1]


One comment, really for the future, regarding the shape of the Java 
API here: It uses Optional and omits the "get" prefix on accessors. 
This is the New Style, as opposed to the Classic Style using null 
(for "empty" results) and a "get" prefix ("getComponentType") to get 
a related type. We may choose to to use the New Style for new 
reflection API points, and if so let's not choose N different New 
Styles, but one New Style. Personally I like removing "get"; I accept 
Optional instead of null; and I also suggest that arrays (if any) be 
replaced by (immutable) Lists in the New Style


There are a few existing Class APIs that use the new API style:
Class arrayClass();
Optional describeConstable();
String descriptorString();

This will set up a precedence of the new API style in this class. 
Should this new permittedSubclasses method return a List instead of an 
array?  It's okay with me if you prefer to revert back to the old API 
style and follow this up after integration.


4442 * Returns true if this {@linkplain Class} is sealed.
4443 *
 * @return returns true if this class is sealed

NIt: {@code true} instead of true -  consistent with the style this 
class uses 

Re: RFR: JDK-8225056 VM support for sealed classes

[Mandy Chung]

Thanks for confirming it.

Mandy

On 5/28/20 3:31 PM, Harold Seigel wrote:


Hi Mandy,

The entries in the PermittedSubclasses attribute are constant pool 
ConstantClass_info entries.  These names get validated by the VM in 
this code in ClassFileParser::parse_constant_pool():


  for (index = 1; index < length; index++) {
    const jbyte tag = cp->tag_at(index).value();
    switch (tag) {
  case JVM_CONSTANT_UnresolvedClass: {
    const Symbol* const class_name = cp->klass_name_at(index);
    // check the name, even if _cp_patches will overwrite it
*verify_legal_class_name(class_name, CHECK);*
    break;
  }

Thanks, Harold


On 5/28/2020 5:12 PM, Mandy Chung wrote:
I read the JVMS but it isn't clear to me that the VM will validate 
the names in `PermittedSubclasses`attribute are valid class 
descriptors.   I see ConstantPool::is_klass_or_reference check but 
can't find where it validates the name is a valid class descriptor - 
can you point me there?   (otherwise, maybe define it to be unspecified?)

Re: RFR: JDK-8245432: Lookup::defineHiddenClass should throw UnsupportedClassVersionError if the given bytes are of an unsupported major or minor version

[Mandy Chung]

On 5/28/20 5:44 PM, David Holmes wrote:


This is to validate the given version.  The runtime will check if 
preview feature is enabled when such class file is loaded. I will 
make a comment to make it clear.


Okay but I thought the intent here was to pre-validate the version 
information so that when these bytes get passed to ASM you don't have 
to worry about the IAE that will be thrown by ASM if there is actually 
a problem.


Yes it is.  ASM does not check if preview features are enabled or not 
neither.  When a class file depending preview features is passed to VM, 
the VM will throw an exception if preview features are not enabled.


Maybe the only real solution here is for ASM to be more specific with 
the exceptions it throws. :(




This was discussed.
https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-May/066734.html



Sure but we provide that kind of cross-package access all the time. We 
also have JAVA_MAX_SUPPORTED_VERSION in the ModuleInfo class. Seems 
messy to add yet a third place where we need to determine what the 
current major version number is.




Ah, that's another place.  I think it's better to add 
VM::isSupportedModuleDescriptorVersion and remove these constants.


That aside isn't the minor version, as set in java.class.version 
guaranteed to be zero?


This is set at build time.   The minor version is zero for the current 
versioning scheme.


Mandy


David
-


Mandy

Cheers,
David


Mandy

On 5/27/20 10:57 AM, Mandy Chung wrote:
I'm reconsidering this fix along with JDK-8245061 that may require 
to do its own checking (a similar issue w.r.t. ASM validation but 
in this case the constant pool entry of `this_class` item is not 
validated).


thanks
Mandy

On 5/27/20 10:39 AM, fo...@univ-mlv.fr wrote:

Hi Alan,
We (the ASM team) recommend to our users to check the byte 6 (and 
perhaps 7) instead of relying on ASM throwing an exception,
because you may update the version of ASM but not the visitors 
your are using in your code.


It's less brittle than catching the IAE thrown by ASM.

Rémi

- Mail original -

De: "Alan Bateman" 
À: "mandy chung" , "core-libs-dev" 
, "Remi Forax"


Envoyé: Mercredi 27 Mai 2020 18:16:33
Objet: Re: RFR: JDK-8245432: Lookup::defineHiddenClass should 
throw UnsupportedClassVersionError if the given bytes are

of an unsupported major or minor version
On 26/05/2020 22:46, Mandy Chung wrote:

Lookup::defineHiddenClass currently throws IAE by ASM if the given
bytes are of unsupported class file version.  The implementation
should catch and throw UnsupportedClassVersionError instead.

webrev:
http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8245432/webrev.00/ 



This patch also includes a spec clarification of @throws IAE if 
the

the bytes has ACC_MODULE flag set to fix JDK-8245596.
Rémi - has there ever been any discussion in ASM about throwing 
more
specific exceptions? Only asking to see if we could avoid 
needing to

depend on the exception message here.

-Alan

Re: RFR: JDK-8245432: Lookup::defineHiddenClass should throw UnsupportedClassVersionError if the given bytes are of an unsupported major or minor version

[David Holmes]

On 29/05/2020 1:52 pm, Mandy Chung wrote:

On 5/28/20 5:44 PM, David Holmes wrote:


This is to validate the given version.  The runtime will check if 
preview feature is enabled when such class file is loaded. I will 
make a comment to make it clear.


Okay but I thought the intent here was to pre-validate the version 
information so that when these bytes get passed to ASM you don't have 
to worry about the IAE that will be thrown by ASM if there is actually 
a problem.


Yes it is.  ASM does not check if preview features are enabled or not 
neither.  When a class file depending preview features is passed to VM, 
the VM will throw an exception if preview features are not enabled.


Yes but will that VM exception propagate as-is, or will ASM catch it and 
turn it into IAE? If the latter then your original problem still exists.


Maybe the only real solution here is for ASM to be more specific with 
the exceptions it throws. :(




This was discussed.
https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-May/066734.html


Sure that is why you made the current change. But if ASM catches the VM 
exceptions and adapts them then your problem is not solved by 
pre-inspecting the major/minor version, unless you can implement a full 
validity check. Otherwise the only place this can be properly fixed is 
in ASM.


Sure but we provide that kind of cross-package access all the time. We 
also have JAVA_MAX_SUPPORTED_VERSION in the ModuleInfo class. Seems 
messy to add yet a third place where we need to determine what the 
current major version number is.




Ah, that's another place.  I think it's better to add 
VM::isSupportedModuleDescriptorVersion and remove these constants.


That aside isn't the minor version, as set in java.class.version 
guaranteed to be zero?


This is set at build time.   The minor version is zero for the current 
versioning scheme.


So you are allowing for someone arbitrarily setting 
DEFAULT_VERSION_CLASSFILE_MINOR in make/autoconf/version-numbers? I 
guess it doesn't hurt to be flexible, but seems unlikely anyone would 
want to mess with that. If this were VM code I'd just assert the minor 
version is 0, but Java assertions are much more heavyweight.


Thanks,
David


Mandy


David
-


Mandy

Cheers,
David


Mandy

On 5/27/20 10:57 AM, Mandy Chung wrote:
I'm reconsidering this fix along with JDK-8245061 that may require 
to do its own checking (a similar issue w.r.t. ASM validation but 
in this case the constant pool entry of `this_class` item is not 
validated).


thanks
Mandy

On 5/27/20 10:39 AM, fo...@univ-mlv.fr wrote:

Hi Alan,
We (the ASM team) recommend to our users to check the byte 6 (and 
perhaps 7) instead of relying on ASM throwing an exception,
because you may update the version of ASM but not the visitors 
your are using in your code.


It's less brittle than catching the IAE thrown by ASM.

Rémi

- Mail original -

De: "Alan Bateman" 
À: "mandy chung" , "core-libs-dev" 
, "Remi Forax"


Envoyé: Mercredi 27 Mai 2020 18:16:33
Objet: Re: RFR: JDK-8245432: Lookup::defineHiddenClass should 
throw UnsupportedClassVersionError if the given bytes are

of an unsupported major or minor version
On 26/05/2020 22:46, Mandy Chung wrote:

Lookup::defineHiddenClass currently throws IAE by ASM if the given
bytes are of unsupported class file version.  The implementation
should catch and throw UnsupportedClassVersionError instead.

webrev:
http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8245432/webrev.00/ 



This patch also includes a spec clarification of @throws IAE if 
the

the bytes has ACC_MODULE flag set to fix JDK-8245596.
Rémi - has there ever been any discussion in ASM about throwing 
more
specific exceptions? Only asking to see if we could avoid 
needing to

depend on the exception message here.

-Alan

Re: RFR: JDK-8225056 VM support for sealed classes

[Harold Seigel]

Hi Mandy,

The entries in the PermittedSubclasses attribute are constant pool 
ConstantClass_info entries.  These names get validated by the VM in this 
code in ClassFileParser::parse_constant_pool():


  for (index = 1; index < length; index++) {
    const jbyte tag = cp->tag_at(index).value();
    switch (tag) {
  case JVM_CONSTANT_UnresolvedClass: {
    const Symbol* const class_name = cp->klass_name_at(index);
    // check the name, even if _cp_patches will overwrite it
   *verify_legal_class_name(class_name, CHECK);*
    break;
  }

Thanks, Harold


On 5/28/2020 5:12 PM, Mandy Chung wrote:
I read the JVMS but it isn't clear to me that the VM will validate the 
names in `PermittedSubclasses`attribute are valid class descriptors.   
I see ConstantPool::is_klass_or_reference check but can't find where 
it validates the name is a valid class descriptor - can you point me 
there?   (otherwise, maybe define it to be unspecified?)

Re: 8245867: Logger/bundleLeak/BundleTest.java fails due to "OutOfMemoryError: Java heap space"

[David Holmes]

Hi Daniel,

This caught my eye ...

On 28/05/2020 6:50 pm, Daniel Fuchs wrote:

Hi,

Please find an almost trivial fix for:

8245867: Logger/bundleLeak/BundleTest.java fails due
  to "OutOfMemoryError: Java heap space"
https://bugs.openjdk.java.net/browse/JDK-8245867

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8245867/webrev.00/

My new test for JDK-8239013 has been observed failing
intermittently in OutOfMemory. The test needs to trigger
the clearing of SoftReferences, and does so by eating
up heap memory in order to trigger a GC that will
clear them.


This seems to be assuming that the GC will clear all SoftReferences when 
it needs to clear any SoftReference. I don't think that is at all 
guaranteed. In theory your memory eating loop could be satisfied by 
clearing only the SoftReference added in the previous iteration of the loop.


I would have expected the fix here to be simply to clear memory ie:

} catch (OutOfMemoryError oome) {
  stop = true;
  memory = null;
  System.gc();
}

Cheers,
David


The issue is that the test didn't release the memory
when it no longer needed it, which caused trouble for
the test harness when it tried to clean up after the
test.

The fix is to use SoftReference to retain the eaten-up
memory (instead of strong references) so that it can
be reclaimed at the time the full GC that clear soft
references is triggered, and also to release the heap
memory as soon as it is no longer needed.

best regards,

-- daniel