RE: RFR 8240524: Removed warnings from test classes

[Langer, Christoph]

Hi Vipin,

this all looks correct to me.

When changing the copyright headers, you have to keep the first (initial) year 
and only update the second year. If this doesn’t exist, you’ll have to add it, 
e.g.
Copyright (c) 1999, Oracle -> Copyright (c) 1999, 2020, Oracle

I can fix that for you though, when sponsoring, no need for new webrev.

Can I have a second review, please?

Thanks
Christoph



From: Vipin Sharma 
Sent: Donnerstag, 5. März 2020 18:27
To: Langer, Christoph ; core-libs-dev@openjdk.java.net
Subject: RFR 8240524: Removed warnings from test classes

Hi All,

Please review patch to remove warnings from test classes.

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


Change description:

Class: test/jdk/java/lang/Boolean/GetBoolean.java
Fixed following warning:
1. "Exception 'java.lang.Exception' is never thrown in the method”

Class: test/jdk/java/lang/Boolean/MakeBooleanComparable.java
Fixed following warnings:
1. Explicit type argument Boolean can be replaced with <>
2. C-style array declaration of parameter 'args'

Class: test/jdk/java/lang/Boolean/ParseBoolean.java
Fixed following warning:
1. Exception 'java.lang.Exception' is never thrown in the method


Regards,
Vipin

Re: RFR(S): 8232081: Try to link all classes during dynamic CDS dump

[Ioi Lam]

Hi Calvin,

Looks good.

In JDK-8240244, I removed "ik->loader_type() != 0" because it's unclear 
what it means. I replaced with a new method 
InstanceKlass::is_shared_unregistered_class(). I think you can use this 
in your patch instead of adding loader_type() back.


No need for new webrev.

Thanks
- Ioi


On 3/5/20 11:48 AM, Calvin Cheung wrote:


On 3/5/20 9:17 AM, Ioi Lam wrote:

Hi Calvin,

Looks good overall. I just have two comment:

I think this is not needed:

 306 void ConstantPool::resolve_class_constants(TRAPS) {
 307   Arguments::assert_is_dumping_archive();

I've reverted this change.


because this function is used to resolve all Strings in the 
statically dumped classes to archive all the Strings. However, the 
archive heap is not supported for the dynamic archive. So I think 
it's better to avoid calling this function from 
LinkSharedClassesClosure for dynamic dump.

Now, the function will not be called with dynamic dump:
1714 if (DumpSharedSpaces) {
1715   // The following function is used to resolve all 
Strings in the statically
1716   // dumped classes to archive all the Strings. The 
archive heap is not supported

1717   // for the dynamic archive.
1718 ik->constants()->resolve_class_constants(THREAD);
1719 }


LinkSharedClassesClosure::_is_static -- maybe we can avoid adding 
this field and just check "if (DumpSharedSpaces)" instead?


This is a good simplification.

btw, you've removed loader_type() from instanceKlass.hpp when you 
pushed the change for JDK-8240244. I've added it back as 
shared_loader_type().


updated webrev:

    http://cr.openjdk.java.net/~ccheung/jdk15/8232081/webrev.04/

thanks,

Calvin



Thanks
- Ioi





On 3/4/20 9:13 PM, Calvin Cheung wrote:

Hi David, Ioi,

Here's an updated webrev which doesn't involve changing 
InstanceKlass::verify_on() and made use of the changes for JDK-8240481.


http://cr.openjdk.java.net/~ccheung/jdk15/8232081/webrev.03/

thanks,
Calvin

On 3/1/20 10:14 PM, David Holmes wrote:

Hi Calvin,

On 28/02/2020 4:12 pm, Calvin Cheung wrote:

Hi David,

On 2/27/20 5:40 PM, David Holmes wrote:

Hi Calvin, Ioi,

Looking good - comments below.

A meta-question: normal application classes are rarely loaded but 
not linked so I'm a little surprised this is an issue. What is 
the main source of such classes - generated classes like lambda 
forms? Also why do we need to link them when loading from the 
archive if they were never linked when the application actually 
executed ??


I saw that Ioi already answered the above.

I'll try to answer your questions inline below..


Responses inline below ...



On 28/02/2020 10:48 am, Calvin Cheung wrote:

Hi David, Ioi,

Thanks for your review and suggestions.

On 2/26/20 9:46 PM, Ioi Lam wrote:



On 2/26/20 7:50 PM, David Holmes wrote:

Hi Calvin,

Adding core-libs-dev as you are messing with their code :)

On 27/02/2020 1:19 pm, Calvin Cheung wrote:

JBS: https://bugs.openjdk.java.net/browse/JDK-8232081
webrev: 
http://cr.openjdk.java.net/~ccheung/jdk15/8232081/webrev.00/


The proposed changeset for this RFE adds a 
JVM_LinkClassesForCDS() function to be called from 
java/lang/Shutdown to notify the JVM to link the classes 
loaded by the builtin class loaders. The 


This would be much less disruptive if this was handled purely 
on the VM side once we have started shutdown. No need to make 
any changes to the Java side then, nor jvm.cpp.




Hi David,

To link the classes, we need to be able to run Java code -- 
when linking a class X loaded by the app loader, X will be 
verified. During verification of X, we may need to load 
additional classes from the app loader, which executes Java 
code during its class loading operations.


We also need to handle all the exit conditions. As far as I can 
tell, an app can exit the JVM in two ways:


(1) Explicitly calling System.exit(). This will call 
java.lang.Shutdown.exit() which does this:


http://hg.openjdk.java.net/jdk/jdk/file/0edc7fd0d7a3/src/java.base/share/classes/java/lang/Shutdown.java#l163 



    beforeHalt(); // native
    runHooks();
    halt(status);

(2) When all non-daemon threads have died (e.g., falling out of 
the bottom of HelloWorld.main()). There's no explicit call to 
System.exit(), but the JVM will proactively call 
java.lang.Shutdown.shutdown() inside 
JavaThread::invoke_shutdown_hooks()


http://hg.openjdk.java.net/jdk/jdk/file/0edc7fd0d7a3/src/hotspot/share/runtime/thread.cpp#l4331 

http://hg.openjdk.java.net/jdk/jdk/file/0edc7fd0d7a3/src/java.base/share/classes/java/lang/Shutdown.java#l184 



If we want to avoid modifying the Java code, I think we can 
intercept JVM_BeforeHalt() and 
JavaThread::invoke_shutdown_hooks(). This way we should be able 
to handle all the classes (except those that are loaded inside 
Shutdown.runHooks).


I've implemented the above. No changes to the Java side.

updated webrev:

http://cr.openjdk.java.net/~ccheung/jdk15/8232081/webrev.

Re: argfiles parsing broken for argfiles > 4096 bytes

[Henry Jen]

Thanks for the report and investigation, bug confirmed, JDK-8240629 is filed to 
tacking the issue.

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

Cheers,
Henry


> On Mar 5, 2020, at 11:43 AM, Robert Stupp  wrote:
> 
> Another note: I've also seen it appending a comment-line in
> http://hg.openjdk.java.net/jdk-updates/jdk11u/annotate/20e49753c388/src/java.base/share/native/libjli/args.c#l296
> with pctx->state==IN_COMMENT, but wasn't able to reconstruct the
> behavior with a "clean" argfile. But that behavior was also fixed with
> the condition ('&& pctx->state == IN_TOKEN') added by the patch.
> 
> On Thu, 2020-03-05 at 20:34 +0100, Robert Stupp wrote:
>> Sorry for the broken formatting...
>> 
>> I recently came across a bug in the java executable with an argfile
>> that's larger than 4096 bytes, if a 4096-byte-chunk ends in a comment
>> line.
>> 
>> The bug happens when the last character of a comment line is the
>> 4096th
>> byte and the trailing newline is the first byte in the next chunk. In
>> that case, nextToken() in src/java.base/share/native/libjli/args.c
>> calls JLI_List_addSubstring (in the if-block at the end of
>> nextToken())
>> with the contents of the current comment line. The next "valid"
>> option
>> gets appended to that comment line and java errors out.
>> 
>> I've added some debugging output to the last if-block and pctx->
>> state==FIND_NEXT and the the substring being added is the comment
>> line.
>> 
>> I've also tried and changed this line
>> http://hg.openjdk.java.net/jdk-updates/jdk11u/annotate/20e49753c388/src/java.base/share/native/libjli/args.c#l294
>> to 'if (anchor != nextc && pctx->state == IN_TOKEN)' and the argfile
>> is
>> parsed correctly.
>> 
>> Steps to reproduce:
>> 1. save the attached my-argfile
>> 2. run 'java @my-argfile my.className'
>> 3. java errors out with
>> Error: Could not find or
>> load main class #
>> X
>> XX
>> X
>> -Dfoo=bar
>> Caused by: java.lang.ClassNotFoundException: #
>> X
>> XX
>> XX
>> -Dfoo=bar
>> 
>> With the above change (the attached patch in argfile-parse-bug.txt),
>> argfile parsing works as expected.Also updated the condition in the
>> loop looking for a comment's newline, as it looks inconsistent to the
>> condition in the loop for FIND_NEXT/SKIP_LEAD_WS and the condition of
>> the outer for-loop.
>> 
>> Robert
>> 
>> 
>> On Thu, 2020-03-05 at 20:00 +0100, Robert Stupp wrote:
>>> Hi,
>> ...
> 

Re: JDK 15 RFR of JDK-8240624: Note mapping of RoundingMode constants to equivalent IEEE 754-2019 policy

