Re: RFR(S): 8241638: launcher time metrics alway report 1 on Linux when _JAVA_LAUNCHER_DEBUG set(Internet mail)

[臧琳]

Thanks Henry, 

I agree to leave Solaris as it is, it seems to me there is no strong reason to 
remove it.

So, Do I need more review before push this patch? 

And may I ask your help to push it if a go decision is made.

Thanks.
 

BRs,
Lin

On 2020/3/31, 12:30 PM, "Henry Jen"  wrote:

OK, I agree with David gettimeofday is an improvement than 1, so we can go 
ahead with the patch. Not sure if the caveat should be disclosed or not, I 
don’t think it’s important enough, just a known fact probably worth to leave a 
trace(perhaps a comment in code or the bug entry). As whether to change the 
ifdef to simply detect Solaris, I prefer just to leave it as is for two reasons:

1. It’s not broken, why change it?
2. I checked the code, it’s just a simply macro defined by the make file. 
Realtime Linux extension would have that function and special custom build can 
still use that, even though that probably is not happening.

Cheers,
Henry


> On Mar 30, 2020, at 7:39 PM, linzang(臧琳)  wrote:
> 
> Hi David, Henry, Alan,
>Thanks a lot for your review. 
>I have considered use clock_gettime() first, but I seached the code 
and found there is a marco SUPPORTS_CLOCK_MONOTONIC guard the usage of it. 
Which makes me think it may not be available under specific situation. So I 
choosed gettimeofday.  
>   Do you think the patch need to be refined to remove HAVE_GETHRTIME as 
Alan suggested?  Thanks. 
> 
> BRs,
> Lin
> 
> >On 2020/3/31, 8:05 AM, "David Holmes"  wrote:
>> 
>>   On 31/03/2020 4:08 am, Henry Jen wrote:
>>> Based on my understanding to gethrtime(), the main benefit is not to be 
affected by settimeofday or adjtime. I think it is probably better to use
>>> 
>>> clock_gettime(CLOCK_MONOTONIC_RAW, ts);
>>> 
>>> which I checked seems to be available on both Linux and Mac. Haven’t 
test it though.
>> 
>>   Not guaranteed to be available - either clock_gettime function or that 
>>   particular clock - at build time or runtime. We use a check in the 
build 
>>   system to determine build-time availability for hotspot, and then use 
>>   dl_lookup etc at runtime to determine if actually available. We should 
>>   be able to get rid of this one day but we checked fairly recently and 
>>   there were still some issues.
> 
>>   gettimeofday is a lot better than returning 1. Otherwise call into the 
>>   VM and use JVM_NanoTime.
>> 
>>   Cheers,
>>   David
>>   -
>> 
>>> Cheers,
>>> Henry
>>> 
 On Mar 30, 2020, at 1:37 AM, Alan Bateman  
wrote:
 
 On 30/03/2020 03:41, linzang(臧琳) wrote:
> Dear All,
>  May I ask your help to reivew this tiny patch? Thanks.
> 
> 
> 
> BRs,
> Lin
> 
> From: "linzang(臧琳)" 
> Date: Thursday, March 26, 2020 at 3:13 PM
> To: "core-libs-dev@openjdk.java.net" 
> Subject: RFR(S): 8241638: launcher time metrics alway report 1 on 
Linux when _JAVA_LAUNCHER_DEBUG set
> 
> Dear All,
> May I ask your help to review this tiny fix?
> Bug: https://bugs.openjdk.java.net/browse/JDK-8241638
> Webrev: http://cr.openjdk.java.net/~lzang/8241638/webrev01/
> Thanks!
> 
 Using gettimeofday on non-Solaris platforms seems reasonable here. The 
comment in the patch suggests Linux but it's other Unix builds too. Also just a 
minor nit that the code in java.base uses 4-space indent, not 2. Looking at the 
patch makes me wondering if we should remove HAVE_GETHRTIME as it seems to be 
only used on Solaris >and the launcher is already using #ifdef __solaris__ in 
several places. Henry, do you have any comments on this?
 
 -Alan
>>> 
> 
> 
> 

Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[Mandy Chung]

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

Re: RFR: 8238665: Add JFR event for direct memory statistics

[Denghui Dong]

Thanks!

Please help sponsor it if there is no other problem.

jdk/jfr and java/lang/management all passed in my Linux env.

Denghui Dong
--
From:Erik Gahlin 
Send Time:2020年4月1日(星期三) 01:01
To:董登辉(卓昂) 
Cc:Alan Bateman ; hotspot-jfr-dev 
; core-libs-dev 

Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics

Looks good. 

Erik

On 31 Mar 2020, at 18:22, Denghui Dong  wrote:
Hi Erik,

IMO, one event type per pool is a better choice, because:
1. easy filtering and configuration as you said, and I don't think there will 
be too many buffer pool types in the future
2. some buffer pools may have extra fields, e.g. direct memory has max capacity 
limit.

webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.05/
diff with webrev.04: http://cr.openjdk.java.net/~ddong/8238665/v4-v5.diff

please review it, thanks!
Denghui Dong
--
From:Erik Gahlin 
Send Time:2020年3月31日(星期二) 19:18
To:董登辉(卓昂) 
Cc:Alan Bateman ; hotspot-jfr-dev 
; core-libs-dev 

Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics

Hi Denghui,

I think there should be one event type per pool, or there should be a text 
field specifying which pool it is. 

If we believe there will be many pools in the future, it may make sense for a 
field, otherwise I think an event type could be used. The benefit of one event 
type per pool is that there can be a description per pool and it also allows 
easy filtering and configuration.

If there would be four pools, they could all derive from an abstract event 
class and only add @Label and @Description per event subclass. If there would 
be forty pools, it would be too much noise and a field would be better.