[Brian Burkhalter]

Hi Joe,

> On Mar 5, 2020, at 2:43 PM, Joe Darcy  wrote:
> 
>> Looks fine. Do you think the IEEE 754-2019 attribute names should have some 
>> kind of formatting, e.g., @code or italics?
> The IEEE standard uses normal next for those terms, no italics, bold, etc. ; 
> in this context, I think preserving that convention is fine.
> 
That make sense.
> I'll push with a slightly reworked opening API note:
> 
> + * @apiNote
> + * Five of the rounding modes declared in this class correspond to
> + * rounding direction attributes defined in the IEEE Standard
> + * for Floating-Point Arithmetic, IEEE 754-2019. Where present,
> + * this correspondence will be noted in the documentation of the
> + * particular constant.
> + *
> 
Looks good.

Brian

Re: JDK 15 RFR of JDK-8240624: Note mapping of RoundingMode constants to equivalent IEEE 754-2019 policy

[Joe Darcy]

Hi Brian,

On 3/5/2020 1:51 PM, Brian Burkhalter wrote:

Hi Joe,

On Mar 5, 2020, at 1:45 PM, Joe Darcy > wrote:


Where such a mapping exists, it would be helpful to tie the policies 
represented by various RoundingMode constants to the equivalent 
rounding attribute defined by IEEE 754-2019, the latest version of 
the floating-point standard:


    JDK-8240624: Note mapping of RoundingMode constants to equivalent 
IEEE 754-2019 policy

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

Looks fine. Do you think the IEEE 754-2019 attribute names should have 
some kind of formatting, e.g., @code or italics?


The IEEE standard uses normal next for those terms, no italics, bold, 
etc. ; in this context, I think preserving that convention is fine.


I'll push with a slightly reworked opening API note:

+ * @apiNote
+ * Five of the rounding modes declared in this class correspond to
+ * rounding direction attributes defined in the IEEE Standard
+ * for Floating-Point Arithmetic, IEEE 754-2019. Where present,
+ * this correspondence will be noted in the documentation of the
+ * particular constant.
+ *

Thanks,

-Joe

Re: [15] 8235792: LineNumberReader.getLineNumber() behavior is inconsistent with respect to EOF

[Brian Burkhalter]

Might anyone else have any comments on this thread the original post in which 
was [1].

Thanks,

Brian

[1] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-January/064417.html

> On Jan 31, 2020, at 12:21 PM, Brian Burkhalter  
> wrote:
> 
>> On Jan 31, 2020, at 12:20 PM, Alan Bateman > > wrote:
>> 
>> On 31/01/2020 20:10, Brian Burkhalter wrote:
>>> :
>>> OK. I’ll change /TERM/EOL/ but not post another webrev unless there are 
>>> further comments. I will wait to check it in until Monday in case someone 
>>> has comments.
>>> 
>> I think it would be good to more eyes on this as LNR has a history of 
>> resisting change.
> 
> I’ll hold off on it then for a while.
> 
>> I think you'll need a CSR too this re-specifies when the conditions when the 
>> line number is incremented.
> 
> Yes, I agree.

Re: JDK 15 RFR of JDK-8240624: Note mapping of RoundingMode constants to equivalent IEEE 754-2019 policy

[Brian Burkhalter]

Hi Joe,

> On Mar 5, 2020, at 1:45 PM, Joe Darcy  wrote:
> 
> Where such a mapping exists, it would be helpful to tie the policies 
> represented by various RoundingMode constants to the equivalent rounding 
> attribute defined by IEEE 754-2019, the latest version of the floating-point 
> standard:
> 
> JDK-8240624: Note mapping of RoundingMode constants to equivalent IEEE 
> 754-2019 policy
> http://cr.openjdk.java.net/~darcy/8240624.0/ 
> 
Looks fine. Do you think the IEEE 754-2019 attribute names should have some 
kind of formatting, e.g., @code or italics?
> Please review the patch below; I'll update the file copyright year before 
> pushing.
> 
+1

Brian

JDK 15 RFR of JDK-8240624: Note mapping of RoundingMode constants to equivalent IEEE 754-2019 policy

[Joe Darcy]

Hello,

Where such a mapping exists, it would be helpful to tie the policies 
represented by various RoundingMode constants to the equivalent rounding 
attribute defined by IEEE 754-2019, the latest version of the 
floating-point standard:


    JDK-8240624: Note mapping of RoundingMode constants to equivalent 
IEEE 754-2019 policy

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

Please review the patch below; I'll update the file copyright year 
before pushing.


Thanks,

-Joe

--- old/src/java.base/share/classes/java/math/RoundingMode.java 2020-03-05 
13:44:23.971037000 -0800
+++ new/src/java.base/share/classes/java/math/RoundingMode.java 2020-03-05 
13:44:23.647037000 -0800
@@ -87,6 +87,12 @@
  * ({@link BigDecimal#ROUND_UP}, {@link BigDecimal#ROUND_DOWN},
  * etc. ).
  *
+ * @apiNote
+ * Five of the rounding modes defined in this class correspond to
+ * rounding direction attributes defined in IEEE 754-2019. Where
+ * present, this correspondence will be noted in the documentation of
+ * the particular constant.
+ *
  * @see BigDecimal
  * @see MathContext
  * @author  Josh Bloch
@@ -130,7 +136,9 @@
  * Rounding mode to round towards zero.  Never increments the digit
  * prior to a discarded fraction (i.e., truncates).  Note that this
  * rounding mode never increases the magnitude of the calculated value.
- *
+ * This mode corresponds to the IEEE 754-2019 rounding
+ * attribute roundTowardZero.
+ *
  *Example:
  *
  * Rounding mode DOWN Examples
@@ -159,6 +167,8 @@
  * result is positive, behaves as for {@code RoundingMode.UP};
  * if negative, behaves as for {@code RoundingMode.DOWN}.  Note
  * that this rounding mode never decreases the calculated value.
+ * This mode corresponds to the IEEE 754-2019 rounding
+ * attribute roundTowardPositive.
  *
  *Example:
  *
@@ -188,6 +198,8 @@
  * result is positive, behave as for {@code RoundingMode.DOWN};
  * if negative, behave as for {@code RoundingMode.UP}.  Note that
  * this rounding mode never increases the calculated value.
+ * This mode corresponds to the IEEE 754-2019 rounding
+ * attribute roundTowardNegative.
  *
  *Example:
  *
@@ -219,6 +231,8 @@
  * fraction is ≥ 0.5; otherwise, behaves as for
  * {@code RoundingMode.DOWN}.  Note that this is the rounding
  * mode commonly taught at school.
+ * This mode corresponds to the IEEE 754-2019 rounding
+ * attribute roundTiesToAway.
  *
  *Example:
  *
@@ -286,6 +300,8 @@
  * chiefly used in the USA.  This rounding mode is analogous to
  * the rounding policy used for {@code float} and {@code double}
  * arithmetic in Java.
+ * This mode corresponds to the IEEE 754-2019 rounding
+ * attribute roundTiesToEven.
  *
  *Example:
  *

Re: RFR 8239893: Windows handle Leak when starting processes using ProcessBuilder

[naoto . sato]

Hi Roger,

In the test, the error value (-1) from the native code seems silently 
suppressed. Should it be caught/reported in the java side?


nit: copyright year in ProcessImpl.java -> 2020.

Naoto

On 3/5/20 12:51 PM, Roger Riggs wrote:
Please review a change to the Windows ProcessImpl to ensure that the 
handles
created for the input and output streams of a process are closed when no 
longer referenced.


Unlike on Linux, there is no thread monitoring the process that can 
close the streams.
The FileDescriptors are registered with the Cleaner to be closed when 
they are no longer referenced.


A test is added that monitors the count of handles as 50 Processes are 
launched and exit.

The test and change only affect the Windows implementation.

Webrev:
   http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893/

Issue:
   https://bugs.openjdk.java.net/browse/JDK-8239893

Thanks, Roger

Re: RFR 8239893: Windows handle Leak when starting processes using ProcessBuilder

[Brian Burkhalter]

Hi Roger,

> On Mar 5, 2020, at 12:51 PM, Roger Riggs  wrote:
> 
> Please review a change to the Windows ProcessImpl to ensure that the handles
> created for the input and output streams of a process are closed when no 
> longer referenced.
> 
> Unlike on Linux, there is no thread monitoring the process that can close the 
> streams.
> The FileDescriptors are registered with the Cleaner to be closed when they 
> are no longer referenced.
> 
> A test is added that monitors the count of handles as 50 Processes are 
> launched and exit.
> The test and change only affect the Windows implementation.
> 
> Webrev:
>   http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893/ 
> 
> 
> Issue:
>   https://bugs.openjdk.java.net/browse/JDK-8239893 
> 
In CheckHandles.java at line 72 there is this calculation:
final long ERROR_THRESHOLD = minHandles + (minHandles / ERROR_PERCENT); // 10% 
increase over min to passing max
Do you think it would be better as

final long ERROR_THRESHOLD = minHandles + ((minHandles + ERROR_PERCENT - 1) / 
ERROR_PERCENT); // 10% increase over min to passing max

, i.e., rounded instead of truncated?

Brian

RFR 8239893: Windows handle Leak when starting processes using ProcessBuilder

[Roger Riggs]

Please review a change to the Windows ProcessImpl to ensure that the handles
created for the input and output streams of a process are closed when no 
longer referenced.


Unlike on Linux, there is no thread monitoring the process that can 
close the streams.
The FileDescriptors are registered with the Cleaner to be closed when 
they are no longer referenced.


A test is added that monitors the count of handles as 50 Processes are 
launched and exit.

The test and change only affect the Windows implementation.

Webrev:
  http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893/

Issue:
  https://bugs.openjdk.java.net/browse/JDK-8239893

Thanks, Roger

Re: RFR(S): 8232081: Try to link all classes during dynamic CDS dump

[Calvin Cheung]

On 3/5/20 9:17 AM, Ioi Lam wrote:

Hi Calvin,

Looks good overall. I just have two comment:

I think this is not needed:

 306 void ConstantPool::resolve_class_constants(TRAPS) {
 307   Arguments::assert_is_dumping_archive();

I've reverted this change.


because this function is used to resolve all Strings in the statically 
dumped classes to archive all the Strings. However, the archive heap 
is not supported for the dynamic archive. So I think it's better to 
avoid calling this function from LinkSharedClassesClosure for dynamic 
dump.

Now, the function will not be called with dynamic dump:
1714 if (DumpSharedSpaces) {
1715   // The following function is used to resolve all Strings 
in the statically
1716   // dumped classes to archive all the Strings. The archive 
heap is not supported

1717   // for the dynamic archive.
1718 ik->constants()->resolve_class_constants(THREAD);
1719 }


LinkSharedClassesClosure::_is_static -- maybe we can avoid adding this 
field and just check "if (DumpSharedSpaces)" instead?


This is a good simplification.

btw, you've removed loader_type() from instanceKlass.hpp when you pushed 
the change for JDK-8240244. I've added it back as shared_loader_type().


updated webrev:

    http://cr.openjdk.java.net/~ccheung/jdk15/8232081/webrev.04/

thanks,

Calvin



Thanks
- Ioi





On 3/4/20 9:13 PM, Calvin Cheung wrote:

Hi David, Ioi,

Here's an updated webrev which doesn't involve changing 
InstanceKlass::verify_on() and made use of the changes for JDK-8240481.


    http://cr.openjdk.java.net/~ccheung/jdk15/8232081/webrev.03/

thanks,
Calvin

On 3/1/20 10:14 PM, David Holmes wrote:

Hi Calvin,

On 28/02/2020 4:12 pm, Calvin Cheung wrote:

Hi David,

On 2/27/20 5:40 PM, David Holmes wrote:

Hi Calvin, Ioi,

Looking good - comments below.

A meta-question: normal application classes are rarely loaded but 
not linked so I'm a little surprised this is an issue. What is the 
main source of such classes - generated classes like lambda forms? 
Also why do we need to link them when loading from the archive if 
they were never linked when the application actually executed ??


I saw that Ioi already answered the above.

I'll try to answer your questions inline below..


Responses inline below ...



On 28/02/2020 10:48 am, Calvin Cheung wrote:

Hi David, Ioi,

Thanks for your review and suggestions.

On 2/26/20 9:46 PM, Ioi Lam wrote:



On 2/26/20 7:50 PM, David Holmes wrote:

Hi Calvin,

Adding core-libs-dev as you are messing with their code :)

On 27/02/2020 1:19 pm, Calvin Cheung wrote:

JBS: https://bugs.openjdk.java.net/browse/JDK-8232081
webrev: 
http://cr.openjdk.java.net/~ccheung/jdk15/8232081/webrev.00/


The proposed changeset for this RFE adds a 
JVM_LinkClassesForCDS() function to be called from 
java/lang/Shutdown to notify the JVM to link the classes 
loaded by the builtin class loaders. The 


This would be much less disruptive if this was handled purely 
on the VM side once we have started shutdown. No need to make 
any changes to the Java side then, nor jvm.cpp.




Hi David,

To link the classes, we need to be able to run Java code -- when 
linking a class X loaded by the app loader, X will be verified. 
During verification of X, we may need to load additional classes 
from the app loader, which executes Java code during its class 
loading operations.


We also need to handle all the exit conditions. As far as I can 
tell, an app can exit the JVM in two ways:


(1) Explicitly calling System.exit(). This will call 
java.lang.Shutdown.exit() which does this:


http://hg.openjdk.java.net/jdk/jdk/file/0edc7fd0d7a3/src/java.base/share/classes/java/lang/Shutdown.java#l163 



    beforeHalt(); // native
    runHooks();
    halt(status);

(2) When all non-daemon threads have died (e.g., falling out of 
the bottom of HelloWorld.main()). There's no explicit call to 
System.exit(), but the JVM will proactively call 
java.lang.Shutdown.shutdown() inside 
JavaThread::invoke_shutdown_hooks()


http://hg.openjdk.java.net/jdk/jdk/file/0edc7fd0d7a3/src/hotspot/share/runtime/thread.cpp#l4331 

http://hg.openjdk.java.net/jdk/jdk/file/0edc7fd0d7a3/src/java.base/share/classes/java/lang/Shutdown.java#l184 



If we want to avoid modifying the Java code, I think we can 
intercept JVM_BeforeHalt() and 
JavaThread::invoke_shutdown_hooks(). This way we should be able 
to handle all the classes (except those that are loaded inside 
Shutdown.runHooks).


I've implemented the above. No changes to the Java side.

updated webrev:

http://cr.openjdk.java.net/~ccheung/jdk15/8232081/webrev.01/


This looks much better to me. I wasn't sure if you were that 
concerned about catching the classes loaded by the Shutdown hooks, 
but it is good you are not as it seems problematic to me to 
trigger class loading etc after the shutdown hooks have executed - 
you may hit unexpected conditions. So this approach is good.


A few minor comments:

Re: argfiles parsing broken for argfiles > 4096 bytes

[Robert Stupp]

Another note: I've also seen it appending a comment-line in
http://hg.openjdk.java.net/jdk-updates/jdk11u/annotate/20e49753c388/src/java.base/share/native/libjli/args.c#l296
with pctx->state==IN_COMMENT, but wasn't able to reconstruct the
behavior with a "clean" argfile. But that behavior was also fixed with
the condition ('&& pctx->state == IN_TOKEN') added by the patch.

On Thu, 2020-03-05 at 20:34 +0100, Robert Stupp wrote:
> Sorry for the broken formatting...
>
> I recently came across a bug in the java executable with an argfile
> that's larger than 4096 bytes, if a 4096-byte-chunk ends in a comment
> line.
>
> The bug happens when the last character of a comment line is the
> 4096th
> byte and the trailing newline is the first byte in the next chunk. In
> that case, nextToken() in src/java.base/share/native/libjli/args.c
> calls JLI_List_addSubstring (in the if-block at the end of
> nextToken())
> with the contents of the current comment line. The next "valid"
> option
> gets appended to that comment line and java errors out.
>
> I've added some debugging output to the last if-block and pctx->
> state==FIND_NEXT and the the substring being added is the comment
> line.
>
> I've also tried and changed this line
> http://hg.openjdk.java.net/jdk-updates/jdk11u/annotate/20e49753c388/src/java.base/share/native/libjli/args.c#l294
> to 'if (anchor != nextc && pctx->state == IN_TOKEN)' and the argfile
> is
> parsed correctly.
>
> Steps to reproduce:
> 1. save the attached my-argfile
> 2. run 'java @my-argfile my.className'
> 3. java errors out with
> Error: Could not find or
> load main class #
> X
> XX
> X
> -Dfoo=bar
> Caused by: java.lang.ClassNotFoundException: #
> X
> XX
> XX
> -Dfoo=bar
>
> With the above change (the attached patch in argfile-parse-bug.txt),
> argfile parsing works as expected.Also updated the condition in the
> loop looking for a comment's newline, as it looks inconsistent to the
> condition in the loop for FIND_NEXT/SKIP_LEAD_WS and the condition of
> the outer for-loop.
>
> Robert
>
>
> On Thu, 2020-03-05 at 20:00 +0100, Robert Stupp wrote:
> > Hi,
> ...

Re: argfiles parsing broken for argfiles > 4096 bytes

[Robert Stupp]

Sorry for the broken formatting...

I recently came across a bug in the java executable with an argfile
that's larger than 4096 bytes, if a 4096-byte-chunk ends in a comment
line.

The bug happens when the last character of a comment line is the 4096th
byte and the trailing newline is the first byte in the next chunk. In
that case, nextToken() in src/java.base/share/native/libjli/args.c
calls JLI_List_addSubstring (in the if-block at the end of nextToken())
with the contents of the current comment line. The next "valid" option
gets appended to that comment line and java errors out.

I've added some debugging output to the last if-block and pctx->
state==FIND_NEXT and the the substring being added is the comment line.

I've also tried and changed this line
http://hg.openjdk.java.net/jdk-updates/jdk11u/annotate/20e49753c388/src/java.base/share/native/libjli/args.c#l294
to 'if (anchor != nextc && pctx->state == IN_TOKEN)' and the argfile is
parsed correctly.

Steps to reproduce:
1. save the attached my-argfile
2. run 'java @my-argfile my.className'
3. java errors out with
Error: Could not find or
load main class #
XXX
X
-Dfoo=bar
Caused by: java.lang.ClassNotFoundException: #
XXX
XX
-Dfoo=bar

With the above change (the attached patch in argfile-parse-bug.txt),
argfile parsing works as expected.Also updated the condition in the
loop looking for a comment's newline, as it looks inconsistent to the
condition in the loop for FIND_NEXT/SKIP_LEAD_WS and the condition of
the outer for-loop.

Robert


On Thu, 2020-03-05 at 20:00 +0100, Robert Stupp wrote:
> Hi,
...
>

RE: jpackage Linux rpath [EXTERNAL]

[Auclair, Andrew]

Yes. I talked to a coworker and realized I glossed over that part. We run 
jpackage to create an application image and then use InstallBuilder to create 
an installer. We have 6 Java applications and dozens of C++ applications that 
all go into one big installer.

Andrew

-Original Message-
From: Alexey Semenyuk  
Sent: Thursday, March 5, 2020 2:03 PM
To: Auclair, Andrew ; core-libs-dev@openjdk.java.net
Subject: Re: jpackage Linux rpath [EXTERNAL]

Andrew,

Thank you for sharing!
As far as I can see you use jpackage to create application image. I think your 
solution is optimal for this scenario.

- Alexey

On 3/5/2020 1:57 PM, Auclair, Andrew wrote:
> Hi Alexey,
>
> Thanks for the response! I think we will continue to do what we're doing now 
> because it works just fine and is pretty simple.
>
> If anyone d is interested, this is how we do it: We run everything through a 
> doLast in our build.gradle file and run an exec {} for jpackage and then if 
> we're on linux we run another exec {} for patchelf.
>
> exec {
>  def icon
>  executable "${System.env.JAVA_HOME}/bin/jpackage"
>  if (OperatingSystem.current().isLinux()) {
>  icon = "Example.png"
>  }
>  else {
>  icon = " Example.ico"
>  }
>  args = ["--type", "app-image", "--icon", icon, "--input", 
> "build/libs", "--dest", "build/install/tmp", "--name", " Example ", 
> "--main-class", "com.example", "--main-jar", " Example.jar"] } if 
> (OperatingSystem.current().isLinux()) {
>  exec {
>  executable "patchelf"
>  args = ["--force-rpath", "--set-rpath", "\$ORIGIN", 
> "../build/install/Example "]
>  }
> }
>
> Thanks Again,
> Andrew
>
> -Original Message-
> From: Alexey Semenyuk 
> Sent: Thursday, March 5, 2020 1:51 PM
> To: Auclair, Andrew ; 
> core-libs-dev@openjdk.java.net
> Subject: Re: jpackage Linux rpath [EXTERNAL]
>
> Hi Andrew,
>
> jpackage is providing similar functionality as javapackager, so I think you 
> need to keep using patchelf with jpackage as you used it with javapackager.
> I assume you use jpackage in two phases. The first phase creates application 
> image and you use patchelf on launchers created by jpackage and then in the 
> second phase you pack application image with modified launchers into rpm/deb 
> package.
> If this is the case and you would benefit from running jpackage in one phase, 
> then I can think of adding to jpackage ability to run 3rd party script after 
> application image creation. In this case you will need to create shell script 
> using patchelf, place the script in resource directory so that jpackage can 
> run the script after application image is created but before building rpm/deb 
> package.
>
> - Alexey
>
> On 3/5/2020 9:42 AM, Auclair, Andrew wrote:
>> Hi,
>>
>> We are in the process of upgrading to jpackage in Java 14 from javapackager 
>> in Java 10. So far the transition has been smooth and we're really liking 
>> jpackage.
>>
>> One question has come up regarding our Linux install. Currently we are using 
>> Patchelf on CentOS to set an rpath for the executables generated by 
>> javapackager. We have continued this with jpackage, but we were wondering if 
>> there's a way to set the rpath with jpackage to avoid using Patchelf.
>>
>> Thanks,
>>
>> Andrew Auclair
>> Software Engineer
>> Tactical Communications Group
>> A Curtiss-Wright Company
>> 2 Highwood Drive, Building 2, Suite 200 Tewksbury, MA  01876-1157
>>
>> -
>> - This e-mail and any files transmitted with it are proprietary and 
>> intended solely for the use of the individual or entity to whom they are 
>> addressed. If you have reason to believe that you have received this e-mail 
>> in error, please notify the sender and destroy this e-mail and any attached 
>> files. Please note that any views or opinions presented in this e-mail are 
>> solely those of the author and do not necessarily represent those of the 
>> Curtiss-Wright Corporation or any of its subsidiaries. Documents attached 
>> hereto may contain technology subject to the U.S. Export Regulations. 
>> Recipient is solely responsible for ensuring that any re-export, transfer or 
>> disclosure of this information is in accordance with U.S. Export 
>> Regulations. The recipient should check this e-mail and any attachments for 
>> the presence of viruses. Curtiss-Wright Corporation and its subsidiaries 
>> accept no liability for any damage caused by any virus transmitted by this 
>> e-mail.
>
> --
> This e-mail and any files transmitted with it are proprietary and intended 
> solely for the use of the individual or entity to whom they are addressed. If 
> you have reason to believe that you have received this e-mail in error, 
> please notify the sender and destroy this e-mail and any attached files. 
> Please note that any views or opinions presented in this 

Re: jpackage Linux rpath [EXTERNAL]

[Alexey Semenyuk]

Andrew,

Thank you for sharing!
As far as I can see you use jpackage to create application image. I 
think your solution is optimal for this scenario.


- Alexey

On 3/5/2020 1:57 PM, Auclair, Andrew wrote:

Hi Alexey,

Thanks for the response! I think we will continue to do what we're doing now 
because it works just fine and is pretty simple.

If anyone d is interested, this is how we do it: We run everything through a 
doLast in our build.gradle file and run an exec {} for jpackage and then if 
we're on linux we run another exec {} for patchelf.

exec {
 def icon
 executable "${System.env.JAVA_HOME}/bin/jpackage"
 if (OperatingSystem.current().isLinux()) {
 icon = "Example.png"
 }
 else {
 icon = " Example.ico"
 }
 args = ["--type", "app-image", "--icon", icon, "--input", "build/libs", "--dest", "build/install/tmp", "--name", " 
Example ", "--main-class", "com.example", "--main-jar", " Example.jar"]
}
if (OperatingSystem.current().isLinux()) {
 exec {
 executable "patchelf"
 args = ["--force-rpath", "--set-rpath", "\$ORIGIN", 
"../build/install/Example "]
 }
}

Thanks Again,
Andrew

-Original Message-
From: Alexey Semenyuk 
Sent: Thursday, March 5, 2020 1:51 PM
To: Auclair, Andrew ; core-libs-dev@openjdk.java.net
Subject: Re: jpackage Linux rpath [EXTERNAL]

Hi Andrew,

jpackage is providing similar functionality as javapackager, so I think you 
need to keep using patchelf with jpackage as you used it with javapackager.
I assume you use jpackage in two phases. The first phase creates application 
image and you use patchelf on launchers created by jpackage and then in the 
second phase you pack application image with modified launchers into rpm/deb 
package.
If this is the case and you would benefit from running jpackage in one phase, 
then I can think of adding to jpackage ability to run 3rd party script after 
application image creation. In this case you will need to create shell script 
using patchelf, place the script in resource directory so that jpackage can run 
the script after application image is created but before building rpm/deb 
package.

- Alexey

On 3/5/2020 9:42 AM, Auclair, Andrew wrote:

Hi,

We are in the process of upgrading to jpackage in Java 14 from javapackager in 
Java 10. So far the transition has been smooth and we're really liking jpackage.

One question has come up regarding our Linux install. Currently we are using 
Patchelf on CentOS to set an rpath for the executables generated by 
javapackager. We have continued this with jpackage, but we were wondering if 
there's a way to set the rpath with jpackage to avoid using Patchelf.

Thanks,

Andrew Auclair
Software Engineer
Tactical Communications Group
A Curtiss-Wright Company
2 Highwood Drive, Building 2, Suite 200 Tewksbury, MA  01876-1157

--
This e-mail and any files transmitted with it are proprietary and intended 
solely for the use of the individual or entity to whom they are addressed. If 
you have reason to believe that you have received this e-mail in error, please 
notify the sender and destroy this e-mail and any attached files. Please note 
that any views or opinions presented in this e-mail are solely those of the 
author and do not necessarily represent those of the Curtiss-Wright Corporation 
or any of its subsidiaries. Documents attached hereto may contain technology 
subject to the U.S. Export Regulations. Recipient is solely responsible for 
ensuring that any re-export, transfer or disclosure of this information is in 
accordance with U.S. Export Regulations. The recipient should check this e-mail 
and any attachments for the presence of viruses. Curtiss-Wright Corporation and 
its subsidiaries accept no liability for any damage caused by any virus 
transmitted by this e-mail.


--
This e-mail and any files transmitted with it are proprietary and intended 
solely for the use of the individual or entity to whom they are addressed. If 
you have reason to believe that you have received this e-mail in error, please 
notify the sender and destroy this e-mail and any attached files. Please note 
that any views or opinions presented in this e-mail are solely those of the 
author and do not necessarily represent those of the Curtiss-Wright Corporation 
or any of its subsidiaries. Documents attached hereto may contain technology 
subject to the U.S. Export Regulations. Recipient is solely responsible for 
ensuring that any re-export, transfer or disclosure of this information is in 
accordance with U.S. Export Regulations. The recipient should check this e-mail 
and any attachments for the presence of viruses. Curtiss-Wright Corporation and 
its subsidiaries accept no liability for any damage caused by any virus 
transmitted by this e-mail.

argfiles parsing broken for argfiles > 4096 bytes

[Robert Stupp]

Hi,
I recently came across a bug in the java executable with an argfile
that's larger than 4096 bytes, if a 4096-byte-chunk ends in a comment
line.
One bug happens when the last character of a comment line is the 4096th
byte and the trailing newline is the first byte in the next chunk. In
that case, nextToken() in src/java.base/share/native/libjli/args.c
calls JLI_List_addSubstring (in the if-block at the end of nextToken())
with the contents of the current comment line. The next "valid" option
gets appended to that comment line and java errors out.
I've added some debugging output to the last if-block and pctx-
>state==FIND_NEXT and the the substring being added is the comment
line.
I've also tried and changed this line
http://hg.openjdk.java.net/jdk-updates/jdk11u/annotate/20e49753c388/src/java.base/share/native/libjli/args.c#l294
to 'if (anchor != nextc && pctx->state == IN_TOKEN)' and the argfile is
parsed correctly.
Steps to reproduce:1. save the attached my-argfile2. run 'java @my-
argfile my.className'3. java errors out with Error: Could not find or
load main class #
XXX
XX-Dfoo=barCaused
by: java.lang.ClassNotFoundException: #
XXX
XX-Dfoo=bar
With the above change (the attached patch in argfile-parse-bug.txt),
argfile parsing works as expected.Also updated the condition in the
loop looking for a comment's newline, as it looks inconsistent to the
condition in the loop for FIND_NEXT/SKIP_LEAD_WS and the condition of
the outer for-loop.
Robert


# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
# 
23456789012345678901234567890123456789012345678901234567

RE: jpackage Linux rpath [EXTERNAL]

[Auclair, Andrew]

Hi Alexey,

Thanks for the response! I think we will continue to do what we're doing now 
because it works just fine and is pretty simple.

If anyone d is interested, this is how we do it: We run everything through a 
doLast in our build.gradle file and run an exec {} for jpackage and then if 
we're on linux we run another exec {} for patchelf.

exec {
def icon
executable "${System.env.JAVA_HOME}/bin/jpackage"
if (OperatingSystem.current().isLinux()) {
icon = "Example.png"
}
else {
icon = " Example.ico"
}
args = ["--type", "app-image", "--icon", icon, "--input", "build/libs", 
"--dest", "build/install/tmp", "--name", " Example ", "--main-class", 
"com.example", "--main-jar", " Example.jar"]
}
if (OperatingSystem.current().isLinux()) {
exec {
executable "patchelf"
args = ["--force-rpath", "--set-rpath", "\$ORIGIN", 
"../build/install/Example "]
}
}

Thanks Again,
Andrew

-Original Message-
From: Alexey Semenyuk  
Sent: Thursday, March 5, 2020 1:51 PM
To: Auclair, Andrew ; core-libs-dev@openjdk.java.net
Subject: Re: jpackage Linux rpath [EXTERNAL]

Hi Andrew,

jpackage is providing similar functionality as javapackager, so I think you 
need to keep using patchelf with jpackage as you used it with javapackager.
I assume you use jpackage in two phases. The first phase creates application 
image and you use patchelf on launchers created by jpackage and then in the 
second phase you pack application image with modified launchers into rpm/deb 
package.
If this is the case and you would benefit from running jpackage in one phase, 
then I can think of adding to jpackage ability to run 3rd party script after 
application image creation. In this case you will need to create shell script 
using patchelf, place the script in resource directory so that jpackage can run 
the script after application image is created but before building rpm/deb 
package.

- Alexey

On 3/5/2020 9:42 AM, Auclair, Andrew wrote:
> Hi,
>
> We are in the process of upgrading to jpackage in Java 14 from javapackager 
> in Java 10. So far the transition has been smooth and we're really liking 
> jpackage.
>
> One question has come up regarding our Linux install. Currently we are using 
> Patchelf on CentOS to set an rpath for the executables generated by 
> javapackager. We have continued this with jpackage, but we were wondering if 
> there's a way to set the rpath with jpackage to avoid using Patchelf.
>
> Thanks,
>
> Andrew Auclair
> Software Engineer
> Tactical Communications Group
> A Curtiss-Wright Company
> 2 Highwood Drive, Building 2, Suite 200 Tewksbury, MA  01876-1157
>
> --
> This e-mail and any files transmitted with it are proprietary and intended 
> solely for the use of the individual or entity to whom they are addressed. If 
> you have reason to believe that you have received this e-mail in error, 
> please notify the sender and destroy this e-mail and any attached files. 
> Please note that any views or opinions presented in this e-mail are solely 
> those of the author and do not necessarily represent those of the 
> Curtiss-Wright Corporation or any of its subsidiaries. Documents attached 
> hereto may contain technology subject to the U.S. Export Regulations. 
> Recipient is solely responsible for ensuring that any re-export, transfer or 
> disclosure of this information is in accordance with U.S. Export Regulations. 
> The recipient should check this e-mail and any attachments for the presence 
> of viruses. Curtiss-Wright Corporation and its subsidiaries accept no 
> liability for any damage caused by any virus transmitted by this e-mail.


--
This e-mail and any files transmitted with it are proprietary and intended 
solely for the use of the individual or entity to whom they are addressed. If 
you have reason to believe that you have received this e-mail in error, please 
notify the sender and destroy this e-mail and any attached files. Please note 
that any views or opinions presented in this e-mail are solely those of the 
author and do not necessarily represent those of the Curtiss-Wright Corporation 
or any of its subsidiaries. Documents attached hereto may contain technology 
subject to the U.S. Export Regulations. Recipient is solely responsible for 
ensuring that any re-export, transfer or disclosure of this information is in 
accordance with U.S. Export Regulations. The recipient should check this e-mail 
and any attachments for the presence of viruses. Curtiss-Wright Corporation and 
its subsidiaries accept no liability for any damage caused by any virus 
transmitted by this e-mail.

Re: jpackage Linux rpath

[Alexey Semenyuk]

Hi Andrew,

jpackage is providing similar functionality as javapackager, so I think 
you need to keep using patchelf with jpackage as you used it with 
javapackager.
I assume you use jpackage in two phases. The first phase creates 
application image and you use patchelf on launchers created by jpackage 
and then in the second phase you pack application image with modified 
launchers into rpm/deb package.
If this is the case and you would benefit from running jpackage in one 
phase, then I can think of adding to jpackage ability to run 3rd party 
script after application image creation. In this case you will need to 
create shell script using patchelf, place the script in resource 
directory so that jpackage can run the script after application image is 
created but before building rpm/deb package.


- Alexey

On 3/5/2020 9:42 AM, Auclair, Andrew wrote:

Hi,

We are in the process of upgrading to jpackage in Java 14 from javapackager in 
Java 10. So far the transition has been smooth and we're really liking jpackage.

One question has come up regarding our Linux install. Currently we are using 
Patchelf on CentOS to set an rpath for the executables generated by 
javapackager. We have continued this with jpackage, but we were wondering if 
there's a way to set the rpath with jpackage to avoid using Patchelf.

Thanks,

Andrew Auclair
Software Engineer
Tactical Communications Group
A Curtiss-Wright Company
2 Highwood Drive, Building 2, Suite 200
Tewksbury, MA  01876-1157

--
This e-mail and any files transmitted with it are proprietary and intended 
solely for the use of the individual or entity to whom they are addressed. If 
you have reason to believe that you have received this e-mail in error, please 
notify the sender and destroy this e-mail and any attached files. Please note 
that any views or opinions presented in this e-mail are solely those of the 
author and do not necessarily represent those of the Curtiss-Wright Corporation 
or any of its subsidiaries. Documents attached hereto may contain technology 
subject to the U.S. Export Regulations. Recipient is solely responsible for 
ensuring that any re-export, transfer or disclosure of this information is in 
accordance with U.S. Export Regulations. The recipient should check this e-mail 
and any attachments for the presence of viruses. Curtiss-Wright Corporation and 
its subsidiaries accept no liability for any damage caused by any virus 
transmitted by this e-mail.

Re: RFC: JEP JDK-8208089: Implement C++14 Language Features

[John Paul Adrian Glaubitz]

On 3/3/20 10:26 AM, Andrew Haley wrote:
> But one of the few things of which I am certain is that we must not
> allow the needs of backporting to determine the future of Java. That's
> the way of staying in the past.

Unpopular opinion: It's the enterprise customers that mainly pay for the
development of software, not the users of rolling release distributions.

I know that maintaining old stuff is boring but that's where the money
is made. Too many developers unfortunately seem to forget that.

> As Patrick Head put it, “Some people tell me that Formula 1 would be
> better if the drivers still used stick shifts, but that’s a bit like
> saying, 'isn’t it a pity we don’t still walk around in clogs!'”

Maintenance of stable software isn't done for nostalgic reasons, it's
because enterprise customers need a reliable and stable platform to
run their businesses on. A lot of businesses are still running on JDK-8
for this reason.

Running bleeding edge software might be an option for a single desktop
user, but not in an enterprise environment with thousands of users
or in a critical environment like the booking system of an airline.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: RFR: 8222000: JFR: Process start event

[Roger Riggs]

Hi Erik,

The update looks fine.

On 3/5/20 12:21 PM, Erik Gahlin wrote:

Hi Roger,

Thanks for the feedback.

Runtime.exec doesn't take arg[0] as a separate parameter, so it may 
make sense to not split it up in the event. For example, if plan is to 
execute "hg clone url", but the user forgets to add "hg", then it will 
look as if "clone" is the command, which could be confusing.
The main interface for launching processes is ProcessBuilder. 
Runtime.exec is a legacy interface and no longer drives the terminology.


I will change the field name to 'command' to make it consistent with 
the single arg form of Runtime.exec.

tnx


Regarding delimiters, I think there are two use cases. Making 
something easy for humans to read and making it unambiguous for 
machine parsing. Since comma, or comma space, might be part of an 
argument, it is still unsafe for a parser. It would be possible to add 
escape sequences, but then it would be even harder for humans to read, 
which I think is the main use case. If we in the future would add 
support for serializing a String[], we could add another field, i.e. 
commandArray, which would work for machines without special parse logic.
understood, it wasn't clear what the focus of a string in the JFR stream 
is intended to mean or how it would be interpreted. I'm thinking that 
more and more it will be tools and AI's (not people) that will be 
looking through the reams and reams of output at scale.


There is no ProcessEnd event today, but that is something that could 
be added in the future. The exit status could be useful.

one step at a time.

Thanks, Roger


Updated webrev:
http://cr.openjdk.java.net/~egahlin/8222000/

Thanks
Erik

On 2020-03-05 15:27, Roger Riggs wrote:

Hi Erik,

Would the event be more useful if the executable arg[0] was a 
separate field from the arguments?

Though I exect it depends on what is later evaluating the event fields.

If all of cmdarrray is being joined, then the event field name is 
more appropriately
called 'command';  similar to the single arg form of Runtime.exec and 
new ProcessBuilder(...).


A small point but the joining with ' ' space has the usual problem of 
trying to parse out the arguments later.

using ',' comma might be less ambiguous.  (and Arrays.toString).

Otherwise, looks fine to me.

Is there already a Process exited event? Its not quite as convenient 
to instrument but useful if it captures the exit status. (ProcessImpl 
- Windows and Linux versions).


Roger



On 3/5/20 9:10 AM, Erik Gahlin wrote:

Hi

Could I have review of a JFR event that is emitted when a process is 
started from Java.


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

Webrev:
http://cr.openjdk.java.net/~egahlin/8222000/

Testing:
tier1,tier2 + jdk/jdk/jfr

Thanks
Erik

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

[Erik Gahlin]

Yes, I think it is fine to export from java.base to jdk.jfr for this 
feature.


If you would add support for other buffer pools, it should not be over 
MXBeans, but using classes/interfaces that is available in java.base.


Erik

On 2020-03-05 16:19, Denghui Dong wrote:

Hi Alan and Erik,

Sorry, I'm not sure if I fully understand what you mean.
For this feature, I still think exporting jdk.internal.access to jdk.jfr is a 
good solutionsince there are
some other packages has already exported to jdk.jfr in this module.

Thanks,
Denghui Dong

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

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

We should avoid adding a dependency on java.management. The
jdk.jfr module today only depends java.base so JFR can be used in
constrained environments. All JMX related functionality is in
jdk.management.jfr.

Erik

On 5 Mar 2020, at 10:54, Denghui Dong mailto:denghui@alibaba-inc.com>> wrote:

Hi Alan,

If I use ManagementFactoryHelper to get memory pools, I still
need to modify src/java.management/share/classes/module-info.java
to export sun.management, right?

Thanks,
Denghui Dong
--
From:Alan Bateman mailto:alan.bate...@oracle.com>>
Send Time:2020年3月5日(星期四) 16:46
To:董登辉(卓昂) mailto:denghui@alibaba-inc.com>>; Erik Gahlin
mailto:erik.gah...@oracle.com>>;
hotspot-jfr-dev mailto:hotspot-jfr-...@openjdk.java.net>>; core-libs-dev
mailto:core-libs-dev@openjdk.java.net>>
Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics

On 05/03/2020 02:44, Denghui Dong wrote:
> Hi Erik,
> Updated.
> Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.01/
>
ManagementFactoryHelper has a method to get the list of MXBeans for the

buffer pools. That could be easily changed to return an unmodifable
list. That would allow you to emit statistics for memory-mapped files
too (the current patch seems to be limited to emitting stats for direct

buffers).

-Alan.

RFR 8240524: Removed warnings from test classes

[Vipin Sharma]

Hi All,

Please review patch to remove warnings from test classes.

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

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



Change description:

Class: test/jdk/java/lang/Boolean/GetBoolean.java
Fixed following warning:
1. "Exception 'java.lang.Exception' is never thrown in the method”

Class: test/jdk/java/lang/Boolean/MakeBooleanComparable.java
Fixed following warnings:
1. Explicit type argument Boolean can be replaced with <> 
2. C-style array declaration of parameter 'args' 

Class: test/jdk/java/lang/Boolean/ParseBoolean.java
Fixed following warning:
1. Exception 'java.lang.Exception' is never thrown in the method


Regards,
Vipin

Re: RFR: 8222000: JFR: Process start event

[Erik Gahlin]

Hi Roger,

Thanks for the feedback.

Runtime.exec doesn't take arg[0] as a separate parameter, so it may make 
sense to not split it up in the event. For example, if plan is to 
execute "hg clone url", but the user forgets to add "hg", then it will 
look as if "clone" is the command, which could be confusing.


I will change the field name to 'command' to make it consistent with the 
single arg form of Runtime.exec.


Regarding delimiters, I think there are two use cases. Making something 
easy for humans to read and making it unambiguous for machine parsing. 
Since comma, or comma space, might be part of an argument, it is still 
unsafe for a parser. It would be possible to add escape sequences, but 
then it would be even harder for humans to read, which I think is the 
main use case. If we in the future would add support for serializing a 
String[], we could add another field, i.e. commandArray, which would 
work for machines without special parse logic.


There is no ProcessEnd event today, but that is something that could be 
added in the future. The exit status could be useful.


Updated webrev:
http://cr.openjdk.java.net/~egahlin/8222000/

Thanks
Erik

On 2020-03-05 15:27, Roger Riggs wrote:

Hi Erik,

Would the event be more useful if the executable arg[0] was a separate 
field from the arguments?

Though I exect it depends on what is later evaluating the event fields.

If all of cmdarrray is being joined, then the event field name is more 
appropriately
called 'command';  similar to the single arg form of Runtime.exec and 
new ProcessBuilder(...).


A small point but the joining with ' ' space has the usual problem of 
trying to parse out the arguments later.

using ',' comma might be less ambiguous.  (and Arrays.toString).

Otherwise, looks fine to me.

Is there already a Process exited event? Its not quite as convenient 
to instrument but useful if it captures the exit status. (ProcessImpl 
- Windows and Linux versions).


Roger



On 3/5/20 9:10 AM, Erik Gahlin wrote:

Hi

Could I have review of a JFR event that is emitted when a process is 
started from Java.


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

Webrev:
http://cr.openjdk.java.net/~egahlin/8222000/

Testing:
tier1,tier2 + jdk/jdk/jfr

Thanks
Erik

Re: RFR(S): 8232081: Try to link all classes during dynamic CDS dump

[Ioi Lam]

Hi Calvin,

Looks good overall. I just have two comment:

I think this is not needed:

 306 void ConstantPool::resolve_class_constants(TRAPS) {
 307   Arguments::assert_is_dumping_archive();

because this function is used to resolve all Strings in the statically 
dumped classes to archive all the Strings. However, the archive heap is 
not supported for the dynamic archive. So I think it's better to avoid 
calling this function from LinkSharedClassesClosure for dynamic dump.


LinkSharedClassesClosure::_is_static -- maybe we can avoid adding this 
field and just check "if (DumpSharedSpaces)" instead?


Thanks
- Ioi





On 3/4/20 9:13 PM, Calvin Cheung wrote:

Hi David, Ioi,

Here's an updated webrev which doesn't involve changing 
InstanceKlass::verify_on() and made use of the changes for JDK-8240481.


    http://cr.openjdk.java.net/~ccheung/jdk15/8232081/webrev.03/

thanks,
Calvin

On 3/1/20 10:14 PM, David Holmes wrote:

Hi Calvin,

On 28/02/2020 4:12 pm, Calvin Cheung wrote:

Hi David,

On 2/27/20 5:40 PM, David Holmes wrote:

Hi Calvin, Ioi,

Looking good - comments below.

A meta-question: normal application classes are rarely loaded but 
not linked so I'm a little surprised this is an issue. What is the 
main source of such classes - generated classes like lambda forms? 
Also why do we need to link them when loading from the archive if 
they were never linked when the application actually executed ??


I saw that Ioi already answered the above.

I'll try to answer your questions inline below..


Responses inline below ...



On 28/02/2020 10:48 am, Calvin Cheung wrote:

Hi David, Ioi,

Thanks for your review and suggestions.

On 2/26/20 9:46 PM, Ioi Lam wrote:



On 2/26/20 7:50 PM, David Holmes wrote:

Hi Calvin,

Adding core-libs-dev as you are messing with their code :)

On 27/02/2020 1:19 pm, Calvin Cheung wrote:

JBS: https://bugs.openjdk.java.net/browse/JDK-8232081
webrev: 
http://cr.openjdk.java.net/~ccheung/jdk15/8232081/webrev.00/


The proposed changeset for this RFE adds a 
JVM_LinkClassesForCDS() function to be called from 
java/lang/Shutdown to notify the JVM to link the classes loaded 
by the builtin class loaders. The 


This would be much less disruptive if this was handled purely on 
the VM side once we have started shutdown. No need to make any 
changes to the Java side then, nor jvm.cpp.




Hi David,

To link the classes, we need to be able to run Java code -- when 
linking a class X loaded by the app loader, X will be verified. 
During verification of X, we may need to load additional classes 
from the app loader, which executes Java code during its class 
loading operations.


We also need to handle all the exit conditions. As far as I can 
tell, an app can exit the JVM in two ways:


(1) Explicitly calling System.exit(). This will call 
java.lang.Shutdown.exit() which does this:


http://hg.openjdk.java.net/jdk/jdk/file/0edc7fd0d7a3/src/java.base/share/classes/java/lang/Shutdown.java#l163 



    beforeHalt(); // native
    runHooks();
    halt(status);

(2) When all non-daemon threads have died (e.g., falling out of 
the bottom of HelloWorld.main()). There's no explicit call to 
System.exit(), but the JVM will proactively call 
java.lang.Shutdown.shutdown() inside 
JavaThread::invoke_shutdown_hooks()


http://hg.openjdk.java.net/jdk/jdk/file/0edc7fd0d7a3/src/hotspot/share/runtime/thread.cpp#l4331 

http://hg.openjdk.java.net/jdk/jdk/file/0edc7fd0d7a3/src/java.base/share/classes/java/lang/Shutdown.java#l184 



If we want to avoid modifying the Java code, I think we can 
intercept JVM_BeforeHalt() and 
JavaThread::invoke_shutdown_hooks(). This way we should be able 
to handle all the classes (except those that are loaded inside 
Shutdown.runHooks).


I've implemented the above. No changes to the Java side.

updated webrev:

http://cr.openjdk.java.net/~ccheung/jdk15/8232081/webrev.01/


This looks much better to me. I wasn't sure if you were that 
concerned about catching the classes loaded by the Shutdown hooks, 
but it is good you are not as it seems problematic to me to trigger 
class loading etc after the shutdown hooks have executed - you may 
hit unexpected conditions. So this approach is good.


A few minor comments:

src/hotspot/share/memory/metaspaceShared.cpp
src/hotspot/share/memory/metaspaceShared.hpp

Why the change from TRAPS to "Thread* THREAD"?

I forgot why I changed it but I've changed it back and it still works.


Ok.



---

src/hotspot/share/oops/instanceKlass.cpp

I'm not clear how verify_on is used so am unsure how the additional 
error state check may affect code unrelated to dynamic dumping ??


Some of the appcds tests by design have some classes which fail 
verification. Without the change, the following assert would fail:


void vtableEntry::verify(klassVtable* vt, outputStream* st) {
   Klass* vtklass = vt->klass();
   if (vtklass->is_instance_klass() &&
  (InstanceKlass::cast(vtklass)->major_version() >= 
klassVtable::V

[15] RFR: 8216332: Grapheme regex does not work with emoji sequences

[naoto . sato]

Hello,

Please review the fix to the following issue:

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

The proposed changeset is located at:

https://cr.openjdk.java.net/~naoto/8216332/webrev.00/

Before this fix, the rule GB11 (Do not break within emoji modifier 
sequences or emoji zwj sequences [1]) was only applied once, and was 
ignored thereafter for the repeated possible rule candidates. The fix is 
to simply stop resetting the GB11 mode.


Naoto

[1] https://unicode.org/reports/tr29/#GB11

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

[Denghui Dong]

Hi Alan and Erik,

Sorry, I'm not sure if I fully understand what you mean.
For this feature, I still think exporting jdk.internal.access to jdk.jfr is a 
good solutionsince there are
some other packages has already exported to jdk.jfr in this module.

Thanks,
Denghui Dong

--
From:Erik Gahlin 
Send Time:2020年3月5日(星期四) 19:55
To:董登辉(卓昂) 
Cc:hotspot-jfr-dev ; core-libs-dev 
; Alan Bateman 
Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics

We should avoid adding a dependency on java.management. The jdk.jfr module 
today only depends java.base so JFR can be used in constrained environments. 
All JMX related functionality is in jdk.management.jfr.

Erik

On 5 Mar 2020, at 10:54, Denghui Dong  wrote:
Hi Alan,

If I use ManagementFactoryHelper to get memory pools, I still need to modify 
src/java.management/share/classes/module-info.java
to export sun.management, right?

Thanks,
Denghui Dong
--
From:Alan Bateman 
Send Time:2020年3月5日(星期四) 16:46
To:董登辉(卓昂) ; Erik Gahlin ; 
hotspot-jfr-dev ; core-libs-dev 

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

On 05/03/2020 02:44, Denghui Dong wrote:
> Hi Erik,
> Updated.
> Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.01/
>
ManagementFactoryHelper has a method to get the list of MXBeans for the 
buffer pools. That could be easily changed to return an unmodifable 
list. That would allow you to emit statistics for memory-mapped files 
too (the current patch seems to be limited to emitting stats for direct 
buffers).

-Alan.

RE: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start

[Langer, Christoph]

Hi Lance,

The revised webrev is at 
http://cr.openjdk.java.net/~lancea/8211917/webrev.02/index.html

This looks good to me now. Ship it 😊

Cheers
Christoph

jpackage Linux rpath

[Auclair, Andrew]

Hi,

We are in the process of upgrading to jpackage in Java 14 from javapackager in 
Java 10. So far the transition has been smooth and we're really liking jpackage.

One question has come up regarding our Linux install. Currently we are using 
Patchelf on CentOS to set an rpath for the executables generated by 
javapackager. We have continued this with jpackage, but we were wondering if 
there's a way to set the rpath with jpackage to avoid using Patchelf.

Thanks,

Andrew Auclair
Software Engineer
Tactical Communications Group
A Curtiss-Wright Company
2 Highwood Drive, Building 2, Suite 200
Tewksbury, MA  01876-1157

--
This e-mail and any files transmitted with it are proprietary and intended 
solely for the use of the individual or entity to whom they are addressed. If 
you have reason to believe that you have received this e-mail in error, please 
notify the sender and destroy this e-mail and any attached files. Please note 
that any views or opinions presented in this e-mail are solely those of the 
author and do not necessarily represent those of the Curtiss-Wright Corporation 
or any of its subsidiaries. Documents attached hereto may contain technology 
subject to the U.S. Export Regulations. Recipient is solely responsible for 
ensuring that any re-export, transfer or disclosure of this information is in 
accordance with U.S. Export Regulations. The recipient should check this e-mail 
and any attachments for the presence of viruses. Curtiss-Wright Corporation and 
its subsidiaries accept no liability for any damage caused by any virus 
transmitted by this e-mail.

RE: RFR: 8239365: ProcessBuilder/Basic.java test modifications for AIX execution

[Adam Farley8]

Hi All,

As mentioned by Tom, a third opinion is sought on the aforementioned 
minor test change.

@Tom - I'd prefer to use the full version of the error messages to keep
things clear, though I would not object to using your abbreviated version 
if it means getting the change in.

So long as it passes when it should pass, and fails only when it should
fail, clarity is a "would be nice", but ultimately a secondary priority.

Thanks for your feedback. :)

Best Regards

Adam Farley 
IBM Runtimes


"Thomas Stüfe"  wrote on 03/03/2020 10:52:10:

> From: "Thomas Stüfe" 
> To: Adam Farley8 
> Cc: core-libs-dev 
> Date: 03/03/2020 10:52
> Subject: [EXTERNAL] Re: RFR: 8239365: ProcessBuilder/Basic.java test
> modifications for AIX execution
> 
> This is why I always was against handing up the result of strerror 
> to the user :) The same problem we would have when running with 
> different locales. We should have a platform agnostic string table 
> in the java lib for that purpose...
> 
> As for the test, looks good, but I personally would shorten the AIX 
> patterns a bit or maybe try to find a short form fitting all 
> platforms (e.g. "[Pp]ermission"). 
> 
> But thats just idle bikeshedding, lets see what others think.
> 
> Cheers Thomas
> 
> On Tue, Mar 3, 2020 at 11:43 AM Adam Farley8  
wrote:
> Hi All,
> 
> Reviews and sponsor requested for a small test change.
> 
> Short version: When an AIX machine has the file set "bos.msg.en_US.rte", 

> the error messages are not in a form that the test expects, causing 
> failure.
> 
> The simplest option appears to be adding the second potential form of 
the 
> message into the regex (see webrev).
> 
> http://cr.openjdk.java.net/~afarley/8239365.1/webrev/
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8239365 
> 
> Best Regards
> 
> Adam Farley 
> IBM Runtimes
> Unless stated otherwise above:
> IBM United Kingdom Limited - Registered in England and Wales with number 

> 741598. 
> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 
3AU

Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Re: RFR: 8222000: JFR: Process start event

[Roger Riggs]

Hi Erik,

Would the event be more useful if the executable arg[0] was a separate 
field from the arguments?

Though I exect it depends on what is later evaluating the event fields.

If all of cmdarrray is being joined, then the event field name is more 
appropriately
called 'command';  similar to the single arg form of Runtime.exec and 
new ProcessBuilder(...).


A small point but the joining with ' ' space has the usual problem of 
trying to parse out the arguments later.

using ',' comma might be less ambiguous.  (and Arrays.toString).

Otherwise, looks fine to me.

Is there already a Process exited event? Its not quite as convenient to 
instrument but useful if it captures the exit status. (ProcessImpl - 
Windows and Linux versions).


Roger



On 3/5/20 9:10 AM, Erik Gahlin wrote:

Hi

Could I have review of a JFR event that is emitted when a process is 
started from Java.


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

Webrev:
http://cr.openjdk.java.net/~egahlin/8222000/

Testing:
tier1,tier2 + jdk/jdk/jfr

Thanks
Erik

RE: RFR: 8222000: JFR: Process start event

[Markus Gronlund]

Hi Erik,

Looks good. Quite straightforward, thank you for adding this capability.

Thanks
Markus

-Original Message-
From: Erik Gahlin 
Sent: den 5 mars 2020 15:11
To: hotspot-jfr-dev ; 
core-libs-dev@openjdk.java.net
Subject: RFR: 8222000: JFR: Process start event

Hi

Could I have review of a JFR event that is emitted when a process is started 
from Java.

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

Webrev:
http://cr.openjdk.java.net/~egahlin/8222000/

Testing:
tier1,tier2 + jdk/jdk/jfr

Thanks
Erik

RFR: 8222000: JFR: Process start event

[Erik Gahlin]

Hi

Could I have review of a JFR event that is emitted when a process is 
started from Java.


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

Webrev:
http://cr.openjdk.java.net/~egahlin/8222000/

Testing:
tier1,tier2 + jdk/jdk/jfr

Thanks
Erik

Re: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start

[Lance Andersen]

Hi Christoph

> On Mar 5, 2020, at 4:03 AM, Langer, Christoph  
> wrote:
> 
> Hi Lance,
> 
> Thanks for addressing my points. Here my answer to those things which weren't 
> in full agreement yet:

Please see below
> 
>> I dislike a bit the fact that we introduce a new testng subfolder in
>> test/jdk/nio/zipfs. Wouldn’t it be possible to consolidate in the current 
>> test
>> folder?
>> 
>> I am trying to add some organization separating the non-testng  tests from
>> the  testng tests and other testng tests will be moved here.  I did the same
>> for JDBC a few years back
> 
> ok, maybe it's a good idea to do this here and gradually move all testng 
> tests over.
> 
>> - line 387: Manfiest -> Manifest
> 
> I think you missed that one

Fixed must have not saved it but it is there now
> 
> 
>> - line 417: Parameter "final Map
>> attributes" of ManifestOrderTest::verify should be renamed to
>> "manifestAttributes" to make it easier to understand its purpose
>> 
>> 
>> Prefer to leave as is as it gets wordy and I believe the description is 
>> clear in
>> the comments
> 
> Hm, I needed to look twice to grasp it. So, I'd still prefer something with 
> "manifest" in the variable name. But I leave it to you since it's probably a 
> personal taste thing 😉

I left the variable name as to your point it sometimes is a personal preference 
;-) 
> 
> However, two more things here:
> 
> The Javadoc of verify says (line 412):
> * @param attributes  Is there a Manifest to check
> 
> You should rather go with the Javadoc of validateManifest here as well:
> * @param attributes A Map containing the attributes expected in the Manifest;
> *   otherwise empty

Updated…  Thought I had done that previously as that @param was originally a 
boolean ….
> 
> Also, I spotted in the Javadoc, line 413:
> * @param entries Entries to validate are in the JAR
> 
> I guess the "are" is wrong here.

Fixed

> 
>> test/jdk/jdk/nio/zipfs/testng/util/ZipFsBaseTest.java:
>> - rename to ZipFSBaseTest (Capital ‘S’)??
>> hmm  I had that originally but did not like it…  but don’t have a strong
>> preference
> 
> Ok, leave it as you have it 😊

:-)
> 
>> - line 120: public static void verify - > this method is not used by
>> ManifestOrderTest. I think we should only add it when having a test that
>> really uses this verify method. But I generally agree that the verify method 
>> is
>> a candidate for communalization
>> 
>> This will be used by some tests I have created and some I will be moving so
>> rather add it now on the initial push.  This is used by several tests that 
>> will be
>> migrated
>> 
>> - line 176: public void zip: dito, this method doesn’t seem used. So I 
>> suggest
>> to remove it for this change
>> Same comment as above
> 
> OK.
> 
>> The webrev for the above
>> is http://cr.openjdk.java.net/~lancea/8211917/webrev.01/index.html
> 
> Looks good, apart from my comments above.

Thank you again for the review

The revised webrev is at 
http://cr.openjdk.java.net/~lancea/8211917/webrev.02/index.html 


Best
Lance
> 
> Thanks
> Christoph
> 

 
  

 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]