Erik

On 31 Mar 2020, at 11:06, Denghui Dong  wrote:
PING.

@Erik Could you review it? 

Denghui Dong



--
From:董登辉(卓昂) 
Send Time:2020年3月19日(星期四) 16:35
To:Alan Bateman ; Erik Gahlin 
Cc:hotspot-jfr-dev ; core-libs-dev 

Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics

Hi,

On Mar 18, 2020, at 11:46 PM, Alan Bateman  wrote:


On 13/03/2020 14:54, Denghui Dong wrote:
Good suggestion, moved.
Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.03/ 
 This looks much better.

 What would you think about renaming the JFR event to "DirectBufferStatistics"? 
The concern I have with the proposed naming is that it will be really awkward 
to extend it to support mapped buffers.

It’s ok for me.


 The implementation changes look okay, hopefully Erik will skim over them. One 
small suggestion for for ManagementFactoryHelper is that you can 
stream().collect(Collectors.toList()) to create the value for bufferPools. You 
could even change it to volatile so that getBufferMXBeans isn't a synchronized 
method.


Make sense, updated.

Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.04/

@Erik, could you help review it?


 -Alan.

Denghui Dong

Re: JDK 15 RF(pre)R of JDK-8241374: add Math.absExact

[Joe Darcy]

Hi Tagir,

On 3/30/2020 9:00 PM, Tagir Valeev wrote:

Hello!

Speaking about implementation, how about this?

return (a < 0) ? negateExact(a) : a;

This would require only one branch for positive numbers and also will
utilize already intrinsified negateExact. The cost is a less verbose
message ("integer overflow" or "long overflow"). However, when one
sees the exception from absExact, its meaning becomes completely clear
after reading the spec.


Given that the spec may not always be handy, I prefer the more precise 
exception message given in the current, more verbose, implementation.


Thanks for the suggestion,

-Joe



With best regards,
Tagir Valeev.

On Tue, Mar 31, 2020 at 12:54 AM Joe Darcy  wrote:

Hello,

Updated webrev at:

  http://cr.openjdk.java.net/~darcy/8241374.1/

One of the four pieces of the update to discuss below. A few comments, I
added links from the abs methods to its sibling absExact method along
with an additional note on referring the differing absExact behavior on
a MIN_VALUE argument. I'll reflow the paragraphs and update the
copyright year before pushing. Link to the relevant JLS section included
in the absExact javadoc.