We should avoid adding a dependency on java.management. The jdk.jfr module 
today only depends java.base so JFR can be used in constrained environments. 
All JMX related functionality is in jdk.management.jfr.

Erik

> On 5 Mar 2020, at 10:54, Denghui Dong  wrote:
> 
> Hi Alan,
> 
> If I use ManagementFactoryHelper to get memory pools, I still need to modify 
> src/java.management/share/classes/module-info.java
> to export sun.management, right?
> 
> Thanks,
> Denghui Dong
> --
> From:Alan Bateman 
> Send Time:2020年3月5日(星期四) 16:46
> To:董登辉(卓昂) ; Erik Gahlin 
> ; hotspot-jfr-dev ; 
> core-libs-dev 
> Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics
> 
> On 05/03/2020 02:44, Denghui Dong wrote:
> > Hi Erik,
> > Updated.
> > Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.01/
> >
> ManagementFactoryHelper has a method to get the list of MXBeans for the 
> buffer pools. That could be easily changed to return an unmodifable 
> list. That would allow you to emit statistics for memory-mapped files 
> too (the current patch seems to be limited to emitting stats for direct 
> buffers).
> 
> -Alan.

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

[Alan Bateman]

On 05/03/2020 09:54, Denghui Dong wrote:

Hi Alan,

If I use ManagementFactoryHelper to get memory pools, I still need to 
modify src/java.management/share/classes/module-info.java

to export sun.management, right?

I should have been clearer in my comment. I think it should be possible 
to put a helper in somewhere like jdk.internal.misc to replace 
ManagementFactoryHelper.getBufferPoolMXBeans(). It can return an 
unmodifable list of the MXBeans for the buffer pools. This should allow 
periodic stats/events for buffer pools other than the direct buffer pool.


-Alan.

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

[Denghui Dong]

Hi Alan,

If I use ManagementFactoryHelper to get memory pools, I still need to modify 
src/java.management/share/classes/module-info.java
to export sun.management, right?

Thanks,
Denghui Dong
--
From:Alan Bateman 
Send Time:2020年3月5日(星期四) 16:46
To:董登辉(卓昂) ; Erik Gahlin ; 
hotspot-jfr-dev ; core-libs-dev 

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

On 05/03/2020 02:44, Denghui Dong wrote:
> Hi Erik,
> Updated.
> Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.01/
>
ManagementFactoryHelper has a method to get the list of MXBeans for the 
buffer pools. That could be easily changed to return an unmodifable 
list. That would allow you to emit statistics for memory-mapped files 
too (the current patch seems to be limited to emitting stats for direct 
buffers).