--- old/src/java.base/share/classes/java/lang/Math.java 2020-03-30
10:28:41.314472163 -0700
+++ new/src/java.base/share/classes/java/lang/Math.java 2020-03-30
10:28:40.930472163 -0700
@@ -1359,9 +1359,12 @@
* {@link Integer#MIN_VALUE}, the most negative representable
* {@code int} value, the result is that same value, which is
* negative.
+ * In contrast, the {@link Math#absExact(int)} method throws an
+ * {@code ArithmeticException} for this value.
*
* @param   a   the argument whose absolute value is to be determined
* @return  the absolute value of the argument.
+ * @see Math#absExact(int)
*/
   @HotSpotIntrinsicCandidate
   public static int abs(int a) {
@@ -1369,6 +1372,32 @@
   }

   /**
+ * Returns the mathematical absolute value of an {@code int} value
+ * if it is exactly representable as an {@code int}, throwing
+ * {@code ArithmeticException} if the result overflows the
+ * positive {@code int} range.
+ *
+ * Since the range of two's complement integers is asymmetric
+ * with one additional negative value (JLS {@jls 4.2.1}), the
+ * mathematical absolute value of {@link Integer#MIN_VALUE}
+ * overflows the positive {@code int} range, so an exception is
+ * thrown for that argument.
+ *
+ * @param  a  the argument whose absolute value is to be determined
+ * @return the absolute value of the argument, unless overflow occurs
+ * @throws ArithmeticException if the argument is {@link
Integer#MIN_VALUE}
+ * @see Math#abs(int)
+ * @since 15
+ */
+public static int absExact(int a) {
+if (a == Integer.MIN_VALUE)
+throw new ArithmeticException(
+"Overflow to represent absolute value of
Integer.MIN_VALUE");
+else
+return abs(a);
+}

For the StrictMath methods, I include links to the Math equivalent, as
the other fooExact StrictMath methods do.

Thanks,

-Joe


On 3/30/2020 9:31 AM, Joe Darcy wrote:

Hi Brian and Chris,

I was considering adding such as JLS link myself; I'll work one in and
post a revised webrev.

Thanks,

-Joe

On 3/30/2020 9:29 AM, Brian Burkhalter wrote:

Hi Joe,


On Mar 30, 2020, at 6:31 AM, Chris Hegarty mailto:chris.hega...@oracle.com>> wrote:


Full webrev for review include including tests:

http://cr.openjdk.java.net/~darcy/8241374.0/

and the companion CSR:

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

Looks good to me.

Likewise.


I do like the explanatory paragraph about the asymmetry in the range
of two's complement integer values. An alternative, or addition,
would be a link to JLS 4.2.1 - Integral Types and Values [1], which
shows the asymmetry from a language perspective.

I also like this suggestion.

Brian


-Chris.

[1]https://docs.oracle.com/javase/specs/jls/se14/html/jls-4.html#jls-4.2.1

Re: [15] RFR: 8214694: cleanup rawtypes warnings in open jndi tests

[Chris Yin]

Thank you, Joe

Regards,
Chris

> On 31 Mar 2020, at 11:49 PM, Joe Darcy  wrote:
> 
> Hi Chris,
> 
> Revised version looks better; thanks,
> 
> -Joe
> 
> On 3/27/2020 1:30 AM, Chris Yin wrote:
>> Hi, Joe
>> 
>> Thank you for reviewing and comments.
>> 
>> I agree that some part of the change could be as precise as they could be, 
>> and yes, after fix them, a round of redundant “cast” warnings just show up 
>> as you mentioned, both fixed now
>> For rest of them, I could see most are due to the API return value or 
>> parameter are imprecise (such as Class.forName, Context.getEnvironment etc), 
>> to avoid useless cast, just kept the same declaration, please kindly advise 
>> if you have any suggestion about that too, thanks. Updated webrev as below
>> 
>> http://cr.openjdk.java.net/~xyin/8214694/webrev.01/
>> 
>> Regards,
>> Chris
>> 
>>> On 27 Mar 2020, at 4:41 AM, Joe Darcy  wrote:
>>> 
>>> Hi Chris,
>>> 
>>> The changes don't appear incorrect, but at least from a cursory inspection, 
>>> they don't appear as precise as they could be. Usually generifying a 
>>> classes like this will next yield a round of redundant "cast" warnings; in 
>>> my cleanup efforts, getting cast warnings was usually a sign the right 
>>> generifiation was found.
>>> 
>>> HTH,
>>> 
>>> -Joe
>>> 
>>> On 3/25/2020 12:57 AM, Chris Yin wrote:
 Hello
 
 Please review following simple changes to cleanup raw types warning for 
 open jndi tests (under test/jdk/com/sun/jndi and test/jdk/javax/naming), 
 thanks
 
 Bug: https://bugs.openjdk.java.net/browse/JDK-8214694
 Webrev: http://cr.openjdk.java.net/~xyin/8214694/webrev.00/
 
 
 The changes should be straightforward, only fix raw types warnings, no 
 test logic change, no code optimization or cleanup. Minor change to each 
 test file, just a little surprised about the affected tests count, hope 
 this covers all. Run related jndi tests on 4 platforms for total 200 
 times, all passed.
 
 Thanks,
 Chris

Re: (XS) Review Request JDK-8241964: Clean up java.lang.Class javadoc

[Mandy Chung]

Thanks Joe for the quick review.

Mandy

On 3/31/20 4:22 PM, Joe Darcy wrote:

Looks fine Mandy; thanks for doing this cleanup,

-Joe

On 3/31/2020 4:15 PM, Mandy Chung wrote:
 This patch cleans up the javadoc in java.lang.Class and replaces 
"this object" and "this class object" with "this {@code Class} object"


Webrev at:
http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8241964/webrev.00/

thanks
Mandy

Re: (XS) Review Request JDK-8241964: Clean up java.lang.Class javadoc

[Joe Darcy]

Looks fine Mandy; thanks for doing this cleanup,

-Joe

On 3/31/2020 4:15 PM, Mandy Chung wrote:
 This patch cleans up the javadoc in java.lang.Class and replaces 
"this object" and "this class object" with "this {@code Class} object"


Webrev at:
http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8241964/webrev.00/

thanks
Mandy

(XS) Review Request JDK-8241964: Clean up java.lang.Class javadoc

[Mandy Chung]

 This patch cleans up the javadoc in java.lang.Class and replaces "this 
object" and "this class object" with "this {@code Class} object"


Webrev at:
http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8241964/webrev.00/

thanks
Mandy

Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[Mandy Chung]

Alex's feedback:  rename isHiddenClass to isHidden as it can be a hidden 
class or interface.


`isLocalClass` and `sAnonymousClass` are correct because the Java 
language only has local classes and anon classes, not local interfaces 
or anon. interfaces.  `isHidden` is like `isSynthetic`, it could be a 
class or interface.


Although isHiddenClass seems clearer, I'm okay to rename it to `isHidden`.

Mandy

On 3/31/20 11:06 AM, Mandy Chung wrote:

This patch addresses Joe's feedback on the CSR [1]:

http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03-delta-jdarcy/ 



Specifically, it adds to the class specification of java.lang.Class to 
describe how the relevant methods behave for hidden classes.  In 
addition, use the new inline @jvms tag.


Thanks
Mandy
[1] https://bugs.openjdk.java.net/browse/JDK-8238359

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 



Hidden class is created via `Lookup::defineHiddenClass`. From JVM's 
point

of view, a hidden class is a normal class except the following:

- A hidden class has no initiating class loader and is not registered 
in any dictionary.
- A hidden class has a name containing an illegal character 
`Class::getName` returns `p.Foo/0x1234` whereas `GetClassSignature` 
returns "Lp/Foo.0x1234;".
- A hidden class is not modifiable, i.e. cannot be redefined or 
retransformed. JVM TI IsModifableClass returns false on a hidden.
- Final fields in a hidden class is "final".  The value of final 
fields cannot be overriden via reflection.  setAccessible(true) can 
still be called on reflected objects representing final fields in a 
hidden class and its access check will be suppressed but only have 
read-access (i.e. can do Field::getXXX but not setXXX).


Brief summary of this patch:

1. A new Lookup::defineHiddenClass method is the API to create a 
hidden class.
2. A new Lookup.ClassOption enum class defines NESTMATE and STRONG 
option that

   can be specified when creating a hidden class.
3. A new Class::isHiddenClass method tests if a class is a hidden class.
4. Field::setXXX method will throw IAE on a final field of a hidden 
class

   regardless of the value of the accessible flag.
5. JVM_LookupDefineClass is the new JVM entry point for 
Lookup::defineClass

   and defineHiddenClass to create a class from the given bytes.
6. ClassLoaderData implementation is not changed.  There is one 
primary CLD
   that holds the classes strongly referenced by its defining 
loader.  There

   can be zero or more additional CLDs - one per weak class.
7. Nest host determination is updated per revised JVMS 5.4.4. Access 
control
   check no longer throws LinkageError but instead it will throw IAE 
with
   a clear message if a class fails to resolve/validate the nest host 
declared

   in NestHost/NestMembers attribute.
8. JFR, jcmd, JDI are updated to support hidden classes.
9. update javac LambdaToMethod as lambda proxy starts using nestmates
   and generate a bridge method to desuger a method reference to a 
protected

   method in its supertype in a different package

This patch also updates StringConcatFactory, LambdaMetaFactory, and 
LambdaForms
to use hidden classes.  The webrev includes changes in nashorn to 
hidden class

and I will update the webrev if JEP 372 removes it any time soon.

We uncovered a bug in Lookup::defineClass spec throws LinkageError 
and intends
to have the newly created class linked.  However, the implementation 
in 14

does not link the class.  A separate CSR [2] proposes to update the
implementation to match the spec.  This patch fixes the implementation.

The spec update on JVM TI, JDI and Instrumentation will be done as
a separate RFE [3].  This patch includes new tests for JVM TI and
java.instrument that validates how the existing APIs work for hidden 
classes.


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

RE: RFR (S): 8241947: Minor comment fixes for system property handling

[Langer, Christoph]

Hi Mandy,

this is a good suggestion. The listing of system properties at the props field 
declaration seems somewhat redundant, given that it already exists more exactly 
and with API normativity in the doc of System::getProperties().

So what do you think of this version: 
http://cr.openjdk.java.net/~clanger/webrevs/8241947.1/ ?

Thanks
Christoph

From: Mandy Chung 
Sent: Dienstag, 31. März 2020 19:34
To: Langer, Christoph ; core-libs-dev@openjdk.java.net
Cc: build-dev 
Subject: Re: RFR (S): 8241947: Minor comment fixes for system property handling


On 3/31/20 7:56 AM, Langer, Christoph wrote:

Hi,



please review a small fix that updates two comments.



The first one, in make/autoconf/spec.gmk.in, is probably quite old. It talks 
about handling of a property "vm.vendor" in VersionProps.java.template. 
However, there is no property "vm.vendor", it must rather be "java.vendor". I 
stumbled over that when looking at the change of JDK-4947890 (Minimize JNI 
upcalls in system-properties initialization).



The second one is the code comment attached to "private static Properties 
props;" in java.lang.System. After JDK-8197927 (Allow the system property 
`java.vendor.version` to be undefined), "java.vendor.version" can be undefined, 
so this should be reflected in the comment. I also took the liberty to remove 
an unneeded import statement.



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

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8241947.0/



I wonder if we still want to keep this block of comments listing a subset of 
system properties.  Anyway the minor comment looks okay.

Mandy

RE: RFR (S): 8241947: Minor comment fixes for system property handling

[Langer, Christoph]

Hi Magnus,

>  From a build perspective this looks fine.

Thanks for the review.

> But it seems you are changing the interface for java.lang.System. Don't
> you need a CSR for that? Or is your claim that the interface was indeed
> changed by JDK-8197927, and it is a bug that the documentation was not
> updated to match this?

Yes, the second is true. The interface/spec was already changed with 
JDK-8197927 which also has a CSR attached. The commit is this: 
http://hg.openjdk.java.net/jdk/jdk/rev/d80becbcd3c1.
What I'm modifying here is only the non API relevant doc of the private field 
props. The actual interface is documented in the Javadoc of method 
java.lang.System::getProperties().

Cheers
Christoph

Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[Mandy Chung]

This patch addresses Joe's feedback on the CSR [1]:

http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03-delta-jdarcy/

Specifically, it adds to the class specification of java.lang.Class to 
describe how the relevant methods behave for hidden classes.  In 
addition, use the new inline @jvms tag.


Thanks
Mandy
[1] https://bugs.openjdk.java.net/browse/JDK-8238359

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 



Hidden class is created via `Lookup::defineHiddenClass`. From JVM's point
of view, a hidden class is a normal class except the following:

- A hidden class has no initiating class loader and is not registered 
in any dictionary.
- A hidden class has a name containing an illegal character 
`Class::getName` returns `p.Foo/0x1234` whereas `GetClassSignature` 
returns "Lp/Foo.0x1234;".
- A hidden class is not modifiable, i.e. cannot be redefined or 
retransformed. JVM TI IsModifableClass returns false on a hidden.
- Final fields in a hidden class is "final".  The value of final 
fields cannot be overriden via reflection.  setAccessible(true) can 
still be called on reflected objects representing final fields in a 
hidden class and its access check will be suppressed but only have 
read-access (i.e. can do Field::getXXX but not setXXX).


Brief summary of this patch:

1. A new Lookup::defineHiddenClass method is the API to create a 
hidden class.
2. A new Lookup.ClassOption enum class defines NESTMATE and STRONG 
option that

   can be specified when creating a hidden class.
3. A new Class::isHiddenClass method tests if a class is a hidden class.
4. Field::setXXX method will throw IAE on a final field of a hidden class
   regardless of the value of the accessible flag.
5. JVM_LookupDefineClass is the new JVM entry point for 
Lookup::defineClass

   and defineHiddenClass to create a class from the given bytes.
6. ClassLoaderData implementation is not changed.  There is one 
primary CLD
   that holds the classes strongly referenced by its defining loader.  
There

   can be zero or more additional CLDs - one per weak class.
7. Nest host determination is updated per revised JVMS 5.4.4. Access 
control

   check no longer throws LinkageError but instead it will throw IAE with
   a clear message if a class fails to resolve/validate the nest host 
declared

   in NestHost/NestMembers attribute.
8. JFR, jcmd, JDI are updated to support hidden classes.
9. update javac LambdaToMethod as lambda proxy starts using nestmates
   and generate a bridge method to desuger a method reference to a 
protected

   method in its supertype in a different package

This patch also updates StringConcatFactory, LambdaMetaFactory, and 
LambdaForms
to use hidden classes.  The webrev includes changes in nashorn to 
hidden class

and I will update the webrev if JEP 372 removes it any time soon.

We uncovered a bug in Lookup::defineClass spec throws LinkageError and 
intends
to have the newly created class linked.  However, the implementation 
in 14

does not link the class.  A separate CSR [2] proposes to update the
implementation to match the spec.  This patch fixes the implementation.

The spec update on JVM TI, JDI and Instrumentation will be done as
a separate RFE [3].  This patch includes new tests for JVM TI and
java.instrument that validates how the existing APIs work for hidden 
classes.


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

Re: RFR (S): 8241947: Minor comment fixes for system property handling

[Mandy Chung]

On 3/31/20 7:56 AM, Langer, Christoph wrote:

Hi,

please review a small fix that updates two comments.

The first one, in make/autoconf/spec.gmk.in, is probably quite old. It talks about handling of a property 
"vm.vendor" in VersionProps.java.template. However, there is no property "vm.vendor", it 
must rather be "java.vendor". I stumbled over that when looking at the change of JDK-4947890 
(Minimize JNI upcalls in system-properties initialization).

The second one is the code comment attached to "private static Properties props;" in 
java.lang.System. After JDK-8197927 (Allow the system property `java.vendor.version` to be 
undefined), "java.vendor.version" can be undefined, so this should be reflected in the 
comment. I also took the liberty to remove an unneeded import statement.

Bug: https://bugs.openjdk.java.net/browse/JDK-8241947
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8241947.0/



I wonder if we still want to keep this block of comments listing a 
subset of system properties.  Anyway the minor comment looks okay.


Mandy

Re: RFR: 8238665: Add JFR event for direct memory statistics

[Erik Gahlin]

Looks good. 

Erik

> On 31 Mar 2020, at 18:22, Denghui Dong  wrote:
> 
> Hi Erik,
> 
> IMO, one event type per pool is a better choice, because:
> 1. easy filtering and configuration as you said, and I don't think there will 
> be too many buffer pool types in the future
> 2. some buffer pools may have extra fields, e.g. direct memory has max 
> capacity limit.
> 
> webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.05/ 
> 
> diff with webrev.04: http://cr.openjdk.java.net/~ddong/8238665/v4-v5.diff 
> 
> 
> please review it, thanks!
> 
> Denghui Dong
> --
> From:Erik Gahlin 
> Send Time:2020年3月31日(星期二) 19:18
> To:董登辉(卓昂) 
> Cc:Alan Bateman ; hotspot-jfr-dev 
> ; core-libs-dev 
> 
> Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics
> 
> Hi Denghui,
> 
> I think there should be one event type per pool, or there should be a text 
> field specifying which pool it is. 
> 
> If we believe there will be many pools in the future, it may make sense for a 
> field, otherwise I think an event type could be used. The benefit of one 
> event type per pool is that there can be a description per pool and it also 
> allows easy filtering and configuration.
> 
> If there would be four pools, they could all derive from an abstract event 
> class and only add @Label and @Description per event subclass. If there would 
> be forty pools, it would be too much noise and a field would be better.
> 
> Erik
> 
> On 31 Mar 2020, at 11:06, Denghui Dong  > wrote:
> 
> PING.
> 
> @Erik Could you review it? 
> 
> Denghui Dong
> 
> 
> 
> --
> From:董登辉(卓昂)  >
> Send Time:2020年3月19日(星期四) 16:35
> To:Alan Bateman mailto:alan.bate...@oracle.com>>; 
> Erik Gahlin mailto:erik.gah...@oracle.com>>
> Cc:hotspot-jfr-dev  >; core-libs-dev 
> mailto:core-libs-dev@openjdk.java.net>>
> Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics
> 
> Hi,
> 
> On Mar 18, 2020, at 11:46 PM, Alan Bateman  > wrote:
> 
> 
> 
> On 13/03/2020 14:54, Denghui Dong wrote:
> Good suggestion, moved.
> Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.03/ 
> 
> This looks much better.
> 
> What would you think about renaming the JFR event to 
> "DirectBufferStatistics"? The concern I have with the proposed naming is that 
> it will be really awkward to extend it to support mapped buffers.
> 
> It’s ok for me.
> 
> 
> The implementation changes look okay, hopefully Erik will skim over them. One 
> small suggestion for for ManagementFactoryHelper is that you can 
> stream().collect(Collectors.toList()) to create the value for bufferPools. 
> You could even change it to volatile so that getBufferMXBeans isn't a 
> synchronized method.
> 
> 
> Make sense, updated.
> 
> Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.04/ 
> 
> 
> @Erik, could you help review it?
> 
> 
> -Alan.
> 
> Denghui Dong
> 

Re: RFR: 8238665: Add JFR event for direct memory statistics

[Denghui Dong]

Hi Erik,

IMO, one event type per pool is a better choice, because:
1. easy filtering and configuration as you said, and I don't think there will 
be too many buffer pool types in the future
2. some buffer pools may have extra fields, e.g. direct memory has max capacity 
limit.

webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.05/
diff with webrev.04: http://cr.openjdk.java.net/~ddong/8238665/v4-v5.diff

please review it, thanks!

Denghui Dong
--
From:Erik Gahlin 
Send Time:2020年3月31日(星期二) 19:18
To:董登辉(卓昂) 
Cc:Alan Bateman ; hotspot-jfr-dev 
; core-libs-dev 

Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics

Hi Denghui,

I think there should be one event type per pool, or there should be a text 
field specifying which pool it is. 

If we believe there will be many pools in the future, it may make sense for a 
field, otherwise I think an event type could be used. The benefit of one event 
type per pool is that there can be a description per pool and it also allows 
easy filtering and configuration.

If there would be four pools, they could all derive from an abstract event 
class and only add @Label and @Description per event subclass. If there would 
be forty pools, it would be too much noise and a field would be better.

Erik

On 31 Mar 2020, at 11:06, Denghui Dong  wrote:
PING.

@Erik Could you review it? 

Denghui Dong



--
From:董登辉(卓昂) 
Send Time:2020年3月19日(星期四) 16:35
To:Alan Bateman ; Erik Gahlin 
Cc:hotspot-jfr-dev ; core-libs-dev 

Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics

Hi,

On Mar 18, 2020, at 11:46 PM, Alan Bateman  wrote:


On 13/03/2020 14:54, Denghui Dong wrote:
Good suggestion, moved.
Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.03/ 
 This looks much better.

 What would you think about renaming the JFR event to "DirectBufferStatistics"? 
The concern I have with the proposed naming is that it will be really awkward 
to extend it to support mapped buffers.

It’s ok for me.


 The implementation changes look okay, hopefully Erik will skim over them. One 
small suggestion for for ManagementFactoryHelper is that you can 
stream().collect(Collectors.toList()) to create the value for bufferPools. You 
could even change it to volatile so that getBufferMXBeans isn't a synchronized 
method.


Make sense, updated.

Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.04/

@Erik, could you help review it?


 -Alan.

Denghui Dong

Re: [15] RFR: 8214694: cleanup rawtypes warnings in open jndi tests

[Joe Darcy]

Hi Chris,

Revised version looks better; thanks,

-Joe

On 3/27/2020 1:30 AM, Chris Yin wrote:

Hi, Joe

Thank you for reviewing and comments.

I agree that some part of the change could be as precise as they could be, and 
yes, after fix them, a round of redundant “cast” warnings just show up as you 
mentioned, both fixed now
For rest of them, I could see most are due to the API return value or parameter 
are imprecise (such as Class.forName, Context.getEnvironment etc), to avoid 
useless cast, just kept the same declaration, please kindly advise if you have 
any suggestion about that too, thanks. Updated webrev as below

http://cr.openjdk.java.net/~xyin/8214694/webrev.01/

Regards,
Chris


On 27 Mar 2020, at 4:41 AM, Joe Darcy  wrote:

Hi Chris,

The changes don't appear incorrect, but at least from a cursory inspection, they don't 
appear as precise as they could be. Usually generifying a classes like this will next 
yield a round of redundant "cast" warnings; in my cleanup efforts, getting cast 
warnings was usually a sign the right generifiation was found.

HTH,

-Joe

On 3/25/2020 12:57 AM, Chris Yin wrote:

Hello

Please review following simple changes to cleanup raw types warning for open 
jndi tests (under test/jdk/com/sun/jndi and test/jdk/javax/naming), thanks

Bug: https://bugs.openjdk.java.net/browse/JDK-8214694
Webrev: http://cr.openjdk.java.net/~xyin/8214694/webrev.00/


The changes should be straightforward, only fix raw types warnings, no test 
logic change, no code optimization or cleanup. Minor change to each test file, 
just a little surprised about the affected tests count, hope this covers all. 
Run related jndi tests on 4 platforms for total 200 times, all passed.

Thanks,
Chris

Re: RFR (S): 8241947: Minor comment fixes for system property handling

[Magnus Ihse Bursie]

From a build perspective this looks fine.

But it seems you are changing the interface for java.lang.System. Don't 
you need a CSR for that? Or is your claim that the interface was indeed 
changed by JDK-8197927, and it is a bug that the documentation was not 
updated to match this?


/Magnus

On 2020-03-31 16:56, Langer, Christoph wrote:

Hi,

please review a small fix that updates two comments.

The first one, in make/autoconf/spec.gmk.in, is probably quite old. It talks about handling of a property 
"vm.vendor" in VersionProps.java.template. However, there is no property "vm.vendor", it 
must rather be "java.vendor". I stumbled over that when looking at the change of JDK-4947890 
(Minimize JNI upcalls in system-properties initialization).

The second one is the code comment attached to "private static Properties props;" in 
java.lang.System. After JDK-8197927 (Allow the system property `java.vendor.version` to be 
undefined), "java.vendor.version" can be undefined, so this should be reflected in the 
comment. I also took the liberty to remove an unneeded import statement.

Bug: https://bugs.openjdk.java.net/browse/JDK-8241947
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8241947.0/

Thanks
Christoph

RFR (S): 8241947: Minor comment fixes for system property handling

[Langer, Christoph]

Hi,

please review a small fix that updates two comments.

The first one, in make/autoconf/spec.gmk.in, is probably quite old. It talks 
about handling of a property "vm.vendor" in VersionProps.java.template. 
However, there is no property "vm.vendor", it must rather be "java.vendor". I 
stumbled over that when looking at the change of JDK-4947890 (Minimize JNI 
upcalls in system-properties initialization).

The second one is the code comment attached to "private static Properties 
props;" in java.lang.System. After JDK-8197927 (Allow the system property 
`java.vendor.version` to be undefined), "java.vendor.version" can be undefined, 
so this should be reflected in the comment. I also took the liberty to remove 
an unneeded import statement.

Bug: https://bugs.openjdk.java.net/browse/JDK-8241947
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8241947.0/

Thanks
Christoph

Re: RFR [15] 8241921: Remove leftover diagnostic from test/jdk/java/io/Serializable/records/SerialPersistentFieldsTest.java

[Lance Andersen]

+1

> On Mar 31, 2020, at 6:40 AM, Chris Hegarty  wrote:
> 
> Remove leftover diagnostic that dumps generated class from 
> test/jdk/java/io/Serializable/records/SerialPersistentFieldsTest.java 
> 
> At one point it was useful to be able to inspect the generated bytecode using 
> javap, but it was noticed recently for a different reason that the output 
> file is always R1.class, which is clearly wrong. Anyway, this can simply be 
> removed. 
> 
> --- a/test/jdk/java/io/Serializable/records/SerialPersistentFieldsTest.java
> +++ b/test/jdk/java/io/Serializable/records/SerialPersistentFieldsTest.java
> @@ -25,28 +25,26 @@
>  * @test
>  * @summary Basic tests for prohibited magic serialPersistentFields
>  * @library /test/lib
>  * @modules java.base/jdk.internal.org.objectweb.asm
>  * @compile --enable-preview -source ${jdk.version} 
> SerialPersistentFieldsTest.java
>  * @run testng/othervm --enable-preview SerialPersistentFieldsTest
>  */
> 
> import java.io.ByteArrayInputStream;
> import java.io.ByteArrayOutputStream;
> -import java.io.FileOutputStream;
> import java.io.IOException;
> import java.io.ObjectInputStream;
> import java.io.ObjectOutputStream;
> import java.io.ObjectStreamClass;
> import java.io.ObjectStreamField;
> import java.io.Serializable;
> -import java.io.UncheckedIOException;
> import java.lang.reflect.Field;
> import java.lang.reflect.Modifier;
> import java.math.BigDecimal;
> import jdk.internal.org.objectweb.asm.ClassReader;
> import jdk.internal.org.objectweb.asm.ClassVisitor;
> import jdk.internal.org.objectweb.asm.ClassWriter;
> import jdk.internal.org.objectweb.asm.FieldVisitor;
> import jdk.internal.org.objectweb.asm.MethodVisitor;
> import jdk.internal.org.objectweb.asm.Type;
> import jdk.test.lib.ByteCodeLoader;
> @@ -222,27 +220,20 @@
> return deserialize(serialize(obj));
> }
> 
> // -- machinery for augmenting a record class with prohibited serial 
> field --
> 
> static byte[] addSerialPersistentFields(byte[] classBytes,
> ObjectStreamField[] spf) {
> ClassReader reader = new ClassReader(classBytes);
> ClassWriter writer = new ClassWriter(reader, COMPUTE_MAXS | 
> COMPUTE_FRAMES);
> reader.accept(new SerialPersistentFieldsVisitor(writer, spf), 0);
> -try {
> -FileOutputStream fos = new FileOutputStream("R1.class");
> -fos.write(writer.toByteArray());
> -fos.close();
> -} catch (IOException ioe) {
> -throw new UncheckedIOException(ioe);
> -}
> return writer.toByteArray();
> }
> 
> /** A visitor that adds a serialPersistentFields field, and assigns it in 
> clinit. */
> static final class SerialPersistentFieldsVisitor extends ClassVisitor {
> static final String FIELD_NAME = "serialPersistentFields";
> static final String FIELD_DESC = "[Ljava/io/ObjectStreamField;";
> final ObjectStreamField[] spf;
> String className;
> SerialPersistentFieldsVisitor(ClassVisitor cv, ObjectStreamField[] 
> spf) {
> 
> 
> -Chris.
> 

 
  

 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: 8238665: Add JFR event for direct memory statistics

[Erik Gahlin]

Hi Denghui,

I think there should be one event type per pool, or there should be a text 
field specifying which pool it is. 

If we believe there will be many pools in the future, it may make sense for a 
field, otherwise I think an event type could be used. The benefit of one event 
type per pool is that there can be a description per pool and it also allows 
easy filtering and configuration.

If there would be four pools, they could all derive from an abstract event 
class and only add @Label and @Description per event subclass. If there would 
be forty pools, it would be too much noise and a field would be better.

Erik

> On 31 Mar 2020, at 11:06, Denghui Dong  wrote:
> 
> PING.
> 
> @Erik Could you review it? 
> 
> Denghui Dong
> 
> 
> 
> --
> From:董登辉(卓昂) 
> Send Time:2020年3月19日(星期四) 16:35
> To:Alan Bateman ; Erik Gahlin 
> 
> Cc:hotspot-jfr-dev ; core-libs-dev 
> 
> Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics
> 
> Hi,
> 
> On Mar 18, 2020, at 11:46 PM, Alan Bateman  > wrote:
> 
> 
> 
> On 13/03/2020 14:54, Denghui Dong wrote:
> Good suggestion, moved.
> Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.03/ 
> 
> This looks much better.
> 
> What would you think about renaming the JFR event to 
> "DirectBufferStatistics"? The concern I have with the proposed naming is that 
> it will be really awkward to extend it to support mapped buffers.
> 
> It’s ok for me.
> 
> 
> The implementation changes look okay, hopefully Erik will skim over them. One 
> small suggestion for for ManagementFactoryHelper is that you can 
> stream().collect(Collectors.toList()) to create the value for bufferPools. 
> You could even change it to volatile so that getBufferMXBeans isn't a 
> synchronized method.
> 
> 
> Make sense, updated.
> 
> Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.04/ 
> 
> 
> @Erik, could you help review it?
> 
> 
> -Alan.
> 
> Denghui Dong

RFR [15] 8241921: Remove leftover diagnostic from test/jdk/java/io/Serializable/records/SerialPersistentFieldsTest.java

[Chris Hegarty]

Remove leftover diagnostic that dumps generated class from 
test/jdk/java/io/Serializable/records/SerialPersistentFieldsTest.java 

At one point it was useful to be able to inspect the generated bytecode using 
javap, but it was noticed recently for a different reason that the output file 
is always R1.class, which is clearly wrong. Anyway, this can simply be removed. 

--- a/test/jdk/java/io/Serializable/records/SerialPersistentFieldsTest.java
+++ b/test/jdk/java/io/Serializable/records/SerialPersistentFieldsTest.java
@@ -25,28 +25,26 @@
  * @test
  * @summary Basic tests for prohibited magic serialPersistentFields
  * @library /test/lib
  * @modules java.base/jdk.internal.org.objectweb.asm
  * @compile --enable-preview -source ${jdk.version} 
SerialPersistentFieldsTest.java
  * @run testng/othervm --enable-preview SerialPersistentFieldsTest
  */
 
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
-import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.ObjectInputStream;
 import java.io.ObjectOutputStream;
 import java.io.ObjectStreamClass;
 import java.io.ObjectStreamField;
 import java.io.Serializable;
-import java.io.UncheckedIOException;
 import java.lang.reflect.Field;
 import java.lang.reflect.Modifier;
 import java.math.BigDecimal;
 import jdk.internal.org.objectweb.asm.ClassReader;
 import jdk.internal.org.objectweb.asm.ClassVisitor;
 import jdk.internal.org.objectweb.asm.ClassWriter;
 import jdk.internal.org.objectweb.asm.FieldVisitor;
 import jdk.internal.org.objectweb.asm.MethodVisitor;
 import jdk.internal.org.objectweb.asm.Type;
 import jdk.test.lib.ByteCodeLoader;
@@ -222,27 +220,20 @@
 return deserialize(serialize(obj));
 }
 
 // -- machinery for augmenting a record class with prohibited serial field 
--
 
 static byte[] addSerialPersistentFields(byte[] classBytes,
 ObjectStreamField[] spf) {
 ClassReader reader = new ClassReader(classBytes);
 ClassWriter writer = new ClassWriter(reader, COMPUTE_MAXS | 
COMPUTE_FRAMES);
 reader.accept(new SerialPersistentFieldsVisitor(writer, spf), 0);
-try {
-FileOutputStream fos = new FileOutputStream("R1.class");
-fos.write(writer.toByteArray());
-fos.close();
-} catch (IOException ioe) {
-throw new UncheckedIOException(ioe);
-}
 return writer.toByteArray();
 }
 
 /** A visitor that adds a serialPersistentFields field, and assigns it in 
clinit. */
 static final class SerialPersistentFieldsVisitor extends ClassVisitor {
 static final String FIELD_NAME = "serialPersistentFields";
 static final String FIELD_DESC = "[Ljava/io/ObjectStreamField;";
 final ObjectStreamField[] spf;
 String className;
 SerialPersistentFieldsVisitor(ClassVisitor cv, ObjectStreamField[] 
spf) {


-Chris.

Re: RFR: 8238665: Add JFR event for direct memory statistics

[Denghui Dong]

thanks for the review!

updated, still in http://cr.openjdk.java.net/~ddong/8238665/webrev.04/

could you sponsor it if everything is okay?

Thanks,
Denghui



--
From:Alan Bateman 
Send Time:2020年3月31日(星期二) 17:22
To:董登辉(卓昂) ; Erik Gahlin 
Cc:hotspot-jfr-dev ; core-libs-dev 

Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics

On 19/03/2020 08:35, Denghui Dong wrote:
>
> Make sense, updated.
>
> Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.04/
>
Thanks for the updates, looks much better.

A minor comment on ManagementFactoryHelper is that bufferPools doesn't 
have to initialized to null. That will avoid a volatile write.

-Alan.

Re: RFR: 8238665: Add JFR event for direct memory statistics

[Alan Bateman]

On 19/03/2020 08:35, Denghui Dong wrote:


Make sense, updated.

Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.04/


Thanks for the updates, looks much better.

A minor comment on ManagementFactoryHelper is that bufferPools doesn't 
have to initialized to null. That will avoid a volatile write.


-Alan.

Re: RFR: 8238665: Add JFR event for direct memory statistics

[Denghui Dong]

PING.

@Erik Could you review it? 

Denghui Dong




--
From:董登辉(卓昂) 
Send Time:2020年3月19日(星期四) 16:35
To:Alan Bateman ; Erik Gahlin 
Cc:hotspot-jfr-dev ; core-libs-dev 

Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics

Hi,

On Mar 18, 2020, at 11:46 PM, Alan Bateman  wrote:


On 13/03/2020 14:54, Denghui Dong wrote:
Good suggestion, moved.
Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.03/ 
 This looks much better.

 What would you think about renaming the JFR event to "DirectBufferStatistics"? 
The concern I have with the proposed naming is that it will be really awkward 
to extend it to support mapped buffers.

It’s ok for me.


 The implementation changes look okay, hopefully Erik will skim over them. One 
small suggestion for for ManagementFactoryHelper is that you can 
stream().collect(Collectors.toList()) to create the value for bufferPools. You 
could even change it to volatile so that getBufferMXBeans isn't a synchronized 
method.


Make sense, updated.

Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.04/

@Erik, could you help review it?


 -Alan.

Denghui Dong

RFR 8015413 Reformat line in Class.privateGetPublicFields() to be more debugger-friendly

[Vipin Mv1]

Hi All,

Please find the below changes for the issue 
https://bugs.openjdk.java.net/browse/JDK-8015413

diff -r 53568400fec3 src/java.base/share/classes/java/lang/Class.java
--- a/src/java.base/share/classes/java/lang/Class.java  Thu Mar 26 15:26:51 
2020 +
+++ b/src/java.base/share/classes/java/lang/Class.java  Mon Mar 30 17:31:04 
2020 +0530
@@ -3163,7 +3163,8 @@
 ReflectionData rd = reflectionData();
 if (rd != null) {
 res = rd.publicFields;
-if (res != null) return res;
+if (res != null)
+return res;
 }
 
 // Use a linked hash set to ensure order is preserved and



Thanks & Regards
 Vipin MV