-Alan.

RE: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start

[Langer, Christoph]

Hi Lance,

Thanks for addressing my points. Here my answer to those things which weren't 
in full agreement yet:

> I dislike a bit the fact that we introduce a new testng subfolder in
> test/jdk/nio/zipfs. Wouldn’t it be possible to consolidate in the current test
> folder?
> 
> I am trying to add some organization separating the non-testng  tests from
> the  testng tests and other testng tests will be moved here.  I did the same
> for JDBC a few years back

ok, maybe it's a good idea to do this here and gradually move all testng tests 
over.

> - line 387: Manfiest -> Manifest

I think you missed that one


> - line 417: Parameter "final Map
> attributes" of ManifestOrderTest::verify should be renamed to
> "manifestAttributes" to make it easier to understand its purpose
> 
> 
> Prefer to leave as is as it gets wordy and I believe the description is clear 
> in
> the comments

Hm, I needed to look twice to grasp it. So, I'd still prefer something with 
"manifest" in the variable name. But I leave it to you since it's probably a 
personal taste thing 😉

However, two more things here:

The Javadoc of verify says (line 412):
* @param attributes  Is there a Manifest to check

You should rather go with the Javadoc of validateManifest here as well:
* @param attributes A Map containing the attributes expected in the Manifest;
*   otherwise empty

Also, I spotted in the Javadoc, line 413:
* @param entries Entries to validate are in the JAR

I guess the "are" is wrong here.


> test/jdk/jdk/nio/zipfs/testng/util/ZipFsBaseTest.java:
> - rename to ZipFSBaseTest (Capital ‘S’)??
> hmm  I had that originally but did not like it…  but don’t have a strong
> preference

Ok, leave it as you have it 😊

> - line 120: public static void verify - > this method is not used by
> ManifestOrderTest. I think we should only add it when having a test that
> really uses this verify method. But I generally agree that the verify method 
> is
> a candidate for communalization
> 
> This will be used by some tests I have created and some I will be moving so
> rather add it now on the initial push.  This is used by several tests that 
> will be
> migrated
> 
> - line 176: public void zip: dito, this method doesn’t seem used. So I suggest
> to remove it for this change
> Same comment as above

OK.

> The webrev for the above
> is http://cr.openjdk.java.net/~lancea/8211917/webrev.01/index.html

Looks good, apart from my comments above.

Thanks
Christoph

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

[Alan Bateman]

On 05/03/2020 02:44, Denghui Dong wrote:

Hi Erik,
Updated.
Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.01/

ManagementFactoryHelper has a method to get the list of MXBeans for the 
buffer pools. That could be easily changed to return an unmodifable 
list. That would allow you to emit statistics for memory-mapped files 
too (the current patch seems to be limited to emitting stats for direct 
buffers).


-Alan.