Re: RFR: 8264285: Clean the modification of ccstr JVM flags [v2]

[David Holmes]

On 1/04/2021 3:29 pm, Ioi Lam wrote:



On 3/31/21 10:22 PM, David Holmes wrote:

On 1/04/2021 5:05 am, Ioi Lam wrote:
On Wed, 31 Mar 2021 12:58:50 GMT, Coleen Phillimore 
 wrote:




36: // have any MANAGEABLE flags of the ccstr type, but we really 
need to
37: // make sure the implementation is correct (in terms of memory 
allocation)

38: // just in case someone may add such a flag in the future.


Could you have just added a develop flag to the manageable flags 
instead?


I had to use a `product` flag due to the following code, which should 
have been removed as part of 
[JDK-8243208](https://bugs.openjdk.java.net/browse/JDK-8243208), but 
I was afraid to do so because I didn't have a test case. I.e., all of 
our diagnostic/manageable/experimental flags were `product` flags.


With this PR, now I have a test case -- I changed 
`DummyManageableStringFlag` to a `notproduct` flag, and removed the 
following code. I am re-running tiers1-4 now.


Sorry but I don't understand how a test involving one flag replaces a 
chunk of code that validated every flag declaration ??




When I did JDK-8243208, I wasn't sure if the VM would actually support 
diagnostic/manageable/experimental flags that were not `product`. So I 
added check_all_flag_declarations() to prevent anyone from adding such a 
flag "casually" without thinking about.


Now that I have added such a flag, and I believe I have tested pretty 
thoroughly (tiers 1-4), I think the VM indeed supports these flags. So 
now I feel it's safe to remove check_all_flag_declarations().


But the check was checking two things:

1. That diagnostic/manageable/experimental are mutually exclusive
2. That they only apply to product flags

I believe both of these rules still stand based on what we defined such 
attributes to mean. And even if you think #2 should not apply, #1 still 
does and so could still be checked. And if #2 is no longer our rule then 
the documentation in globals.hpp should be updated - though IMHO #2 
should remain as-is.


David
-



Thanks
- Ioi




David
-


void JVMFlag::check_all_flag_declarations() {
   for (JVMFlag* current = [0]; current->_name != NULL; 
current++) {

 int flags = static_cast(current->_flags);
 // Backwards compatibility. This will be relaxed/removed in 
JDK-7123237.
 int mask = JVMFlag::KIND_DIAGNOSTIC | JVMFlag::KIND_MANAGEABLE | 
JVMFlag::KIND_EXPERIMENTAL;

 if ((flags & mask) != 0) {
   assert((flags & mask) == JVMFlag::KIND_DIAGNOSTIC ||
  (flags & mask) == JVMFlag::KIND_MANAGEABLE ||
  (flags & mask) == JVMFlag::KIND_EXPERIMENTAL,
  "%s can be declared with at most one of "
  "DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL", current->_name);
   assert((flags & KIND_NOT_PRODUCT) == 0 &&
  (flags & KIND_DEVELOP) == 0,
  "%s has an optional DIAGNOSTIC, MANAGEABLE or 
EXPERIMENTAL "
  "attribute; it must be declared as a product flag", 
current->_name);

 }
   }
}

-

PR: https://git.openjdk.java.net/jdk/pull/3254

Re: RFR: 8264285: Clean the modification of ccstr JVM flags [v2]

[Ioi Lam]

On 3/31/21 10:22 PM, David Holmes wrote:

On 1/04/2021 5:05 am, Ioi Lam wrote:
On Wed, 31 Mar 2021 12:58:50 GMT, Coleen Phillimore 
 wrote:




36: // have any MANAGEABLE flags of the ccstr type, but we really 
need to
37: // make sure the implementation is correct (in terms of memory 
allocation)

38: // just in case someone may add such a flag in the future.


Could you have just added a develop flag to the manageable flags 
instead?


I had to use a `product` flag due to the following code, which should 
have been removed as part of 
[JDK-8243208](https://bugs.openjdk.java.net/browse/JDK-8243208), but 
I was afraid to do so because I didn't have a test case. I.e., all of 
our diagnostic/manageable/experimental flags were `product` flags.


With this PR, now I have a test case -- I changed 
`DummyManageableStringFlag` to a `notproduct` flag, and removed the 
following code. I am re-running tiers1-4 now.


Sorry but I don't understand how a test involving one flag replaces a 
chunk of code that validated every flag declaration ??




When I did JDK-8243208, I wasn't sure if the VM would actually support 
diagnostic/manageable/experimental flags that were not `product`. So I 
added check_all_flag_declarations() to prevent anyone from adding such a 
flag "casually" without thinking about.


Now that I have added such a flag, and I believe I have tested pretty 
thoroughly (tiers 1-4), I think the VM indeed supports these flags. So 
now I feel it's safe to remove check_all_flag_declarations().


Thanks
- Ioi




David
-


void JVMFlag::check_all_flag_declarations() {
   for (JVMFlag* current = [0]; current->_name != NULL; 
current++) {

 int flags = static_cast(current->_flags);
 // Backwards compatibility. This will be relaxed/removed in 
JDK-7123237.
 int mask = JVMFlag::KIND_DIAGNOSTIC | JVMFlag::KIND_MANAGEABLE | 
JVMFlag::KIND_EXPERIMENTAL;

 if ((flags & mask) != 0) {
   assert((flags & mask) == JVMFlag::KIND_DIAGNOSTIC ||
  (flags & mask) == JVMFlag::KIND_MANAGEABLE ||
  (flags & mask) == JVMFlag::KIND_EXPERIMENTAL,
  "%s can be declared with at most one of "
  "DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL", current->_name);
   assert((flags & KIND_NOT_PRODUCT) == 0 &&
  (flags & KIND_DEVELOP) == 0,
  "%s has an optional DIAGNOSTIC, MANAGEABLE or 
EXPERIMENTAL "
  "attribute; it must be declared as a product flag", 
current->_name);

 }
   }
}

-

PR: https://git.openjdk.java.net/jdk/pull/3254

Re: RFR: 8264285: Clean the modification of ccstr JVM flags [v2]

[David Holmes]

On 1/04/2021 5:05 am, Ioi Lam wrote:

On Wed, 31 Mar 2021 12:58:50 GMT, Coleen Phillimore  wrote:


Ioi Lam has updated the pull request incrementally with one additional commit 
since the last revision:

   relax flag attributions (ala JDK-7123237)


src/hotspot/share/runtime/flags/debug_globals.hpp line 38:


36: // have any MANAGEABLE flags of the ccstr type, but we really need to
37: // make sure the implementation is correct (in terms of memory allocation)
38: // just in case someone may add such a flag in the future.


Could you have just added a develop flag to the manageable flags instead?


I had to use a `product` flag due to the following code, which should have been 
removed as part of 
[JDK-8243208](https://bugs.openjdk.java.net/browse/JDK-8243208), but I was 
afraid to do so because I didn't have a test case. I.e., all of our 
diagnostic/manageable/experimental flags were `product` flags.

With this PR, now I have a test case -- I changed `DummyManageableStringFlag` 
to a `notproduct` flag, and removed the following code. I am re-running 
tiers1-4 now.


Sorry but I don't understand how a test involving one flag replaces a 
chunk of code that validated every flag declaration ??


David
-


void JVMFlag::check_all_flag_declarations() {
   for (JVMFlag* current = [0]; current->_name != NULL; current++) {
 int flags = static_cast(current->_flags);
 // Backwards compatibility. This will be relaxed/removed in JDK-7123237.
 int mask = JVMFlag::KIND_DIAGNOSTIC | JVMFlag::KIND_MANAGEABLE | 
JVMFlag::KIND_EXPERIMENTAL;
 if ((flags & mask) != 0) {
   assert((flags & mask) == JVMFlag::KIND_DIAGNOSTIC ||
  (flags & mask) == JVMFlag::KIND_MANAGEABLE ||
  (flags & mask) == JVMFlag::KIND_EXPERIMENTAL,
  "%s can be declared with at most one of "
  "DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL", current->_name);
   assert((flags & KIND_NOT_PRODUCT) == 0 &&
  (flags & KIND_DEVELOP) == 0,
  "%s has an optional DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL "
  "attribute; it must be declared as a product flag", 
current->_name);
 }
   }
}

-

PR: https://git.openjdk.java.net/jdk/pull/3254

Re: RFR: 8259070: Add jcmd option to dump CDS [v8]

[Ioi Lam]

On Wed, 31 Mar 2021 20:58:46 GMT, Yumin Qi  wrote:

>> Hi, Please review
>> 
>>   Added jcmd option for dumping CDS archive during application runtime. 
>> Before this change, user has to dump shared archive in two steps: first run 
>> application with
>>   `java -XX:DumpLoadedClassList=  `  
>>  to collect shareable class names and saved in file `` , then 
>>   `java -Xshare:dump -XX:SharedClassListFile= 
>> -XX:SharedArchiveFile= ...`
>>   With this change, user can use jcmd to dump CDS without going through 
>> above steps. Also user can choose a moment during the app runtime  to dump 
>> an archive.
>>The bug is associated with the CSR: 
>> https://bugs.openjdk.java.net/browse/JDK-8259798 which has been approved.
>>New added jcmd option:
>>`jcmd  VM.cds static_dump `
>>or
>> `jcmd  VM.cds dynamic_dump `
>>   To dump dynamic archive, requires start app with newly added flag 
>> `-XX:+RecordDynamicDumpInfo`, with this flag, some information related to 
>> dynamic dump like loader constraints will be recorded. Note the dumping 
>> process changed some object memory locations so for dumping dynamic archive, 
>> can only done once for a running app. For static dump, user can dump 
>> multiple times against same process. 
>>The file name is optional, if the file name is not supplied, the file 
>> name will take format of `java_pid_static.jsa` or 
>> `java_pid_dynamic.jsa` for static and dynamic respectively. The 
>> `` is the application process ID.
>> 
>>   Tests: tier1,tier2,tier3,tier4
>> 
>> Thanks
>> Yumin
>
> Yumin Qi has updated the pull request with a new target base due to a merge 
> or a rebase. The pull request now contains 10 commits:
> 
>  - Fix revert unintentionally comment, merge master.
>  - Merge branch 'master' of ssh://github.com/yminqi/jdk into jdk-8259070
>  - Remove CDS.getVMArguments, changed to use VM.getRuntimeVMArguments. 
> Removed unused function from ClassLoader. Improved 
> InstanceKlass::is_shareable() and related test. Added more test scenarios.
>  - Remove redundant check for if a class is shareable
>  - Fix according to review comment and add more tests
>  - Fix filter more flags to exclude in static dump, add more test cases
>  - Merge branch 'master' into jdk-8259070
>  - Fix white space in CDS.java
>  - Add function CDS.dumpSharedArchive in java to dump shared archive
>  - 8259070: Add jcmd option to dump CDS

Changes requested by iklam (Reviewer).

src/hotspot/share/services/diagnosticCommand.cpp line 1106:

> 1104: output()->print_cr("Dynamic dump:");
> 1105: if (!UseSharedSpaces) {
> 1106:   output()->print_cr("CDS is not available for the JDK");

I think we should use a message similar to this:

$ java -Xshare:off -XX:ArchiveClassesAtExit=xxx -version
Error occurred during initialization of VM
DynamicDumpSharedSpaces is unsupported when base CDS archive is not loaded

How about "dynamic_dump is unsupported when base CDS archive is not loaded".

src/hotspot/share/services/diagnosticCommand.cpp line 1125:

> 1123:   Symbol* cds_name  = vmSymbols::jdk_internal_misc_CDS();
> 1124:   Klass*  cds_klass = SystemDictionary::resolve_or_fail(cds_name, true 
> /*throw error*/,  CHECK);
> 1125:   JavaValue result(T_OBJECT);

Should be `result(T_VOID)` to match the signature of `CDS.dumpSharedArchive()`

src/hotspot/share/services/diagnosticCommand.cpp line 1133:

> 1131:  vmSymbols::dumpSharedArchive(),
> 1132:  vmSymbols::dumpSharedArchive_signature(),
> 1133:  , THREAD);

Should be `, CHECK);`. Recently, we have started to to avoid using 
`THREAD` if the  callee function may throw an exception.

src/java.base/share/classes/jdk/internal/misc/CDS.java line 218:

> 216: try {
> 217: PrintStream prt = new PrintStream(fileName);
> 218: prt.println("Command:");

[Try-with-resources](https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html)
 should be used to make sure the streams are closed when the method exits 
(normally or on exception).

try (InputStreamReader isr = new InputStreamReader(stream);
 BufferedReader rdr = new BufferedReader(isr);
 PrintStream prt = new PrintStream(fileName);)
{
prt.println("Command:"); 

src/java.base/share/classes/jdk/internal/misc/CDS.java line 307:

> 305: outputStdStream(proc.getErrorStream(), stdErrFile, 
> cmds);
> 306: }).start();
> 307: 

I think the above code can be refactored to avoid duplication. Something like:

String stdOutFile = drainOutput(proc.getInputStream(), "stdout");
String stdErrFile = drainOutput(proc.getErrorStream(), "stderr");

and move the common code into drainOutput().

-

PR: https://git.openjdk.java.net/jdk/pull/2737

Re: RFR: 8264150: CDS dumping code calls TRAPS functions in VM thread

[David Holmes]

On Tue, 30 Mar 2021 22:25:06 GMT, Coleen Phillimore  wrote:

> This change initializes the vtables/itables without checking constraints.  
> After initializing but before the class is visible in the SystemDictionary, 
> constraints are checked in a separate loop.
> 
> Tested with tier1-6 which includes jck tests.

Thanks for fixing this one Coleen! The refactoring was a bit more extensive 
than I had envisaged but the end result looks good.

Thanks,
David

-

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3277

Re: RFR: 8257831: Suspend with handshakes [v3]

[David Holmes]

On 1/04/2021 1:29 pm, Patricio Chilano Mateo wrote:

On Wed, 31 Mar 2021 06:32:40 GMT, David Holmes  wrote:


Robbin Ehn has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains three commits:

  - Merge branch 'master' into SuspendInHandshake
  - Merge branch 'master' into SuspendInHandshake
  - 8257831: Suspend with handshake (review baseline)


src/hotspot/share/runtime/thread.cpp line 667:


665:
666:   // We wait for the thread to transition to a more usable state.
667:   for (int i = 1; i <= SuspendRetryCount; i++) {


You will need to obsolete SuspendRetryCount and SuspendRetryDelay. This will 
need a CSR request indicating why there is no deprecation step.


I think we will need to do the same for flags AssertOnSuspendWaitFailure and 
TraceSuspendWaitFailures.


Well spotted - both of those flags are unused already - their use was 
deleted with JDK-8223312, but we overlooked dropping the flags themselves.


Cheers,
David




-

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8264285: Clean the modification of ccstr JVM flags [v2]

[Ioi Lam]

On Thu, 1 Apr 2021 00:25:53 GMT, Coleen Phillimore  wrote:

>> I had to use a `product` flag due to the following code, which should have 
>> been removed as part of 
>> [JDK-8243208](https://bugs.openjdk.java.net/browse/JDK-8243208), but I was 
>> afraid to do so because I didn't have a test case. I.e., all of our 
>> diagnostic/manageable/experimental flags were `product` flags.
>> 
>> With this PR, now I have a test case -- I changed 
>> `DummyManageableStringFlag` to a `notproduct` flag, and removed the 
>> following code. I am re-running tiers1-4 now.
>> 
>> void JVMFlag::check_all_flag_declarations() {
>>   for (JVMFlag* current = [0]; current->_name != NULL; current++) {
>> int flags = static_cast(current->_flags);
>> // Backwards compatibility. This will be relaxed/removed in JDK-7123237.
>> int mask = JVMFlag::KIND_DIAGNOSTIC | JVMFlag::KIND_MANAGEABLE | 
>> JVMFlag::KIND_EXPERIMENTAL;
>> if ((flags & mask) != 0) {
>>   assert((flags & mask) == JVMFlag::KIND_DIAGNOSTIC ||
>>  (flags & mask) == JVMFlag::KIND_MANAGEABLE ||
>>  (flags & mask) == JVMFlag::KIND_EXPERIMENTAL,
>>  "%s can be declared with at most one of "
>>  "DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL", current->_name);
>>   assert((flags & KIND_NOT_PRODUCT) == 0 &&
>>  (flags & KIND_DEVELOP) == 0,
>>  "%s has an optional DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL "
>>  "attribute; it must be declared as a product flag", 
>> current->_name);
>> }
>>   }
>> }
>
> What's the difference between notproduct and develop again?  Do we run tests 
> with the optimized build and why would this flag be available in that build?  
> ie. why not develop?

>From the top of globals.hpp: 

- `develop` flags are settable / visible only during development and are 
constant in the PRODUCT version
- `notproduct` flags are settable / visible only during development and are not 
declared in the PRODUCT version

Since this flag is only used in test cases, and specifically for modifying its 
value, it doesn't make sense to expose this flag to the PRODUCT version at all. 
So I chose `notproduct`, so we can save a few bytes for the PRODUCT as well.

-

PR: https://git.openjdk.java.net/jdk/pull/3254

Integrated: 8176026: SA: Huge heap sizes cause a negative value to be displayed in the jhisto heap total

[Koichi Sakata]

On Fri, 19 Mar 2021 11:49:39 GMT, Koichi Sakata  wrote:

> When a heap is used more than about 2.1GB, clhsdb jhisto shows a negative 
> number in the total field.
> 
> $ java -Xmx20g Sample
> 
> $ jhsdb clhsdb --pid 5773
> Attaching to process 5773, please wait...
> hsdb> jhisto
> ...
> 299:1   16  jdk.internal.misc.Unsafe
> 300:340210737610256 byte[]
> Total : 15823   -2146661280
> Heap traversal took 1.793 seconds.
> (Incidentally, the Sample is a program that only allocates many objects.)
> 
>  Details
> This is because in ObjectHistogram class the totalSize variable is int type.
> 
> The total size is the total of ObjectHistogramElement#getSize() and getSize() 
> returns long. So I changed int to long in the ObjectHistogram class.
> 
> Additionally, I changed the type of the totalCount. This doesn't cause a bug, 
> but ObjectHistogramElement#getCount() also returns long. So it doesn't need 
> to treat it as int, I think. ObjectHistogramElement#compare() also do the 
> same thing, so a class that is a huge size show the bottom of the list.
> 
>  Tests
> The jtreg test was successful.
> $ sudo make run-test TEST=serviceability/sa/ClhsdbJhisto.java
> 
> $ cat 
> build/linux-x86_64-server-fastdebug/test-results/jtreg_test_hotspot_jtreg_serviceability_sa_ClhsdbJhisto_java/text/summary.txt
> serviceability/sa/ClhsdbJhisto.java  Passed. Execution successful
> 
> I confirmed the output with the same program.
> 
> $ ./jdk/build/linux-x86_64-server-fastdebug/jdk/bin/java -Xmx20g Sample
> $ ./jdk/build/linux-x86_64-server-fastdebug/jdk/bin/jhsdb clhsdb --pid 10196
> Attaching to process 10196, please wait...
> hsdb> jhisto
> Object Histogram:
> 
> num   #instances#bytes  Class description
> --
> 1:  340513958838400 byte[]
> 2:  887 109032  java.lang.Class
> ...
> 300:1   16  jdk.internal.misc.Unsafe
> Total : 15827   13959470288
> Heap traversal took 1.72 seconds.

This pull request has now been integrated.

Changeset: 39f0b27a
Author:Koichi Sakata 
Committer: Yasumasa Suenaga 
URL:   https://git.openjdk.java.net/jdk/commit/39f0b27a
Stats: 5 lines in 2 files changed: 0 ins; 0 del; 5 mod

8176026: SA: Huge heap sizes cause a negative value to be displayed in the 
jhisto heap total

Reviewed-by: cjplummer, kevinw, ysuenaga

-

PR: https://git.openjdk.java.net/jdk/pull/3087

Re: RFR: 8176026: SA: Huge heap sizes cause a negative value to be displayed in the jhisto heap total [v3]

[Yasumasa Suenaga]

On Fri, 26 Mar 2021 13:51:40 GMT, Koichi Sakata  wrote:

>> When a heap is used more than about 2.1GB, clhsdb jhisto shows a negative 
>> number in the total field.
>> 
>> $ java -Xmx20g Sample
>> 
>> $ jhsdb clhsdb --pid 5773
>> Attaching to process 5773, please wait...
>> hsdb> jhisto
>> ...
>> 299:1   16  jdk.internal.misc.Unsafe
>> 300:340210737610256 byte[]
>> Total : 15823   -2146661280
>> Heap traversal took 1.793 seconds.
>> (Incidentally, the Sample is a program that only allocates many objects.)
>> 
>>  Details
>> This is because in ObjectHistogram class the totalSize variable is int type.
>> 
>> The total size is the total of ObjectHistogramElement#getSize() and 
>> getSize() returns long. So I changed int to long in the ObjectHistogram 
>> class.
>> 
>> Additionally, I changed the type of the totalCount. This doesn't cause a 
>> bug, but ObjectHistogramElement#getCount() also returns long. So it doesn't 
>> need to treat it as int, I think. ObjectHistogramElement#compare() also do 
>> the same thing, so a class that is a huge size show the bottom of the list.
>> 
>>  Tests
>> The jtreg test was successful.
>> $ sudo make run-test TEST=serviceability/sa/ClhsdbJhisto.java
>> 
>> $ cat 
>> build/linux-x86_64-server-fastdebug/test-results/jtreg_test_hotspot_jtreg_serviceability_sa_ClhsdbJhisto_java/text/summary.txt
>> serviceability/sa/ClhsdbJhisto.java  Passed. Execution successful
>> 
>> I confirmed the output with the same program.
>> 
>> $ ./jdk/build/linux-x86_64-server-fastdebug/jdk/bin/java -Xmx20g Sample
>> $ ./jdk/build/linux-x86_64-server-fastdebug/jdk/bin/jhsdb clhsdb --pid 10196
>> Attaching to process 10196, please wait...
>> hsdb> jhisto
>> Object Histogram:
>> 
>> num   #instances#bytes  Class description
>> --
>> 1:  340513958838400 byte[]
>> 2:  887 109032  java.lang.Class
>> ...
>> 300:1   16  jdk.internal.misc.Unsafe
>> Total : 15827   13959470288
>> Heap traversal took 1.72 seconds.
>
> Koichi Sakata has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update copyright

Looks good. I will sponsor you.

-

Marked as reviewed by ysuenaga (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3087

Re: RFR: 8257831: Suspend with handshakes [v3]

[Patricio Chilano Mateo]

On Wed, 31 Mar 2021 06:32:40 GMT, David Holmes  wrote:

>> Robbin Ehn has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains three commits:
>> 
>>  - Merge branch 'master' into SuspendInHandshake
>>  - Merge branch 'master' into SuspendInHandshake
>>  - 8257831: Suspend with handshake (review baseline)
>
> src/hotspot/share/runtime/thread.cpp line 667:
> 
>> 665: 
>> 666:   // We wait for the thread to transition to a more usable state.
>> 667:   for (int i = 1; i <= SuspendRetryCount; i++) {
> 
> You will need to obsolete SuspendRetryCount and SuspendRetryDelay. This will 
> need a CSR request indicating why there is no deprecation step.

I think we will need to do the same for flags AssertOnSuspendWaitFailure and 
TraceSuspendWaitFailures.

-

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8257831: Suspend with handshakes [v3]

[Patricio Chilano Mateo]

On Wed, 31 Mar 2021 08:27:35 GMT, Robbin Ehn  wrote:

>> A suspend request is done by handshaking thread target thread(s). When 
>> executing the handshake operation we know the target mutator thread is in a 
>> dormant state (as in safepoint safe state). We have a guarantee that it will 
>> check it's poll before leaving the dormant state. To stop the thread from 
>> leaving the the dormant state we install a second asynchronous handshake to 
>> be executed by the targeted thread. The asynchronous handshake will wait on 
>> a monitor while the thread is suspended. The target thread cannot not leave 
>> the dormant state without a resume request.
>> 
>> Per thread suspend requests are naturally serialized by the per thread 
>> HandshakeState lock (we can only execute one handshake at a time per thread).
>> Instead of having a separate lock we use this to our advantage and use 
>> HandshakeState lock for serializing access to the suspend flag and for 
>> wait/notify. 
>> 
>> Suspend:
>> Requesting thread -> synchronous handshake -> target thread
>> Inside synchronus handshake (HandshakeState lock is locked while
>> executing any handshake):
>>  - Set suspended flag
>>  - Install asynchronous handshake
>> 
>> Target thread -> tries to leave dormant state -> Executes handshakes
>> Target only executes asynchronous handshake:
>>  - While suspended
>>  - Go to blocked
>>  - Wait on HandshakeState lock
>> 
>> Resume:
>> Resuming thread:
>>  - Lock HandshakeState lock
>>  - Clear suspended flag
>>  - Notify HandshakeState lock
>>  - Unlock HandshakeState lock
>> 
>> The "suspend requested" flag is an optimization, without it a dormant thread 
>> could be suspended and resumed many times and each would add a new 
>> asynchronous handshake. Suspend requested flag means there is already an 
>> asynchronous suspend handshake in queue which can be re-used, only the 
>> suspend flag needs to be set.
>> 
>> 
>> Some code can be simplified or done in a smarter way but I refrained from 
>> doing such changes instead tried to keep existing code as is as far as 
>> possible. This concerns especially raw monitors.
>> 
>> 
>> Regarding the changed test, the documentation says:
>> "If the calling thread is specified in the request_list array, this function 
>> will not return until some other thread resumes it."
>> 
>> But the code:
>>   LOG("suspendTestedThreads: before JVMTI SuspendThreadList");
>>   err = jvmti->SuspendThreadList(threads_count, threads, results);
>>   ...
>>   // Allow the Main thread to inspect the result of tested threads 
>> suspension
>>   agent_unlock(jni);
>>   
>> The thread will never return from SuspendThreadList until resumed, so it 
>> cannot unlock with agent_unlock().
>> Thus main thread is stuck forever on:
>>   // Block until the suspender thread competes the tested threads suspension 
>>   agent_lock(jni);
>> 
>> And never checks and resumes the threads. So I removed that lock instead 
>> just sleep and check until all thread have the expected suspended state.
>> 
>> 
>> 
>> This version already contains updates after pre-review comments from 
>> @dcubed-ojdk, @pchilano, @coleenp.
>> (Pre-review comments here:
>> https://github.com/openjdk/jdk/pull/2625)
>> 
>>  
>> Testing t1-t8, nsk_jdi/nsk_jvmti/jdk_jdi/tck, KS24, RunThese and
>> combinations like running with -XX:ZCollectionInterval=0.01 -
>> XX:ZFragmentationLimit=0.
>> Running above some of above concurrently (load ~240), slow debug,
>> etc...
>
> Robbin Ehn has updated the pull request with a new target base due to a merge 
> or a rebase. The pull request now contains three commits:
> 
>  - Merge branch 'master' into SuspendInHandshake
>  - Merge branch 'master' into SuspendInHandshake
>  - 8257831: Suspend with handshake (review baseline)

src/hotspot/share/runtime/handshake.hpp line 151:

> 149: 
> 150:   bool is_suspended() { return 
> Atomic::load(&_suspended); }
> 151:   void set_suspend(bool to)   { return 
> Atomic::store(&_suspended, to); }

Maybe set_suspended() instead()?

src/hotspot/share/runtime/objectMonitor.cpp line 436:

> 434: current->set_current_pending_monitor(NULL);
> 435: 
> 436: // We cleared the pending monitor info since we've just gotten past

Comment below needs updating since it mentions ThreadBlockInVM but we are not 
using it anymore.

src/hotspot/share/runtime/thread.cpp line 277:

> 275: #endif
> 276: 
> 277:   _SR_lock = new Monitor(Mutex::suspend_resume, "SR_lock", true,

Mutex::suspend_resume rank is not used by any other monitor so we could remove 
it.

src/hotspot/share/runtime/thread.hpp line 1142:

> 1140:   bool java_resume();  // higher-level resume logic called by the 
> public APIs
> 1141:   bool is_suspended() { return 
> _handshake.is_suspended(); }
> 1142:   bool suspend_request_pending()  { return 
> _handshake.suspend_request_pending(); }

If you use is_suspended() to check 

Re: RFR: 8257831: Suspend with handshakes [v2]

[Patricio Chilano Mateo]

On Tue, 30 Mar 2021 13:26:13 GMT, Richard Reingruber  wrote:

>> Robbin Ehn has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains two commits:
>> 
>>  - Merge branch 'master' into SuspendInHandshake
>>  - 8257831: Suspend with handshake (review baseline)
>
> src/hotspot/share/runtime/handshake.hpp line 142:
> 
>> 140:  private:
>> 141:   volatile bool _suspended;
>> 142:   volatile bool _suspend_requested;
> 
> According to the PR description `_suspend_requested` is an optimization.
> 
>> The "suspend requested" flag is an optimization, without it a dormant thread
>> could be suspended and resumed many times and each would add a new
>> asynchronous handshake. Suspend requested flag means there is already an
>> asynchronous suspend handshake in queue which can be re-used, only the 
>> suspend
>> flag needs to be set.
> 
> I think there are a few places where _suspend_requested is queried by mistake 
> instead of _suspended. Maybe it would help to prevent this if 
> _suspend_requested was renamed to something that better describes its 
> purpose, e.g. _has_async_suspend_handshake or similar.

I also think it would be a good idea to rename it with something along those 
lines.

-

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8257831: Suspend with handshakes [v3]

[Patricio Chilano Mateo]

On Wed, 31 Mar 2021 08:27:35 GMT, Robbin Ehn  wrote:

>> A suspend request is done by handshaking thread target thread(s). When 
>> executing the handshake operation we know the target mutator thread is in a 
>> dormant state (as in safepoint safe state). We have a guarantee that it will 
>> check it's poll before leaving the dormant state. To stop the thread from 
>> leaving the the dormant state we install a second asynchronous handshake to 
>> be executed by the targeted thread. The asynchronous handshake will wait on 
>> a monitor while the thread is suspended. The target thread cannot not leave 
>> the dormant state without a resume request.
>> 
>> Per thread suspend requests are naturally serialized by the per thread 
>> HandshakeState lock (we can only execute one handshake at a time per thread).
>> Instead of having a separate lock we use this to our advantage and use 
>> HandshakeState lock for serializing access to the suspend flag and for 
>> wait/notify. 
>> 
>> Suspend:
>> Requesting thread -> synchronous handshake -> target thread
>> Inside synchronus handshake (HandshakeState lock is locked while
>> executing any handshake):
>>  - Set suspended flag
>>  - Install asynchronous handshake
>> 
>> Target thread -> tries to leave dormant state -> Executes handshakes
>> Target only executes asynchronous handshake:
>>  - While suspended
>>  - Go to blocked
>>  - Wait on HandshakeState lock
>> 
>> Resume:
>> Resuming thread:
>>  - Lock HandshakeState lock
>>  - Clear suspended flag
>>  - Notify HandshakeState lock
>>  - Unlock HandshakeState lock
>> 
>> The "suspend requested" flag is an optimization, without it a dormant thread 
>> could be suspended and resumed many times and each would add a new 
>> asynchronous handshake. Suspend requested flag means there is already an 
>> asynchronous suspend handshake in queue which can be re-used, only the 
>> suspend flag needs to be set.
>> 
>> 
>> Some code can be simplified or done in a smarter way but I refrained from 
>> doing such changes instead tried to keep existing code as is as far as 
>> possible. This concerns especially raw monitors.
>> 
>> 
>> Regarding the changed test, the documentation says:
>> "If the calling thread is specified in the request_list array, this function 
>> will not return until some other thread resumes it."
>> 
>> But the code:
>>   LOG("suspendTestedThreads: before JVMTI SuspendThreadList");
>>   err = jvmti->SuspendThreadList(threads_count, threads, results);
>>   ...
>>   // Allow the Main thread to inspect the result of tested threads 
>> suspension
>>   agent_unlock(jni);
>>   
>> The thread will never return from SuspendThreadList until resumed, so it 
>> cannot unlock with agent_unlock().
>> Thus main thread is stuck forever on:
>>   // Block until the suspender thread competes the tested threads suspension 
>>   agent_lock(jni);
>> 
>> And never checks and resumes the threads. So I removed that lock instead 
>> just sleep and check until all thread have the expected suspended state.
>> 
>> 
>> 
>> This version already contains updates after pre-review comments from 
>> @dcubed-ojdk, @pchilano, @coleenp.
>> (Pre-review comments here:
>> https://github.com/openjdk/jdk/pull/2625)
>> 
>>  
>> Testing t1-t8, nsk_jdi/nsk_jvmti/jdk_jdi/tck, KS24, RunThese and
>> combinations like running with -XX:ZCollectionInterval=0.01 -
>> XX:ZFragmentationLimit=0.
>> Running above some of above concurrently (load ~240), slow debug,
>> etc...
>
> Robbin Ehn has updated the pull request with a new target base due to a merge 
> or a rebase. The pull request now contains three commits:
> 
>  - Merge branch 'master' into SuspendInHandshake
>  - Merge branch 'master' into SuspendInHandshake
>  - 8257831: Suspend with handshake (review baseline)

Hi Robbin,

Great work! I only have minor comments.

-

Marked as reviewed by pchilanomate (Committer).

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8176026: SA: Huge heap sizes cause a negative value to be displayed in the jhisto heap total [v2]

[Koichi Sakata]

On Fri, 26 Mar 2021 13:57:00 GMT, Koichi Sakata  wrote:

>> Marked as reviewed by mgkw...@github.com (no known OpenJDK username).
>
> I just updated the copyright.

Could someone sponsor this pull request? It is ready.

-

PR: https://git.openjdk.java.net/jdk/pull/3087

RFR: 8264150: CDS dumping code calls TRAPS functions in VM thread

[Coleen Phillimore]

This change initializes the vtables/itables without checking constraints.  
After initializing but before the class is visible in the SystemDictionary, 
constraints are checked in a separate loop.

Tested with tier1-6 which includes jck tests.

-

Commit messages:
 - 8264150: CDS dumping code calls TRAPS functions in VM thread

Changes: https://git.openjdk.java.net/jdk/pull/3277/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3277=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264150
  Stats: 290 lines in 10 files changed: 146 ins; 84 del; 60 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3277.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3277/head:pull/3277

PR: https://git.openjdk.java.net/jdk/pull/3277

Re: RFR: 8264285: Clean the modification of ccstr JVM flags [v2]

[Coleen Phillimore]

On Wed, 31 Mar 2021 19:02:02 GMT, Ioi Lam  wrote:

>> src/hotspot/share/runtime/flags/debug_globals.hpp line 38:
>> 
>>> 36: // have any MANAGEABLE flags of the ccstr type, but we really need to
>>> 37: // make sure the implementation is correct (in terms of memory 
>>> allocation)
>>> 38: // just in case someone may add such a flag in the future.
>> 
>> Could you have just added a develop flag to the manageable flags instead?
>
> I had to use a `product` flag due to the following code, which should have 
> been removed as part of 
> [JDK-8243208](https://bugs.openjdk.java.net/browse/JDK-8243208), but I was 
> afraid to do so because I didn't have a test case. I.e., all of our 
> diagnostic/manageable/experimental flags were `product` flags.
> 
> With this PR, now I have a test case -- I changed `DummyManageableStringFlag` 
> to a `notproduct` flag, and removed the following code. I am re-running 
> tiers1-4 now.
> 
> void JVMFlag::check_all_flag_declarations() {
>   for (JVMFlag* current = [0]; current->_name != NULL; current++) {
> int flags = static_cast(current->_flags);
> // Backwards compatibility. This will be relaxed/removed in JDK-7123237.
> int mask = JVMFlag::KIND_DIAGNOSTIC | JVMFlag::KIND_MANAGEABLE | 
> JVMFlag::KIND_EXPERIMENTAL;
> if ((flags & mask) != 0) {
>   assert((flags & mask) == JVMFlag::KIND_DIAGNOSTIC ||
>  (flags & mask) == JVMFlag::KIND_MANAGEABLE ||
>  (flags & mask) == JVMFlag::KIND_EXPERIMENTAL,
>  "%s can be declared with at most one of "
>  "DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL", current->_name);
>   assert((flags & KIND_NOT_PRODUCT) == 0 &&
>  (flags & KIND_DEVELOP) == 0,
>  "%s has an optional DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL "
>  "attribute; it must be declared as a product flag", 
> current->_name);
> }
>   }
> }

What's the difference between notproduct and develop again?  Do we run tests 
with the optimized build and why would this flag be available in that build?  
ie. why not develop?

-

PR: https://git.openjdk.java.net/jdk/pull/3254

Re: RFR: 8264285: Clean the modification of ccstr JVM flags [v2]

[Coleen Phillimore]

On Wed, 31 Mar 2021 19:01:48 GMT, Ioi Lam  wrote:

>> There are two versions of JVMFlagAccess::ccstrAtPut() for modifying JVM 
>> flags of the ccstr type (i.e., strings).
>> 
>> - One version requires the caller to free the old value, but some callers 
>> don't do that (writeableFlags.cpp).
>> - The other version frees the old value on behalf of the caller. However, 
>> this version is accessible only via FLAG_SET_XXX macros and is currently 
>> unused. So it's unclear whether it actually works.
>> 
>> We should combine these two versions into a single function, fix problems in 
>> the callers, and add test cases. The old value should be freed 
>> automatically, because typically the caller isn't interested in the old 
>> value.
>> 
>> Note that the FLAG_SET_XXX macros do not return the old value. Requiring the 
>> caller of FLAG_SET_XXX to free the old value would be tedious and error 
>> prone.
>
> Ioi Lam has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   relax flag attributions (ala JDK-7123237)

Marked as reviewed by coleenp (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/3254

Re: RFR: 8264484: Replace uses of StringBuffer with StringBuilder in jdk.hotspot.agent [v3]

[Yasumasa Suenaga]

On Wed, 31 Mar 2021 17:45:33 GMT, Andrey Turbanov 
 wrote:

>> Found by IntelliJ IDEA inspection `Java | Java language level migration aids 
>> | Java 5 | 'StringBuffer' may be 'StringBuilder'`
>> As suggested in 
>> https://github.com/openjdk/jdk/pull/1507#issuecomment-757369003 I've created 
>> separate PR for module `jdk.hotspot.agent`
>> Similar cleanups in the past: 
>> https://bugs.openjdk.java.net/browse/JDK-8041679 
>> https://bugs.openjdk.java.net/browse/JDK-8264029
>
> Andrey Turbanov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8264484: Replace uses of StringBuffer with StringBuilder in 
> jdk.hotspot.agent
>   
>   fix indentation

Looks good!
BTW have you found sponsor? I can sponsor you if you need.

-

Marked as reviewed by ysuenaga (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3275

Re: RFR: 8259070: Add jcmd option to dump CDS [v8]

[Ioi Lam]

On Wed, 31 Mar 2021 20:58:46 GMT, Yumin Qi  wrote:

>> Hi, Please review
>> 
>>   Added jcmd option for dumping CDS archive during application runtime. 
>> Before this change, user has to dump shared archive in two steps: first run 
>> application with
>>   `java -XX:DumpLoadedClassList=  `  
>>  to collect shareable class names and saved in file `` , then 
>>   `java -Xshare:dump -XX:SharedClassListFile= 
>> -XX:SharedArchiveFile= ...`
>>   With this change, user can use jcmd to dump CDS without going through 
>> above steps. Also user can choose a moment during the app runtime  to dump 
>> an archive.
>>The bug is associated with the CSR: 
>> https://bugs.openjdk.java.net/browse/JDK-8259798 which has been approved.
>>New added jcmd option:
>>`jcmd  VM.cds static_dump `
>>or
>> `jcmd  VM.cds dynamic_dump `
>>   To dump dynamic archive, requires start app with newly added flag 
>> `-XX:+RecordDynamicDumpInfo`, with this flag, some information related to 
>> dynamic dump like loader constraints will be recorded. Note the dumping 
>> process changed some object memory locations so for dumping dynamic archive, 
>> can only done once for a running app. For static dump, user can dump 
>> multiple times against same process. 
>>The file name is optional, if the file name is not supplied, the file 
>> name will take format of `java_pid_static.jsa` or 
>> `java_pid_dynamic.jsa` for static and dynamic respectively. The 
>> `` is the application process ID.
>> 
>>   Tests: tier1,tier2,tier3,tier4
>> 
>> Thanks
>> Yumin
>
> Yumin Qi has updated the pull request with a new target base due to a merge 
> or a rebase. The pull request now contains 10 commits:
> 
>  - Fix revert unintentionally comment, merge master.
>  - Merge branch 'master' of ssh://github.com/yminqi/jdk into jdk-8259070
>  - Remove CDS.getVMArguments, changed to use VM.getRuntimeVMArguments. 
> Removed unused function from ClassLoader. Improved 
> InstanceKlass::is_shareable() and related test. Added more test scenarios.
>  - Remove redundant check for if a class is shareable
>  - Fix according to review comment and add more tests
>  - Fix filter more flags to exclude in static dump, add more test cases
>  - Merge branch 'master' into jdk-8259070
>  - Fix white space in CDS.java
>  - Add function CDS.dumpSharedArchive in java to dump shared archive
>  - 8259070: Add jcmd option to dump CDS

Changes requested by iklam (Reviewer).

src/hotspot/share/classfile/vmSymbols.hpp line 304:

> 302:   template(generateLambdaFormHolderClasses_signature, 
> "([Ljava/lang/String;)[Ljava/lang/Object;") \
> 303:   template(dumpSharedArchive, "dumpSharedArchive")   
>  \
> 304:   template(dumpSharedArchive_signature, "(ZLjava/lang/String;)V")
>  \

Need to align the "dumpSharedArchive" part with the previous line.

src/hotspot/share/prims/jvm.cpp line 3745:

> 3743: #if INCLUDE_CDS
> 3744:   assert(UseSharedSpaces && RecordDynamicDumpInfo, "already checked in 
> arguments.cpp?");
> 3745:   if (DynamicArchive::has_been_dumped_once()) {

Maybe add a comment like this:? 
// During dynamic archive dumping, some of the data structures are overwritten 
so
// we cannot dump the dynamic archive again. TODO: this should be fixed.

src/hotspot/share/prims/jvm.cpp line 3754:

> 3752:   assert(ArchiveClassesAtExit == nullptr, "already checked in 
> arguments.cpp?");
> 3753:   Handle file_handle(THREAD, JNIHandles::resolve_non_null(archiveName));
> 3754:   char* archive_name  = java_lang_String::as_utf8_string(file_handle());

A ResourceMark is needed before calling java_lang_String::as_utf8_string().

In general, I think the code in jvm.cpp should only marshall the jobject 
argument (e.g., convert `jstring` to `char*`.). The main functionality of 
JVM_DumpDynamicArchive should be moved to dynamicArchive.cpp. Similarly, most 
of the work in JVM_DumpClassListToFile should be moved to metaspaceShared.cpp.

src/hotspot/share/prims/jvm.cpp line 3759:

> 3757: DynamicArchive::dump();
> 3758:   } else {
> 3759: THROW_MSG(vmSymbols::java_lang_RuntimeException(),

Need to set ArchiveClassesAtExit to NULL before throwing the exception, since 
dynamic dump may not work anymore after the failure.

test/hotspot/jtreg/runtime/cds/appcds/jcmd/LingeredTestApp.java line 28:

> 26: public class LingeredTestApp extends LingeredApp {
> 27: // Do not use default test.class.path in class path.
> 28: public boolean useDefaultClasspath() { return false; }

It's not obvious that you're changing the behavior of the base class by 
overriding a member function. It's better to have 

public LingeredTestApp() {
   setUseDefaultClasspath(false);
}

Also, the name of LingeredTestApp is kind of generic. How about renaming it to 
JCmdTestLingeredApp?

-

PR: https://git.openjdk.java.net/jdk/pull/2737

Re: RFR: 8257831: Suspend with handshakes [v2]

[David Holmes]

On 1/04/2021 12:35 am, Robbin Ehn wrote:

On Wed, 31 Mar 2021 10:48:11 GMT, Robbin Ehn  wrote:


Thanks @dcubed-ojdk - no it won't. The problem is that the signal can hit very 
late in a thread's termination process, after the JavaThread destructor has 
executed, so no query of JavaThread state is possible. So we needed something 
in the Thread state and the SR_lock was a good enough proxy for that. It may be 
possible to use a different Thread field (perhaps _ParkEvent but encapsulated 
in a Thread::has_terminated() helper method).


SR_handler is used for OS-level suspend/resume (not to conflict with this 
change-set).
This feature is only used by JFR (AFAIK), and JFR only samples threads on it's 
ThreadsList.
This means the JavaThread can never be terminated, hence this code will always 
pass.

If someone else is using OS-level suspend/resume without a ThreadsList, the bug 
is there is no ThreadsList AFAICT.

Since ThreadLocalStorage::thread() is cleared last in ~Thread with 
Thread::clear_thread_current(); may be in the ~Thread destructor.
So the argument is that would be safe to read stuff from Thread but not 
JavaThread?
Since the compiler (and CPU) may reorder and optimize away code, so I argue 
reading from a half destroyed object is not a great idea.
E.g. Monitor* _SR_lock; is not a volatile pointer; since reading from this 
memory is UB after destruction, compiler is free to remove or not publish the 
store to NULL.

So I suggest either to remove this check, since the only user is using a 
ThreadsList and any other should also be using that too.
Or call Thread::clear_thread_current() before the JavaThread destructor is 
called, that way we can be certain that there is no UB.


I got some offline input from David, there seem to be an issue with signal 
being delivered after the ThreadsListHandle destructor. If that is the case a 
ThreadsList doesn't help here.

So I suggested we keep this out of this change-set and just take another 
suitable field to mirror what we have today.

`ParkEvent * _ParkEvent;` ?


Yes nicely packaged as:

bool Thread::has_terminated() {
  return _ParkEvent == NULL;
}

Also note:

 // It's possible we can encounter a null _ParkEvent, etc., in 
stillborn threads.

  // We NULL out the fields for good hygiene.
  ParkEvent::Release(_ParkEvent); _ParkEvent   = NULL;

the comment is wrong as it can't be NULL even if stillborn. And now we 
NULL it out for good hygiene and as a late-stage termination indicator.


And yes we can make _ParkEvent volatile to dissuade the compiler from 
eliding the NULLing out.


Thanks,
David



-

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8257831: Suspend with handshakes [v2]

[David Holmes]

Hi Dan,

On 1/04/2021 12:07 am, Daniel D.Daugherty wrote:

On Wed, 31 Mar 2021 10:22:28 GMT, Richard Reingruber  wrote:


My comment was about JVMTI_ERROR_THREAD_NOT_ALIVE


Sure. I agree with your comment.


It depends on how late we can call into an event handler during the
JavaThread exit code path. Is it possible to post an event on a thread
that has passed the `terminated` point in JavaThread::exit()?


The JVM TI thread_end event is posted before the thread is marked as 
exiting.


David



PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8264285: Clean the modification of ccstr JVM flags [v2]

[Ioi Lam]

On Tue, 30 Mar 2021 03:44:26 GMT, David Holmes  wrote:

>> Ioi Lam has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   relax flag attributions (ala JDK-7123237)
>
> src/hotspot/share/services/writeableFlags.cpp line 250:
> 
>> 248:   if (err == JVMFlag::SUCCESS) {
>> 249: assert(value == NULL, "old value is freed automatically and not 
>> returned");
>> 250:   }
> 
> The whole block should be ifdef DEBUG.

Since this whole block can be optimized out by the C compiler in product 
builds, I'd rather leave out the `#ifdef` to avoid clutter.

-

PR: https://git.openjdk.java.net/jdk/pull/3254

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v6]

[Mandy Chung]

On Wed, 31 Mar 2021 21:02:51 GMT, Daniel Fuchs  wrote:

>> This RFE proposes to extend the MXBean framework to define a mapping to 
>> records.
>> 
>> The MXBean framework already defines a mapping of `CompositeType` to plain 
>> java objects. Records are a natural representation of CompositeTypes. A 
>> record can be easily reconstructed from a `CompositeData` through the record 
>> canonical constructor. A clear advantage of records over plain java objects 
>> is that the canonical constructor will not need to be annotated in order to 
>> map composite data property names to constructor parameter names.
>> 
>> With this RFE, here is an example comparing coding a composite type 
>> `NamedNumber` that consists of an `int` and a `String`, using records and 
>> using a plain java class. In both case, the `CompositeType` looks like this:
>> 
>> CompositeType(
>> "NamedNumber",  // typeName
>> "NamedNumber",  // description
>> new String[] {"number", "name"},// itemNames
>> new String[] {"number", "name"},// itemDescriptions
>> new OpenType[] {SimpleType.INTEGER,
>> SimpleType.STRING}  // itemTypes
>> );
>> 
>> The plain Java class needs a public constructor annotated with 
>> `@ConstructorParameters` annotation:
>> 
>> public class NamedNumber {
>> public int getNumber() {return number;}
>> public String getName() {return name;}
>> @ConstructorParameters({"number", "name"})
>> public NamedNumber(int number, String name) {
>> this.number = number;
>> this.name = name;
>> }
>> private final int number;
>> private final String name;
>> }
>> 
>> And the equivalent with a record class: 
>> 
>> public record NamedNumber(int number, String name) {}
>
> Daniel Fuchs has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - minor style issue
>  - minor style issue

Thanks for making the change.  The spec change looks good to me.

-

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8264538: Rename SystemDictionary::parse_stream [v2]

[Coleen Phillimore]

On Wed, 31 Mar 2021 20:22:08 GMT, Harold Seigel  wrote:

>> Coleen Phillimore has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   fifix comment
>
> src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 1395:
> 
>> 1393:   cl_info,
>> 1394:   THREAD);
>> 1395: 
> 
> Could you add a comment above line 1390 saying you can't call 
> resolve_class_from_stream() here because the resulting class should not go in 
> the system dictionary?

// Parse and create a class from the bytes, but this class isn't added
// to the dictionary, so do not call resolve_from_stream.

> src/hotspot/share/prims/jvmtiRedefineClasses.hpp line 305:
> 
>> 303: // - How do we serialize the RedefineClasses() API without deadlocking?
>> 304: //
>> 305: // - KlassFactory::create_from_stream() was called with a NULL 
>> protection
> 
> Maybe delete the comment that goes from lines 305 - 309 ?

Good idea.  The comment is really old and no longer relevant.

-

PR: https://git.openjdk.java.net/jdk/pull/3289

Re: RFR: 8264538: Rename SystemDictionary::parse_stream [v2]

[Coleen Phillimore]

On Wed, 31 Mar 2021 20:07:50 GMT, Lois Foltan  wrote:

>> Coleen Phillimore has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   fifix comment
>
> src/hotspot/share/prims/jvm.cpp line 950:
> 
>> 948:   InstanceKlass* ik = NULL;
>> 949:   if (!is_hidden) {
>> 950: ClassLoadInfo cl_info(protection_domain);
> 
> Minor comment, you could pull the creation of ClassLoadInfo out of this if 
> statement since both the the if and the else sections create a ClassLoadInfo 
> with pretty much the same information.

That other ClassLoadInfo cl_info(protection_domain) you see is from another 
function, so I can't pull it out.
The other side of the 'if' statement creates a ClassLoadInfo with all the 
hidden class goodies.

-

PR: https://git.openjdk.java.net/jdk/pull/3289

Re: RFR: 8264538: Rename SystemDictionary::parse_stream [v3]

[Coleen Phillimore]

> This function is used to call the classfile parser for hidden or anonymous 
> classes, and for use with jvmti RedefineClasses. The latter only calls 
> KlassFactory::create_from_stream and skips the rest of the code in 
> SystemDictionary::parse_stream.
> 
> Renamed SystemDictionary::parse_stream -> resolve_hidden_class_from_stream
> resolve_from_stream -> resolve_class_from_stream
> and have SystemDictionary::resolve_from_stream() call the right version 
> depending on ClassLoadInfo flags.  Callers of resolve_from_stream now pass 
> protection domain via. ClassLoadInfo.
> 
> So the external API is resolve_from_stream.
> 
> Tested with tier1 on 4 Oracle supported platforms.

Coleen Phillimore has updated the pull request incrementally with one 
additional commit since the last revision:

  Add and remove comments.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3289/files
  - new: https://git.openjdk.java.net/jdk/pull/3289/files/8dfcb093..cd49552a

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3289=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3289=01-02

  Stats: 9 lines in 2 files changed: 2 ins; 7 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3289.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3289/head:pull/3289

PR: https://git.openjdk.java.net/jdk/pull/3289

Re: RFR: 8264538: Rename SystemDictionary::parse_stream [v2]

[Coleen Phillimore]

On Wed, 31 Mar 2021 19:57:57 GMT, Coleen Phillimore  wrote:

>> This function is used to call the classfile parser for hidden or anonymous 
>> classes, and for use with jvmti RedefineClasses. The latter only calls 
>> KlassFactory::create_from_stream and skips the rest of the code in 
>> SystemDictionary::parse_stream.
>> 
>> Renamed SystemDictionary::parse_stream -> resolve_hidden_class_from_stream
>> resolve_from_stream -> resolve_class_from_stream
>> and have SystemDictionary::resolve_from_stream() call the right version 
>> depending on ClassLoadInfo flags.  Callers of resolve_from_stream now pass 
>> protection domain via. ClassLoadInfo.
>> 
>> So the external API is resolve_from_stream.
>> 
>> Tested with tier1 on 4 Oracle supported platforms.
>
> Coleen Phillimore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   fifix comment

Thank you for reviewing this Lois and Harold.  Some replies attached.

-

PR: https://git.openjdk.java.net/jdk/pull/3289

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v6]

[Daniel Fuchs]

> This RFE proposes to extend the MXBean framework to define a mapping to 
> records.
> 
> The MXBean framework already defines a mapping of `CompositeType` to plain 
> java objects. Records are a natural representation of CompositeTypes. A 
> record can be easily reconstructed from a `CompositeData` through the record 
> canonical constructor. A clear advantage of records over plain java objects 
> is that the canonical constructor will not need to be annotated in order to 
> map composite data property names to constructor parameter names.
> 
> With this RFE, here is an example comparing coding a composite type 
> `NamedNumber` that consists of an `int` and a `String`, using records and 
> using a plain java class. In both case, the `CompositeType` looks like this:
> 
> CompositeType(
> "NamedNumber",  // typeName
> "NamedNumber",  // description
> new String[] {"number", "name"},// itemNames
> new String[] {"number", "name"},// itemDescriptions
> new OpenType[] {SimpleType.INTEGER,
> SimpleType.STRING}  // itemTypes
> );
> 
> The plain Java class needs a public constructor annotated with 
> `@ConstructorParameters` annotation:
> 
> public class NamedNumber {
> public int getNumber() {return number;}
> public String getName() {return name;}
> @ConstructorParameters({"number", "name"})
> public NamedNumber(int number, String name) {
> this.number = number;
> this.name = name;
> }
> private final int number;
> private final String name;
> }
> 
> And the equivalent with a record class: 
> 
> public record NamedNumber(int number, String name) {}

Daniel Fuchs has updated the pull request incrementally with two additional 
commits since the last revision:

 - minor style issue
 - minor style issue

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3201/files
  - new: https://git.openjdk.java.net/jdk/pull/3201/files/9227ba58..a624a763

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3201=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3201=04-05

  Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3201.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3201/head:pull/3201

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8259070: Add jcmd option to dump CDS [v8]

[Yumin Qi]

> Hi, Please review
> 
>   Added jcmd option for dumping CDS archive during application runtime. 
> Before this change, user has to dump shared archive in two steps: first run 
> application with
>   `java -XX:DumpLoadedClassList=  `  
>  to collect shareable class names and saved in file `` , then 
>   `java -Xshare:dump -XX:SharedClassListFile= 
> -XX:SharedArchiveFile= ...`
>   With this change, user can use jcmd to dump CDS without going through above 
> steps. Also user can choose a moment during the app runtime  to dump an 
> archive.
>The bug is associated with the CSR: 
> https://bugs.openjdk.java.net/browse/JDK-8259798 which has been approved.
>New added jcmd option:
>`jcmd  VM.cds static_dump `
>or
> `jcmd  VM.cds dynamic_dump `
>   To dump dynamic archive, requires start app with newly added flag 
> `-XX:+RecordDynamicDumpInfo`, with this flag, some information related to 
> dynamic dump like loader constraints will be recorded. Note the dumping 
> process changed some object memory locations so for dumping dynamic archive, 
> can only done once for a running app. For static dump, user can dump multiple 
> times against same process. 
>The file name is optional, if the file name is not supplied, the file name 
> will take format of `java_pid_static.jsa` or 
> `java_pid_dynamic.jsa` for static and dynamic respectively. The 
> `` is the application process ID.
> 
>   Tests: tier1,tier2,tier3,tier4
> 
> Thanks
> Yumin

Yumin Qi has updated the pull request with a new target base due to a merge or 
a rebase. The pull request now contains 10 commits:

 - Fix revert unintentionally comment, merge master.
 - Merge branch 'master' of ssh://github.com/yminqi/jdk into jdk-8259070
 - Remove CDS.getVMArguments, changed to use VM.getRuntimeVMArguments. Removed 
unused function from ClassLoader. Improved InstanceKlass::is_shareable() and 
related test. Added more test scenarios.
 - Remove redundant check for if a class is shareable
 - Fix according to review comment and add more tests
 - Fix filter more flags to exclude in static dump, add more test cases
 - Merge branch 'master' into jdk-8259070
 - Fix white space in CDS.java
 - Add function CDS.dumpSharedArchive in java to dump shared archive
 - 8259070: Add jcmd option to dump CDS

-

Changes: https://git.openjdk.java.net/jdk/pull/2737/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2737=07
  Stats: 830 lines in 21 files changed: 758 ins; 58 del; 14 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2737.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2737/head:pull/2737

PR: https://git.openjdk.java.net/jdk/pull/2737

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v2]

[Daniel Fuchs]

On Wed, 31 Mar 2021 18:32:31 GMT, Mandy Chung  wrote:

>> Hi Mandy, I have updated the PR with a revised version of the text above. Is 
>> that more in line what you had in mind?
>> 
>> best regards,
>> -- daniel
>
> This looks better.  I like a separate mapping section for records since the 
> mapping rules are straight-forward.   The complexity comes if we want to 
> support a record to implement `CompositeDataView` that allows a type to 
> support more flexible conversion to `CompositeData` which is why you add 
> records in the "Mappings for other types".   I am wondering if we really want 
> to support records to implement `CompositeDataView` but we may want to avoid 
> such a special case.
> 
> I suggest add subsections in "Mappings for other types":   There are two 
> mapping rules: (1) _opentype(J)_ maps `J` to `CompositeType` (2) 
> _opendata(J)_ maps `J` to `CompositeData`.   _opentype(J)_ for record class 
> does not need to refer to other types.   The common cases for _opendata(J)_ 
> for record class is using the canonical constructors.   The other less common 
> cases like using non-canonical constructors and `CompositeDataView` can state 
> that it follows the same rules as specified for other types.   "Mappings for 
> other types" does not need any change for records.   
> 
> "Reconstructing an instance of Java type J from a CompositeData" section 
> should be renamed to "Reconstructing an instance of Java type or record class 
> J from a CompositeData"
> 
> Here is a minor tweaking on "Mappings for Records"
> 
> A record is convertible to a CompositeType if and only if all its components 
> are 
> convertible to open types. Otherwise, it is not convertible.
> 
> A record class is converted to a CompositeType with one item for every record 
> component as follows.
> - The type name of this CompositeType is determined by the type name rules 
> detailed below. 
> - Each record component of type T, the item in the CompositeType has the same 
> name 
>   as the record component and of type opentype(T).
> 
> The mapping from an instance of a record class to a CompositeData 
> corresponding to 
> the CompositeType is the same as specified as for other types (add a link).
> 
> A record is reconstructed from a CompositeData using its canonical 
> constructor. 
> The canonical constructor doesn't require the presence of 
> @javax.management.ConstructorParameters 
> or @java.beans.ConstructorProperties annotations. If these annotations are 
> present on
> the canonical constructor, they will be ignored.
> 
> If the CompositeData from which the record is reconstructed doesn't contain 
> all the record components, 
> the MXBean framework will attempt to reconstruct the record in the same way 
> as specified for 
> other types (add a link).

I have updated the PR with those changes, plus a few minor adjustments.

-

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v5]

[Daniel Fuchs]

> This RFE proposes to extend the MXBean framework to define a mapping to 
> records.
> 
> The MXBean framework already defines a mapping of `CompositeType` to plain 
> java objects. Records are a natural representation of CompositeTypes. A 
> record can be easily reconstructed from a `CompositeData` through the record 
> canonical constructor. A clear advantage of records over plain java objects 
> is that the canonical constructor will not need to be annotated in order to 
> map composite data property names to constructor parameter names.
> 
> With this RFE, here is an example comparing coding a composite type 
> `NamedNumber` that consists of an `int` and a `String`, using records and 
> using a plain java class. In both case, the `CompositeType` looks like this:
> 
> CompositeType(
> "NamedNumber",  // typeName
> "NamedNumber",  // description
> new String[] {"number", "name"},// itemNames
> new String[] {"number", "name"},// itemDescriptions
> new OpenType[] {SimpleType.INTEGER,
> SimpleType.STRING}  // itemTypes
> );
> 
> The plain Java class needs a public constructor annotated with 
> `@ConstructorParameters` annotation:
> 
> public class NamedNumber {
> public int getNumber() {return number;}
> public String getName() {return name;}
> @ConstructorParameters({"number", "name"})
> public NamedNumber(int number, String name) {
> this.number = number;
> this.name = name;
> }
> private final int number;
> private final String name;
> }
> 
> And the equivalent with a record class: 
> 
> public record NamedNumber(int number, String name) {}

Daniel Fuchs has updated the pull request incrementally with two additional 
commits since the last revision:

 - Integrated review feedback
 - Improved test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3201/files
  - new: https://git.openjdk.java.net/jdk/pull/3201/files/7d478dec..9227ba58

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3201=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3201=03-04

  Stats: 114 lines in 2 files changed: 53 ins; 13 del; 48 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3201.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3201/head:pull/3201

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8264538: Rename SystemDictionary::parse_stream [v2]

[Harold Seigel]

On Wed, 31 Mar 2021 19:57:57 GMT, Coleen Phillimore  wrote:

>> This function is used to call the classfile parser for hidden or anonymous 
>> classes, and for use with jvmti RedefineClasses. The latter only calls 
>> KlassFactory::create_from_stream and skips the rest of the code in 
>> SystemDictionary::parse_stream.
>> 
>> Renamed SystemDictionary::parse_stream -> resolve_hidden_class_from_stream
>> resolve_from_stream -> resolve_class_from_stream
>> and have SystemDictionary::resolve_from_stream() call the right version 
>> depending on ClassLoadInfo flags.  Callers of resolve_from_stream now pass 
>> protection domain via. ClassLoadInfo.
>> 
>> So the external API is resolve_from_stream.
>> 
>> Tested with tier1 on 4 Oracle supported platforms.
>
> Coleen Phillimore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   fifix comment

src/hotspot/share/prims/jvmtiRedefineClasses.hpp line 305:

> 303: // - How do we serialize the RedefineClasses() API without deadlocking?
> 304: //
> 305: // - KlassFactory::create_from_stream() was called with a NULL protection

Maybe delete the comment that goes from lines 305 - 309 ?

-

PR: https://git.openjdk.java.net/jdk/pull/3289

Re: RFR: 8264538: Rename SystemDictionary::parse_stream [v2]

[Harold Seigel]

On Wed, 31 Mar 2021 19:57:57 GMT, Coleen Phillimore  wrote:

>> This function is used to call the classfile parser for hidden or anonymous 
>> classes, and for use with jvmti RedefineClasses. The latter only calls 
>> KlassFactory::create_from_stream and skips the rest of the code in 
>> SystemDictionary::parse_stream.
>> 
>> Renamed SystemDictionary::parse_stream -> resolve_hidden_class_from_stream
>> resolve_from_stream -> resolve_class_from_stream
>> and have SystemDictionary::resolve_from_stream() call the right version 
>> depending on ClassLoadInfo flags.  Callers of resolve_from_stream now pass 
>> protection domain via. ClassLoadInfo.
>> 
>> So the external API is resolve_from_stream.
>> 
>> Tested with tier1 on 4 Oracle supported platforms.
>
> Coleen Phillimore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   fifix comment

src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 1395:

> 1393:   cl_info,
> 1394:   THREAD);
> 1395: 

Could you add a comment above line 1390 saying you can't call 
resolve_class_from_stream() here because the resulting class should not go in 
the system dictionary?

-

PR: https://git.openjdk.java.net/jdk/pull/3289

Re: RFR: 8264538: Rename SystemDictionary::parse_stream [v2]

[Lois Foltan]

On Wed, 31 Mar 2021 19:57:57 GMT, Coleen Phillimore  wrote:

>> This function is used to call the classfile parser for hidden or anonymous 
>> classes, and for use with jvmti RedefineClasses. The latter only calls 
>> KlassFactory::create_from_stream and skips the rest of the code in 
>> SystemDictionary::parse_stream.
>> 
>> Renamed SystemDictionary::parse_stream -> resolve_hidden_class_from_stream
>> resolve_from_stream -> resolve_class_from_stream
>> and have SystemDictionary::resolve_from_stream() call the right version 
>> depending on ClassLoadInfo flags.  Callers of resolve_from_stream now pass 
>> protection domain via. ClassLoadInfo.
>> 
>> So the external API is resolve_from_stream.
>> 
>> Tested with tier1 on 4 Oracle supported platforms.
>
> Coleen Phillimore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   fifix comment

Nice clean up Coleen.  One minor comment.

Thanks,
Lois

src/hotspot/share/prims/jvm.cpp line 950:

> 948:   InstanceKlass* ik = NULL;
> 949:   if (!is_hidden) {
> 950: ClassLoadInfo cl_info(protection_domain);

Minor comment, you could pull the creation of ClassLoadInfo out of this if 
statement since both the the if and the else sections create a ClassLoadInfo 
with pretty much the same information.

-

Marked as reviewed by lfoltan (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3289

Re: RFR: 8264538: Rename SystemDictionary::parse_stream [v2]

[Coleen Phillimore]

> This function is used to call the classfile parser for hidden or anonymous 
> classes, and for use with jvmti RedefineClasses. The latter only calls 
> KlassFactory::create_from_stream and skips the rest of the code in 
> SystemDictionary::parse_stream.
> 
> Renamed SystemDictionary::parse_stream -> resolve_hidden_class_from_stream
> resolve_from_stream -> resolve_class_from_stream
> and have SystemDictionary::resolve_from_stream() call the right version 
> depending on ClassLoadInfo flags.  Callers of resolve_from_stream now pass 
> protection domain via. ClassLoadInfo.
> 
> So the external API is resolve_from_stream.
> 
> Tested with tier1 on 4 Oracle supported platforms.

Coleen Phillimore has updated the pull request incrementally with one 
additional commit since the last revision:

  fifix comment

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3289/files
  - new: https://git.openjdk.java.net/jdk/pull/3289/files/ba7532bd..8dfcb093

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3289=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3289=00-01

  Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3289.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3289/head:pull/3289

PR: https://git.openjdk.java.net/jdk/pull/3289

RFR: 8264538: Rename SystemDictionary::parse_stream

[Coleen Phillimore]

This function is used to call the classfile parser for hidden or anonymous 
classes, and for use with jvmti RedefineClasses. The latter only calls 
KlassFactory::create_from_stream and skips the rest of the code in 
SystemDictionary::parse_stream.

Renamed SystemDictionary::parse_stream -> resolve_hidden_class_from_stream
resolve_from_stream -> resolve_class_from_stream
and have SystemDictionary::resolve_from_stream() call the right version 
depending on ClassLoadInfo flags.  Callers of resolve_from_stream now pass 
protection domain via. ClassLoadInfo.

So the external API is resolve_from_stream.

Tested with tier1 on 4 Oracle supported platforms.

-

Commit messages:
 - Rename parse_stream

Changes: https://git.openjdk.java.net/jdk/pull/3289/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3289=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264538
  Stats: 132 lines in 7 files changed: 37 ins; 23 del; 72 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3289.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3289/head:pull/3289

PR: https://git.openjdk.java.net/jdk/pull/3289

Re: RFR: 8264484: Replace uses of StringBuffer with StringBuilder in jdk.hotspot.agent [v3]

[Alex Menkov]

On Wed, 31 Mar 2021 17:45:33 GMT, Andrey Turbanov 
 wrote:

>> Found by IntelliJ IDEA inspection `Java | Java language level migration aids 
>> | Java 5 | 'StringBuffer' may be 'StringBuilder'`
>> As suggested in 
>> https://github.com/openjdk/jdk/pull/1507#issuecomment-757369003 I've created 
>> separate PR for module `jdk.hotspot.agent`
>> Similar cleanups in the past: 
>> https://bugs.openjdk.java.net/browse/JDK-8041679 
>> https://bugs.openjdk.java.net/browse/JDK-8264029
>
> Andrey Turbanov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8264484: Replace uses of StringBuffer with StringBuilder in 
> jdk.hotspot.agent
>   
>   fix indentation

Marked as reviewed by amenkov (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/3275

Re: RFR: 8264285: Clean the modification of ccstr JVM flags [v2]

[Ioi Lam]

On Wed, 31 Mar 2021 12:58:50 GMT, Coleen Phillimore  wrote:

>> Ioi Lam has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   relax flag attributions (ala JDK-7123237)
>
> src/hotspot/share/runtime/flags/debug_globals.hpp line 38:
> 
>> 36: // have any MANAGEABLE flags of the ccstr type, but we really need to
>> 37: // make sure the implementation is correct (in terms of memory 
>> allocation)
>> 38: // just in case someone may add such a flag in the future.
> 
> Could you have just added a develop flag to the manageable flags instead?

I had to use a `product` flag due to the following code, which should have been 
removed as part of 
[JDK-8243208](https://bugs.openjdk.java.net/browse/JDK-8243208), but I was 
afraid to do so because I didn't have a test case. I.e., all of our 
diagnostic/manageable/experimental flags were `product` flags.

With this PR, now I have a test case -- I changed `DummyManageableStringFlag` 
to a `notproduct` flag, and removed the following code. I am re-running 
tiers1-4 now.

void JVMFlag::check_all_flag_declarations() {
  for (JVMFlag* current = [0]; current->_name != NULL; current++) {
int flags = static_cast(current->_flags);
// Backwards compatibility. This will be relaxed/removed in JDK-7123237.
int mask = JVMFlag::KIND_DIAGNOSTIC | JVMFlag::KIND_MANAGEABLE | 
JVMFlag::KIND_EXPERIMENTAL;
if ((flags & mask) != 0) {
  assert((flags & mask) == JVMFlag::KIND_DIAGNOSTIC ||
 (flags & mask) == JVMFlag::KIND_MANAGEABLE ||
 (flags & mask) == JVMFlag::KIND_EXPERIMENTAL,
 "%s can be declared with at most one of "
 "DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL", current->_name);
  assert((flags & KIND_NOT_PRODUCT) == 0 &&
 (flags & KIND_DEVELOP) == 0,
 "%s has an optional DIAGNOSTIC, MANAGEABLE or EXPERIMENTAL "
 "attribute; it must be declared as a product flag", 
current->_name);
}
  }
}

-

PR: https://git.openjdk.java.net/jdk/pull/3254

Re: RFR: 8264285: Clean the modification of ccstr JVM flags [v2]

[Ioi Lam]

> There are two versions of JVMFlagAccess::ccstrAtPut() for modifying JVM flags 
> of the ccstr type (i.e., strings).
> 
> - One version requires the caller to free the old value, but some callers 
> don't do that (writeableFlags.cpp).
> - The other version frees the old value on behalf of the caller. However, 
> this version is accessible only via FLAG_SET_XXX macros and is currently 
> unused. So it's unclear whether it actually works.
> 
> We should combine these two versions into a single function, fix problems in 
> the callers, and add test cases. The old value should be freed automatically, 
> because typically the caller isn't interested in the old value.
> 
> Note that the FLAG_SET_XXX macros do not return the old value. Requiring the 
> caller of FLAG_SET_XXX to free the old value would be tedious and error prone.

Ioi Lam has updated the pull request incrementally with one additional commit 
since the last revision:

  relax flag attributions (ala JDK-7123237)

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3254/files
  - new: https://git.openjdk.java.net/jdk/pull/3254/files/7eca2343..673aaafc

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3254=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3254=00-01

  Stats: 37 lines in 4 files changed: 0 ins; 36 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3254.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3254/head:pull/3254

PR: https://git.openjdk.java.net/jdk/pull/3254

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v2]

[Mandy Chung]

On Wed, 31 Mar 2021 15:38:22 GMT, Daniel Fuchs  wrote:

>> Link added. With respect to adding a section for records, it's a bit 
>> difficult to separate that from the "Mapping for other types" without a lot 
>> of duplication. I can add a row in the summary table, and then add a new 
>> section "Mapping for records" just before "Mapping for other types", that 
>> just gives a brief overview and refers to the mapping for other types below. 
>> 
>> Something like this - what do you think?
>> 
>> Mappings for Records
>> 
>> A {@linkplain Record record} R whose {@linkplain
>>   Class#getRecordComponents() components} are
>>   all convertible to open types, is itself convertible to a
>>   {@link CompositeType} as follows.
>>   The type name of this {@code CompositeType}
>>   is determined by the same type name rules
>>   defined by the Mapping for other types
>>   below. Its getters are the accessors for the {@linkplain
>>   RecordComponent record components}, and the record is reconstructed
>>   using its canonical constructor, without needing any annotation.
>> 
>> A record may also expose additional non-canonical constructors, which
>>   can be used to reconstruct the record if the composite data does
>>   not exactly contain all the components expected by the
>>   canonical constructor. However, in order to be taken into account
>>   by the MXBean framework, such non-canonical constructors
>>   need to be annotated with either the {@link ConstructorParameters
>>   javax.management.ConstructorParameters} or
>>  {@code @java.beans.ConstructorProperties} annotation.
>> 
>> The complete rules for the mapping are detailed as part
>>   of the Mapping for other types
>>   below.
>> 
>> Mappings for other types
>> 
>> Given a record, or a Java class or interface J that does not 
>> match the other
>>   rules in the table above, the MXBean framework will attempt to map
>>   it to a {@link CompositeType} as follows.  []
>
> Hi Mandy, I have updated the PR with a revised version of the text above. Is 
> that more in line what you had in mind?
> 
> best regards,
> -- daniel

This looks better.  I like a separate mapping section for records since the 
mapping rules are straight-forward.   The complexity comes if we want to 
support a record to implement `CompositeDataView` that allows a type to support 
more flexible conversion to `CompositeData` which is why you add records in the 
"Mappings for other types".   I am wondering if we really want to support 
records to implement `CompositeDataView` but we may want to avoid such a 
special case.

I suggest add subsections in "Mappings for other types":   There are two 
mapping rules: (1) _opentype(J)_ maps `J` to `CompositeType` (2) _opendata(J)_ 
maps `J` to `CompositeData`.   _opentype(J)_ for record class does not need to 
refer to other types.   The common cases for _opendata(J)_ for record class is 
using the canonical constructors.   The other less common cases like using 
non-canonical constructors and `CompositeDataView` can state that it follows 
the same rules as specified for other types.   "Mappings for other types" does 
not need any change for records.   

"Reconstructing an instance of Java type J from a CompositeData" section should 
be renamed to "Reconstructing an instance of Java type or record class J from a 
CompositeData"

Here is a minor tweaking on "Mappings for Records"

A record is convertible to a CompositeType if and only if all its components 
are 
convertible to open types. Otherwise, it is not convertible.

A record class is converted to a CompositeType with one item for every record 
component as follows.
- The type name of this CompositeType is determined by the type name rules 
detailed below. 
- Each record component of type T, the item in the CompositeType has the same 
name 
  as the record component and of type opentype(T).

The mapping from an instance of a record class to a CompositeData corresponding 
to 
the CompositeType is the same as specified as for other types (add a link).

A record is reconstructed from a CompositeData using its canonical constructor. 
The canonical constructor doesn't require the presence of 
@javax.management.ConstructorParameters 
or @java.beans.ConstructorProperties annotations. If these annotations are 
present on
the canonical constructor, they will be ignored.

If the CompositeData from which the record is reconstructed doesn't contain all 
the record components, 
the MXBean framework will attempt to reconstruct the record in the same way as 
specified for 
other types (add a link).

-

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8264484: Replace uses of StringBuffer with StringBuilder in jdk.hotspot.agent [v3]

[Kevin Walls]

On Wed, 31 Mar 2021 17:45:33 GMT, Andrey Turbanov 
 wrote:

>> Found by IntelliJ IDEA inspection `Java | Java language level migration aids 
>> | Java 5 | 'StringBuffer' may be 'StringBuilder'`
>> As suggested in 
>> https://github.com/openjdk/jdk/pull/1507#issuecomment-757369003 I've created 
>> separate PR for module `jdk.hotspot.agent`
>> Similar cleanups in the past: 
>> https://bugs.openjdk.java.net/browse/JDK-8041679 
>> https://bugs.openjdk.java.net/browse/JDK-8264029
>
> Andrey Turbanov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8264484: Replace uses of StringBuffer with StringBuilder in 
> jdk.hotspot.agent
>   
>   fix indentation

Marked as reviewed by kevinw (Committer).

-

PR: https://git.openjdk.java.net/jdk/pull/3275

Re: RFR: 8264484: Replace uses of StringBuffer with StringBuilder in jdk.hotspot.agent [v3]

[Kevin Walls]

On Wed, 31 Mar 2021 15:42:03 GMT, Kevin Walls  wrote:

>> Andrey Turbanov has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   8264484: Replace uses of StringBuffer with StringBuilder in 
>> jdk.hotspot.agent
>>   
>>   fix indentation
>
> Changes requested by kevinw (Committer).

Thanks!

-

PR: https://git.openjdk.java.net/jdk/pull/3275

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v4]

[Daniel Fuchs]

> This RFE proposes to extend the MXBean framework to define a mapping to 
> records.
> 
> The MXBean framework already defines a mapping of `CompositeType` to plain 
> java objects. Records are a natural representation of CompositeTypes. A 
> record can be easily reconstructed from a `CompositeData` through the record 
> canonical constructor. A clear advantage of records over plain java objects 
> is that the canonical constructor will not need to be annotated in order to 
> map composite data property names to constructor parameter names.
> 
> With this RFE, here is an example comparing coding a composite type 
> `NamedNumber` that consists of an `int` and a `String`, using records and 
> using a plain java class. In both case, the `CompositeType` looks like this:
> 
> CompositeType(
> "NamedNumber",  // typeName
> "NamedNumber",  // description
> new String[] {"number", "name"},// itemNames
> new String[] {"number", "name"},// itemDescriptions
> new OpenType[] {SimpleType.INTEGER,
> SimpleType.STRING}  // itemTypes
> );
> 
> The plain Java class needs a public constructor annotated with 
> `@ConstructorParameters` annotation:
> 
> public class NamedNumber {
> public int getNumber() {return number;}
> public String getName() {return name;}
> @ConstructorParameters({"number", "name"})
> public NamedNumber(int number, String name) {
> this.number = number;
> this.name = name;
> }
> private final int number;
> private final String name;
> }
> 
> And the equivalent with a record class: 
> 
> public record NamedNumber(int number, String name) {}

Daniel Fuchs has updated the pull request incrementally with one additional 
commit since the last revision:

  Moved mapping for records just before Mapping for MXBean interface; Improved 
test.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3201/files
  - new: https://git.openjdk.java.net/jdk/pull/3201/files/59965133..7d478dec

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3201=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3201=02-03

  Stats: 127 lines in 2 files changed: 81 ins; 44 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3201.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3201/head:pull/3201

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8264484: Replace uses of StringBuffer with StringBuilder in jdk.hotspot.agent [v3]

[Andrey Turbanov]

> Found by IntelliJ IDEA inspection `Java | Java language level migration aids 
> | Java 5 | 'StringBuffer' may be 'StringBuilder'`
> As suggested in 
> https://github.com/openjdk/jdk/pull/1507#issuecomment-757369003 I've created 
> separate PR for module `jdk.hotspot.agent`
> Similar cleanups in the past: 
> https://bugs.openjdk.java.net/browse/JDK-8041679 
> https://bugs.openjdk.java.net/browse/JDK-8264029

Andrey Turbanov has updated the pull request incrementally with one additional 
commit since the last revision:

  8264484: Replace uses of StringBuffer with StringBuilder in jdk.hotspot.agent
  
  fix indentation

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3275/files
  - new: https://git.openjdk.java.net/jdk/pull/3275/files/75e4f6b0..32274cbc

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3275=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3275=01-02

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3275.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3275/head:pull/3275

PR: https://git.openjdk.java.net/jdk/pull/3275

Re: RFR: 8264484: Replace uses of StringBuffer with StringBuilder in jdk.hotspot.agent [v3]

[Andrey Turbanov]

On Wed, 31 Mar 2021 16:35:32 GMT, Kevin Walls  wrote:

>> done
>
> Thanks - just a nit, looks like an extra space means "return genBaseHref() 
> ..etc.." is not aligned.

fixed

-

PR: https://git.openjdk.java.net/jdk/pull/3275

Integrated: 8264396: Use the blessed modifier order in jdk.internal.jvmstat

[Alex Blewitt]

On Mon, 29 Mar 2021 21:00:20 GMT, Alex Blewitt 
 wrote:

> 8264396: Use the blessed modifier order in jdk.internal.jvmstat

This pull request has now been integrated.

Changeset: f43d14a2
Author:Alex Blewitt 
Committer: Kevin Walls 
URL:   https://git.openjdk.java.net/jdk/commit/f43d14a2
Stats: 83 lines in 5 files changed: 0 ins; 0 del; 83 mod

8264396: Use the blessed modifier order in jdk.internal.jvmstat

Reviewed-by: cjplummer, kevinw, shade

-

PR: https://git.openjdk.java.net/jdk/pull/3252

Re: RFR: 8264484: Replace uses of StringBuffer with StringBuilder in jdk.hotspot.agent [v2]

[Kevin Walls]

On Wed, 31 Mar 2021 16:07:56 GMT, Andrey Turbanov 
 wrote:

>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/ui/classbrowser/HTMLGenerator.java
>>  line 887:
>> 
>>> 885:protected String genMultPCHref(String pcs) {
>>> 886:   String buf = genBaseHref() + "pc_multiple=" + pcs;
>>> 887:   return buf;
>> 
>> This is like VMVersionMismatchException.java above, where as it is reduced 
>> to two lines, we may as well reduce it to one line with:
>> return genBaseHref() + ..etc... 
>> 
>> With these two minor changes, I think this looks good.
>
> done

Thanks - just a nit, looks like an extra space means "return genBaseHref() 
..etc.." is not aligned.

-

PR: https://git.openjdk.java.net/jdk/pull/3275

Re: RFR: 8264396: Use the blessed modifier order in jdk.internal.jvmstat [v2]

[Chris Plummer]

On Tue, 30 Mar 2021 20:37:45 GMT, Alex Blewitt 
 wrote:

>> 8264396: Use the blessed modifier order in jdk.internal.jvmstat
>
> Alex Blewitt has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Updated copyright dates

Marked as reviewed by cjplummer (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/3252

Re: RFR: 8264484: Replace uses of StringBuffer with StringBuilder in jdk.hotspot.agent [v2]

[Andrey Turbanov]

On Wed, 31 Mar 2021 15:41:42 GMT, Kevin Walls  wrote:

>> Andrey Turbanov has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   8264484: Replace uses of StringBuffer with StringBuilder in 
>> jdk.hotspot.agent
>>   
>>   remove redundant local variables as suggested by reviewers
>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/ui/classbrowser/HTMLGenerator.java
>  line 887:
> 
>> 885:protected String genMultPCHref(String pcs) {
>> 886:   String buf = genBaseHref() + "pc_multiple=" + pcs;
>> 887:   return buf;
> 
> This is like VMVersionMismatchException.java above, where as it is reduced to 
> two lines, we may as well reduce it to one line with:
> return genBaseHref() + ..etc... 
> 
> With these two minor changes, I think this looks good.

done

-

PR: https://git.openjdk.java.net/jdk/pull/3275

Re: RFR: 8264484: Replace uses of StringBuffer with StringBuilder in jdk.hotspot.agent [v2]

[Andrey Turbanov]

On Wed, 31 Mar 2021 08:23:24 GMT, Yasumasa Suenaga  wrote:

>> Andrey Turbanov has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   8264484: Replace uses of StringBuffer with StringBuilder in 
>> jdk.hotspot.agent
>>   
>>   remove redundant local variables as suggested by reviewers
>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/VMVersionMismatchException.java
>  line 40:
> 
>> 38: ". Target VM is " + targetVersion;
>> 39: return msg;
>> 40: }
> 
> Why don't you return `msg` directly? I think `msg` is not needed.
> I saw similar codes in other places.

inlined

-

PR: https://git.openjdk.java.net/jdk/pull/3275

Re: RFR: 8264484: Replace uses of StringBuffer with StringBuilder in jdk.hotspot.agent [v2]

[Andrey Turbanov]

> Found by IntelliJ IDEA inspection `Java | Java language level migration aids 
> | Java 5 | 'StringBuffer' may be 'StringBuilder'`
> As suggested in 
> https://github.com/openjdk/jdk/pull/1507#issuecomment-757369003 I've created 
> separate PR for module `jdk.hotspot.agent`
> Similar cleanups in the past: 
> https://bugs.openjdk.java.net/browse/JDK-8041679 
> https://bugs.openjdk.java.net/browse/JDK-8264029

Andrey Turbanov has updated the pull request incrementally with one additional 
commit since the last revision:

  8264484: Replace uses of StringBuffer with StringBuilder in jdk.hotspot.agent
  
  remove redundant local variables as suggested by reviewers

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3275/files
  - new: https://git.openjdk.java.net/jdk/pull/3275/files/cd4fbf1d..75e4f6b0

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3275=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3275=00-01

  Stats: 4 lines in 2 files changed: 0 ins; 2 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3275.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3275/head:pull/3275

PR: https://git.openjdk.java.net/jdk/pull/3275

Re: RFR: 8264484: Replace uses of StringBuffer with StringBuilder in jdk.hotspot.agent

[Kevin Walls]

On Tue, 30 Mar 2021 18:58:24 GMT, Andrey Turbanov 
 wrote:

> Found by IntelliJ IDEA inspection `Java | Java language level migration aids 
> | Java 5 | 'StringBuffer' may be 'StringBuilder'`
> As suggested in 
> https://github.com/openjdk/jdk/pull/1507#issuecomment-757369003 I've created 
> separate PR for module `jdk.hotspot.agent`
> Similar cleanups in the past: 
> https://bugs.openjdk.java.net/browse/JDK-8041679 
> https://bugs.openjdk.java.net/browse/JDK-8264029

Changes requested by kevinw (Committer).

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/ui/classbrowser/HTMLGenerator.java
 line 887:

> 885:protected String genMultPCHref(String pcs) {
> 886:   String buf = genBaseHref() + "pc_multiple=" + pcs;
> 887:   return buf;

This is like VMVersionMismatchException.java above, where as it is reduced to 
two lines, we may as well reduce it to one line with:
return genBaseHref() + ..etc... 

With these two minor changes, I think this looks good.

-

PR: https://git.openjdk.java.net/jdk/pull/3275

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v2]

[Daniel Fuchs]

On Mon, 29 Mar 2021 22:12:01 GMT, Daniel Fuchs  wrote:

>> You add record in "Mappings for other types".  I think it deserves a 
>> separate section "Mappings for record classes" (maybe after primitive 
>> types).   It's useful to add a row for record in the summary table above 
>> "Mappings for primitive types"
>
> Link added. With respect to adding a section for records, it's a bit 
> difficult to separate that from the "Mapping for other types" without a lot 
> of duplication. I can add a row in the summary table, and then add a new 
> section "Mapping for records" just before "Mapping for other types", that 
> just gives a brief overview and refers to the mapping for other types below. 
> 
> Something like this - what do you think?
> 
> Mappings for Records
> 
> A {@linkplain Record record} R whose {@linkplain
>   Class#getRecordComponents() components} are
>   all convertible to open types, is itself convertible to a
>   {@link CompositeType} as follows.
>   The type name of this {@code CompositeType}
>   is determined by the same type name rules
>   defined by the Mapping for other types
>   below. Its getters are the accessors for the {@linkplain
>   RecordComponent record components}, and the record is reconstructed
>   using its canonical constructor, without needing any annotation.
> 
> A record may also expose additional non-canonical constructors, which
>   can be used to reconstruct the record if the composite data does
>   not exactly contain all the components expected by the
>   canonical constructor. However, in order to be taken into account
>   by the MXBean framework, such non-canonical constructors
>   need to be annotated with either the {@link ConstructorParameters
>   javax.management.ConstructorParameters} or
>  {@code @java.beans.ConstructorProperties} annotation.
> 
> The complete rules for the mapping are detailed as part
>   of the Mapping for other types
>   below.
> 
> Mappings for other types
> 
> Given a record, or a Java class or interface J that does not 
> match the other
>   rules in the table above, the MXBean framework will attempt to map
>   it to a {@link CompositeType} as follows.  []

Hi Mandy, I have updated the PR with a revised version of the text above. Is 
that more in line what you had in mind?

best regards,
-- daniel

-

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v3]

[Daniel Fuchs]

> This RFE proposes to extend the MXBean framework to define a mapping to 
> records.
> 
> The MXBean framework already defines a mapping of `CompositeType` to plain 
> java objects. Records are a natural representation of CompositeTypes. A 
> record can be easily reconstructed from a `CompositeData` through the record 
> canonical constructor. A clear advantage of records over plain java objects 
> is that the canonical constructor will not need to be annotated in order to 
> map composite data property names to constructor parameter names.
> 
> With this RFE, here is an example comparing coding a composite type 
> `NamedNumber` that consists of an `int` and a `String`, using records and 
> using a plain java class. In both case, the `CompositeType` looks like this:
> 
> CompositeType(
> "NamedNumber",  // typeName
> "NamedNumber",  // description
> new String[] {"number", "name"},// itemNames
> new String[] {"number", "name"},// itemDescriptions
> new OpenType[] {SimpleType.INTEGER,
> SimpleType.STRING}  // itemTypes
> );
> 
> The plain Java class needs a public constructor annotated with 
> `@ConstructorParameters` annotation:
> 
> public class NamedNumber {
> public int getNumber() {return number;}
> public String getName() {return name;}
> @ConstructorParameters({"number", "name"})
> public NamedNumber(int number, String name) {
> this.number = number;
> this.name = name;
> }
> private final int number;
> private final String name;
> }
> 
> And the equivalent with a record class: 
> 
> public record NamedNumber(int number, String name) {}

Daniel Fuchs has updated the pull request with a new target base due to a merge 
or a rebase. The incremental webrev excludes the unrelated changes brought in 
by the merge/rebase. The pull request contains seven additional commits since 
the last revision:

 - Merge branch 'master' into mxbeans-8264124
 - Minor tweaks. Improved test.
 - Merge branch 'master' into mxbeans-8264124
 - Integrated review feedback. Updated the section for Mapping to records.
 - Integrated review feedback. Added a section for Mapping to records.
 - Fixed typo. Moved example with record after example with from method to 
respect the logical order of precedence.
 - 8264124: Update MXBean specification and implementation to extend mapping of 
CompositeType to records

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3201/files
  - new: https://git.openjdk.java.net/jdk/pull/3201/files/1c3292ce..59965133

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3201=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3201=01-02

  Stats: 15001 lines in 575 files changed: 11278 ins; 1193 del; 2530 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3201.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3201/head:pull/3201

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8257831: Suspend with handshakes [v2]

[Robbin Ehn]

On Wed, 31 Mar 2021 10:48:11 GMT, Robbin Ehn  wrote:

>> Thanks @dcubed-ojdk - no it won't. The problem is that the signal can hit 
>> very late in a thread's termination process, after the JavaThread destructor 
>> has executed, so no query of JavaThread state is possible. So we needed 
>> something in the Thread state and the SR_lock was a good enough proxy for 
>> that. It may be possible to use a different Thread field (perhaps _ParkEvent 
>> but encapsulated in a Thread::has_terminated() helper method).
>
> SR_handler is used for OS-level suspend/resume (not to conflict with this 
> change-set).
> This feature is only used by JFR (AFAIK), and JFR only samples threads on 
> it's ThreadsList.
> This means the JavaThread can never be terminated, hence this code will 
> always pass.
> 
> If someone else is using OS-level suspend/resume without a ThreadsList, the 
> bug is there is no ThreadsList AFAICT.
> 
> Since ThreadLocalStorage::thread() is cleared last in ~Thread with 
> Thread::clear_thread_current(); may be in the ~Thread destructor.
> So the argument is that would be safe to read stuff from Thread but not 
> JavaThread?
> Since the compiler (and CPU) may reorder and optimize away code, so I argue 
> reading from a half destroyed object is not a great idea.
> E.g. Monitor* _SR_lock; is not a volatile pointer; since reading from this 
> memory is UB after destruction, compiler is free to remove or not publish the 
> store to NULL.
> 
> So I suggest either to remove this check, since the only user is using a 
> ThreadsList and any other should also be using that too.
> Or call Thread::clear_thread_current() before the JavaThread destructor is 
> called, that way we can be certain that there is no UB.

I got some offline input from David, there seem to be an issue with signal 
being delivered after the ThreadsListHandle destructor. If that is the case a 
ThreadsList doesn't help here.

So I suggested we keep this out of this change-set and just take another 
suitable field to mirror what we have today.

`ParkEvent * _ParkEvent;` ?

-

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8257831: Suspend with handshakes [v2]

[Daniel D . Daugherty]

On Wed, 31 Mar 2021 10:22:28 GMT, Richard Reingruber  wrote:

>> My comment was about JVMTI_ERROR_THREAD_NOT_ALIVE
>
> Sure. I agree with your comment.

It depends on how late we can call into an event handler during the
JavaThread exit code path. Is it possible to post an event on a thread
that has passed the `terminated` point in JavaThread::exit()?

-

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8257831: Suspend with handshakes [v2]

[Daniel D . Daugherty]

On Wed, 31 Mar 2021 03:47:28 GMT, David Holmes  wrote:

>> Robbin Ehn has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains two commits:
>> 
>>  - Merge branch 'master' into SuspendInHandshake
>>  - 8257831: Suspend with handshake (review baseline)
>
> src/hotspot/share/prims/jvmtiEnv.cpp line 946:
> 
>> 944: // java_thread - pre-checked
>> 945: jvmtiError
>> 946: JvmtiEnv::SuspendThread(JavaThread* java_thread) {
> 
> The comment above this still states:
> 
> // Threads_lock NOT held, java_thread not protected by lock
> 
> but the java_thread is protected by a TLH so we should document that so we 
> know it is always safe to refer to java_thread below.

These `Threads_lock NOT held ...` comments in JVM/TI are a left over
issue from the Thread-SMR project and I think I forgot to file a follow-up
bug to clean those up.

I recommend handling that in a general cleanup issue for all of these
comments in JVM/TI (and possibly elsewhere). Please let me know if I
should go ahead and file that bug.

-

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8257831: Suspend with handshakes [v2]

[David Holmes]

On 31/03/2021 8:51 pm, Robbin Ehn wrote:

On Wed, 31 Mar 2021 02:43:21 GMT, David Holmes  wrote:


src/hotspot/os/posix/signals_posix.cpp line 1587:


1585:   // destructor has completed.
1586:
1587:   if (thread->is_Java_thread() && 
thread->as_Java_thread()->is_terminated()) {


We need @dholmes-ora to verify that this version of the code will
still solve the bug he was fixing when he added old L1610.


Thanks @dcubed-ojdk - no it won't. The problem is that the signal can hit very 
late in a thread's termination process, after the JavaThread destructor has 
executed, so no query of JavaThread state is possible. So we needed something 
in the Thread state and the SR_lock was a good enough proxy for that. It may be 
possible to use a different Thread field (perhaps _ParkEvent but encapsulated 
in a Thread::has_terminated() helper method).


SR_handler is used for OS-level suspend/resume (not to conflict with this 
change-set).
This feature is only used by JFR (AFAIK), and JFR only samples threads on it's 
ThreadsList.
This means the JavaThread can never be terminated, hence this code will always 
pass.


I need to dig up the exact details but from memory the problem we were 
fixing was that the signal was seemingly not being delivered to the 
target thread until it modified its own signal mask sometime during 
thread termination. IIRC JFR may have a target thread on a ThreadList 
and try to suspend it, but the signal may not get delivered, 
consequently JFR will timeout and cancel the suspend request. After that 
the thread can continue on its way and may start to terminate, at which 
point the signal gets delivered and the SR_handler runs.



If someone else is using OS-level suspend/resume without a ThreadsList, the bug 
is there is no ThreadsList AFAICT.

Since ThreadLocalStorage::thread() is cleared last in ~Thread with 
Thread::clear_thread_current(); may be in the ~Thread destructor.
So the argument is that would be safe to read stuff from Thread but not 
JavaThread?


If the SR_handler gets a non-null value from 
Thread::current_or_null_safe() then we know that the signal got 
delivered before we called clear_current_thread() in the Thread 
destructor. We don't exactly where, but we know that it could be after 
the JavaThread destructor and so trying to treat the thread as a 
JavaThread is definitely not safe (hence the way the bug was 
discovered!). We deliberately nulled _SR_lock so that we could query it 
as a termination indicator and we do that while we still have a valid 
osThread. Hence if you see a non-NULL _SR_lock you know you can safely 
interact with the osthread.



Since the compiler (and CPU) may reorder and optimize away code, so I argue 
reading from a half destroyed object is not a great idea.


The Thread object itself is not destroyed at this point, only objects 
referred to there-from.



E.g. Monitor* _SR_lock; is not a volatile pointer; since reading from this 
memory is UB after destruction, compiler is free to remove or not publish the 
store to NULL.


It may well be free to do so but that wasn't what we found. Perhaps it 
should have been volatile with a storestore() for good measure.



So I suggest either to remove this check, since the only user is using a 
ThreadsList and any other should also be using that too.
Or call Thread::clear_thread_current() before the JavaThread destructor is 
called, that way we can be certain that there is no UB.


That would be very fragile IMO as it would require that no code 
executing from the destructors can require access to Thread:current() to 
work. It would certainly take some analysis to determine if it could be 
done that way. (Crash reporting would be affected.)


There is no UB in what we currently do AFAICS.

David
-


-

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8264285: Clean the modification of ccstr JVM flags

[Coleen Phillimore]

On Mon, 29 Mar 2021 21:35:52 GMT, Ioi Lam  wrote:

> There are two versions of JVMFlagAccess::ccstrAtPut() for modifying JVM flags 
> of the ccstr type (i.e., strings).
> 
> - One version requires the caller to free the old value, but some callers 
> don't do that (writeableFlags.cpp).
> - The other version frees the old value on behalf of the caller. However, 
> this version is accessible only via FLAG_SET_XXX macros and is currently 
> unused. So it's unclear whether it actually works.
> 
> We should combine these two versions into a single function, fix problems in 
> the callers, and add test cases. The old value should be freed automatically, 
> because typically the caller isn't interested in the old value.
> 
> Note that the FLAG_SET_XXX macros do not return the old value. Requiring the 
> caller of FLAG_SET_XXX to free the old value would be tedious and error prone.

I had a question but overall nice cleanup!

src/hotspot/share/runtime/flags/debug_globals.hpp line 38:

> 36: // have any MANAGEABLE flags of the ccstr type, but we really need to
> 37: // make sure the implementation is correct (in terms of memory allocation)
> 38: // just in case someone may add such a flag in the future.

Could you have just added a develop flag to the manageable flags instead?

src/hotspot/share/runtime/flags/jvmFlagAccess.cpp line 327:

> 325:   // The callers typically don't care what the old value is.
> 326:   // If the caller really wants to know the old value, read it (and make 
> a copy if necessary)
> 327:   // before calling this API.

good comment!

-

Marked as reviewed by coleenp (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3254

Re: RFR: 8257831: Suspend with handshakes [v2]

[Robbin Ehn]

On Tue, 30 Mar 2021 08:20:21 GMT, Richard Reingruber  wrote:

>> Robbin Ehn has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains two commits:
>> 
>>  - Merge branch 'master' into SuspendInHandshake
>>  - 8257831: Suspend with handshake (review baseline)
>
> src/hotspot/share/prims/jvmtiRawMonitor.cpp line 318:
> 
>> 316:   if (self->is_Java_thread()) {
>> 317: jt = self->as_Java_thread();
>> 318: while (true) {
> 
> From the PR description:
> 
>> Some code can be simplified or done in a smarter way but I refrained from 
>> doing such changes instead tried to keep existing code as is as far as 
>> possible. This concerns especially raw monitors.
> 
> I read this section but then forgot about it. So you can skip the following 
> comment.
> 
> Possible follow-up: It could be worth the attempt to make use of the generic 
> state transition classes provided by interface.hpp. Directly I don't see why 
> this wouldn't be feasible and I doubt the [old comment in 
> JvmtiEnv::RawMonitorEnter()](https://github.com/reinrich/jdk/blob/aefc1560b51f0ce96d8f5ce396ba0d2fe08fd650/src/hotspot/share/prims/jvmtiEnv.cpp#L3208-L3214).
> This would make the custom suspend logic here superfluous.
> If ThreadBlockInVM was changed to take a generic callback for unlocking, then 
> this could replace the custom logic in L360-L374.

You are right on, this has been briefly discussed off-line.

-

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8257831: Suspend with handshakes [v2]

[Robbin Ehn]

On Wed, 31 Mar 2021 08:01:27 GMT, Richard Reingruber  wrote:

>> I think this relates to why the PEM was moved from the loop-scope to the 
>> sync op case only. That said it isn't clear why we need the HM or PEM.
>
> I guess it should be "... must not execute ~PreserveExceptionMark ..."
> ~PreserveExceptionMark calls _thread->pending_exception() which is an oop and 
> that would be illegal as the vm could be at a safepoint when the async 
> handshake returns.

I don't think we need HM or PEM.
PEM was added if the handshake would execute code that did throw an exception.

Yes, exactly we don't want to be touching "_thread->pending_exception()" during 
a safepoint.

Updating comment.

-

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8257831: Suspend with handshakes [v2]

[Robbin Ehn]

On Mon, 29 Mar 2021 17:23:11 GMT, Richard Reingruber  wrote:

>> Robbin Ehn has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains two commits:
>> 
>>  - Merge branch 'master' into SuspendInHandshake
>>  - 8257831: Suspend with handshake (review baseline)
>
> src/hotspot/share/runtime/handshake.cpp line 492:
> 
>> 490:   }
>> 491: } else {
>> 492:   return true;
> 
> `true` as return value means that the caller does _not_ need to check again 
> for a safepoint, correct? Maybe this could be added as comment to the method 
> declaration?

Yes, added comment!

-

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8257831: Suspend with handshakes [v2]

[Robbin Ehn]

On Tue, 30 Mar 2021 21:12:47 GMT, Richard Reingruber  wrote:

> Hi Robbin,
> 
> this is a great simplification. Excellent work! :)
> 
> Thanks, Richard.

Thanks Richard!

> src/hotspot/share/prims/jvmtiRawMonitor.cpp line 421:
> 
>> 419: guarantee(jt->thread_state() == _thread_in_native, "invariant");
>> 420: for (;;) {
>> 421:   if (!SafepointMechanism::should_process(jt)) {
> 
> Potential follow-up enhancement: there's a very similar for(;;) loop in 
> JvmtiRawMonitor::raw_enter(). It looks as if it was possible to merge both 
> loops into just one loop in simple_enter().

Yes!

-

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8257831: Suspend with handshakes [v2]

[Robbin Ehn]

On Tue, 30 Mar 2021 17:15:52 GMT, Daniel D. Daugherty  
wrote:

> This is an elegant evolution of the suspend/resume mechanism.
> 
> It is so nice to see all the suspend-equivalent stuff go away!

Thanks Dan!

> src/hotspot/share/runtime/handshake.hpp line 37:
> 
>> 35: class JavaThread;
>> 36: class ThreadSuspensionHandshake;
>> 37: class SuspendThreadHandshake;
> 
> Should these be in alpha sort order?

Thanks, fixed.

> src/hotspot/share/runtime/handshake.hpp line 147:
> 
>> 145:   bool handshake_suspend();
>> 146:   // Called from the async handshake (the trap)
>> 147:   // to stop a thread from continuing executing when suspended.
> 
> nit typo: s/continuing executing/continuing execution/

Thanks, Fixed.

-

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8257831: Suspend with handshakes [v2]

[Robbin Ehn]

On Wed, 31 Mar 2021 02:43:21 GMT, David Holmes  wrote:

>> src/hotspot/os/posix/signals_posix.cpp line 1587:
>> 
>>> 1585:   // destructor has completed.
>>> 1586: 
>>> 1587:   if (thread->is_Java_thread() && 
>>> thread->as_Java_thread()->is_terminated()) {
>> 
>> We need @dholmes-ora to verify that this version of the code will
>> still solve the bug he was fixing when he added old L1610.
>
> Thanks @dcubed-ojdk - no it won't. The problem is that the signal can hit 
> very late in a thread's termination process, after the JavaThread destructor 
> has executed, so no query of JavaThread state is possible. So we needed 
> something in the Thread state and the SR_lock was a good enough proxy for 
> that. It may be possible to use a different Thread field (perhaps _ParkEvent 
> but encapsulated in a Thread::has_terminated() helper method).

SR_handler is used for OS-level suspend/resume (not to conflict with this 
change-set).
This feature is only used by JFR (AFAIK), and JFR only samples threads on it's 
ThreadsList.
This means the JavaThread can never be terminated, hence this code will always 
pass.

If someone else is using OS-level suspend/resume without a ThreadsList, the bug 
is there is no ThreadsList AFAICT.

Since ThreadLocalStorage::thread() is cleared last in ~Thread with 
Thread::clear_thread_current(); may be in the ~Thread destructor.
So the argument is that would be safe to read stuff from Thread but not 
JavaThread?
Since the compiler (and CPU) may reorder and optimize away code, so I argue 
reading from a half destroyed object is not a great idea.
E.g. Monitor* _SR_lock; is not a volatile pointer; since reading from this 
memory is UB after destruction, compiler is free to remove or not publish the 
store to NULL.

So I suggest either to remove this check, since the only user is using a 
ThreadsList and any other should also be using that too.
Or call Thread::clear_thread_current() before the JavaThread destructor is 
called, that way we can be certain that there is no UB.

-

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8257831: Suspend with handshakes [v2]

[Richard Reingruber]

On Wed, 31 Mar 2021 09:18:56 GMT, David Holmes  wrote:

>> Another thread can win the race to suspend the current thread (if 
>> claim_handshake() in try_process() fails). Then JVMTI_ERROR_THREAD_SUSPENDED 
>> should be returned.
>
> My comment was about JVMTI_ERROR_THREAD_NOT_ALIVE

Sure. I agree with your comment.

-

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8257831: Suspend with handshakes [v3]

[Richard Reingruber]

On Wed, 31 Mar 2021 06:42:27 GMT, David Holmes  wrote:

>> Robbin Ehn has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains three commits:
>> 
>>  - Merge branch 'master' into SuspendInHandshake
>>  - Merge branch 'master' into SuspendInHandshake
>>  - 8257831: Suspend with handshake (review baseline)
>
> src/hotspot/share/runtime/thread.cpp line 2105:
> 
>> 2103: if (is_external_suspend()) {
>> 2104:   java_suspend_self();
>> 2105: }
> 
> It is not at all clear to me how this method should interact with thread 
> suspension ??

In JavaThread::wait_for_object_deoptimization() the current thread can 
transition to the safe state _thread_blocked. In that state it can be 
suspended. In the baseline version wait_for_object_deoptimization() checks for 
suspension explicitly and self suspends if positive. With this enhancement it 
happens implicitly by calling SafepointMechanism::process_if_requested().

-

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8264396: Use the blessed modifier order in jdk.internal.jvmstat [v2]

[Alex Blewitt]

On Wed, 31 Mar 2021 09:24:46 GMT, Alex Blewitt 
 wrote:

>> Looks fine to me as well.
>
> Going to upstream this patch into opendjk/panama-foreign

Sorry, closed the wrong PR. This one should be good to go 

-

PR: https://git.openjdk.java.net/jdk/pull/3252

Re: RFR: 8264396: Use the blessed modifier order in jdk.internal.jvmstat [v2]

[Alex Blewitt]

On Wed, 31 Mar 2021 06:07:57 GMT, Aleksey Shipilev  wrote:

>> Alex Blewitt has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Updated copyright dates
>
> Looks fine to me as well.

Going to upstream this patch into opendjk/panama-foreign

-

PR: https://git.openjdk.java.net/jdk/pull/3252

Withdrawn: 8264396: Use the blessed modifier order in jdk.internal.jvmstat

[Alex Blewitt]

On Mon, 29 Mar 2021 21:00:20 GMT, Alex Blewitt 
 wrote:

> 8264396: Use the blessed modifier order in jdk.internal.jvmstat

This pull request has been closed without being integrated.

-

PR: https://git.openjdk.java.net/jdk/pull/3252

Re: RFR: 8257831: Suspend with handshakes [v2]

[David Holmes]

On Wed, 31 Mar 2021 07:48:00 GMT, Richard Reingruber  wrote:

>> src/hotspot/share/prims/jvmtiEnv.cpp line 1009:
>> 
>>> 1007:   if (self_index >= 0) {
>>> 1008: if (!JvmtiSuspendControl::suspend(current)) {
>>> 1009:   results[self_index] = JVMTI_ERROR_THREAD_NOT_ALIVE;
>> 
>> Surely impossible when dealing with the current thread!
>
> Another thread can win the race to suspend the current thread (if 
> claim_handshake() in try_process() fails). Then JVMTI_ERROR_THREAD_SUSPENDED 
> should be returned.

My comment was about JVMTI_ERROR_THREAD_NOT_ALIVE

-

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8257831: Suspend with handshakes [v2]

[Robbin Ehn]

On Tue, 30 Mar 2021 17:08:07 GMT, Daniel D. Daugherty  
wrote:

>> Robbin Ehn has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains two commits:
>> 
>>  - Merge branch 'master' into SuspendInHandshake
>>  - 8257831: Suspend with handshake (review baseline)
>
> src/hotspot/share/runtime/objectMonitor.cpp line 422:
> 
>> 420: _recursions = 0;
>> 421: _succ = NULL;
>> 422: exit(false, current);
> 
> You'll have a conflict with @pchilano recent fix.

Fixed with a merge with master.

-

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8257831: Suspend with handshakes [v3]

[Robbin Ehn]

> A suspend request is done by handshaking thread target thread(s). When 
> executing the handshake operation we know the target mutator thread is in a 
> dormant state (as in safepoint safe state). We have a guarantee that it will 
> check it's poll before leaving the dormant state. To stop the thread from 
> leaving the the dormant state we install a second asynchronous handshake to 
> be executed by the targeted thread. The asynchronous handshake will wait on a 
> monitor while the thread is suspended. The target thread cannot not leave the 
> dormant state without a resume request.
> 
> Per thread suspend requests are naturally serialized by the per thread 
> HandshakeState lock (we can only execute one handshake at a time per thread).
> Instead of having a separate lock we use this to our advantage and use 
> HandshakeState lock for serializing access to the suspend flag and for 
> wait/notify. 
> 
> Suspend:
> Requesting thread -> synchronous handshake -> target thread
> Inside synchronus handshake (HandshakeState lock is locked while
> executing any handshake):
>   - Set suspended flag
>   - Install asynchronous handshake
> 
> Target thread -> tries to leave dormant state -> Executes handshakes
> Target only executes asynchronous handshake:
>   - While suspended
>   - Go to blocked
>   - Wait on HandshakeState lock
> 
> Resume:
> Resuming thread:
>   - Lock HandshakeState lock
>   - Clear suspended flag
>   - Notify HandshakeState lock
>   - Unlock HandshakeState lock
> 
> The "suspend requested" flag is an optimization, without it a dormant thread 
> could be suspended and resumed many times and each would add a new 
> asynchronous handshake. Suspend requested flag means there is already an 
> asynchronous suspend handshake in queue which can be re-used, only the 
> suspend flag needs to be set.
> 
> 
> Some code can be simplified or done in a smarter way but I refrained from 
> doing such changes instead tried to keep existing code as is as far as 
> possible. This concerns especially raw monitors.
> 
> 
> Regarding the changed test, the documentation says:
> "If the calling thread is specified in the request_list array, this function 
> will not return until some other thread resumes it."
> 
> But the code:
>   LOG("suspendTestedThreads: before JVMTI SuspendThreadList");
>   err = jvmti->SuspendThreadList(threads_count, threads, results);
>   ...
>   // Allow the Main thread to inspect the result of tested threads suspension 
>   agent_unlock(jni);
>   
> The thread will never return from SuspendThreadList until resumed, so it 
> cannot unlock with agent_unlock().
> Thus main thread is stuck forever on:
>   // Block until the suspender thread competes the tested threads suspension  
>   agent_lock(jni);
> 
> And never checks and resumes the threads. So I removed that lock instead just 
> sleep and check until all thread have the expected suspended state.
> 
> 
> 
> This version already contains updates after pre-review comments from 
> @dcubed-ojdk, @pchilano, @coleenp.
> (Pre-review comments here:
> https://github.com/openjdk/jdk/pull/2625)
> 
>  
> Testing t1-t8, nsk_jdi/nsk_jvmti/jdk_jdi/tck, KS24, RunThese and
> combinations like running with -XX:ZCollectionInterval=0.01 -
> XX:ZFragmentationLimit=0.
> Running above some of above concurrently (load ~240), slow debug,
> etc...

Robbin Ehn has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains three commits:

 - Merge branch 'master' into SuspendInHandshake
 - Merge branch 'master' into SuspendInHandshake
 - 8257831: Suspend with handshake (review baseline)

-

Changes: https://git.openjdk.java.net/jdk/pull/3191/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3191=02
  Stats: 1313 lines in 38 files changed: 271 ins; 855 del; 187 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3191.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3191/head:pull/3191

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8264484: Replace uses of StringBuffer with StringBuilder in jdk.hotspot.agent

[Yasumasa Suenaga]

On Tue, 30 Mar 2021 18:58:24 GMT, Andrey Turbanov 
 wrote:

> Found by IntelliJ IDEA inspection `Java | Java language level migration aids 
> | Java 5 | 'StringBuffer' may be 'StringBuilder'`
> As suggested in 
> https://github.com/openjdk/jdk/pull/1507#issuecomment-757369003 I've created 
> separate PR for module `jdk.hotspot.agent`
> Similar cleanups in the past: 
> https://bugs.openjdk.java.net/browse/JDK-8041679 
> https://bugs.openjdk.java.net/browse/JDK-8264029

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/VMVersionMismatchException.java
 line 40:

> 38: ". Target VM is " + targetVersion;
> 39: return msg;
> 40: }

Why don't you return `msg` directly? I think `msg` is not needed.
I saw similar codes in other places.

-

PR: https://git.openjdk.java.net/jdk/pull/3275

Re: RFR: 8264484: Replace uses of StringBuffer with StringBuilder in jdk.hotspot.agent

[Aleksey Shipilev]

On Tue, 30 Mar 2021 19:41:32 GMT, actuallyasmartname 
 wrote:

>> Found by IntelliJ IDEA inspection `Java | Java language level migration aids 
>> | Java 5 | 'StringBuffer' may be 'StringBuilder'`
>> As suggested in 
>> https://github.com/openjdk/jdk/pull/1507#issuecomment-757369003 I've created 
>> separate PR for module `jdk.hotspot.agent`
>> Similar cleanups in the past: 
>> https://bugs.openjdk.java.net/browse/JDK-8041679 
>> https://bugs.openjdk.java.net/browse/JDK-8264029
>
> Marked as reviewed by actuallyasmartn...@github.com (no known OpenJDK 
> username).

Created https://bugs.openjdk.java.net/browse/JDK-8264484 for it.

-

PR: https://git.openjdk.java.net/jdk/pull/3275

RFR: 8264484: Replace uses of StringBuffer with StringBuilder in jdk.hotspot.agent

[Andrey Turbanov]

Found by IntelliJ IDEA inspection `Java | Java language level migration aids | 
Java 5 | 'StringBuffer' may be 'StringBuilder'`
As suggested in https://github.com/openjdk/jdk/pull/1507#issuecomment-757369003 
I've created separate PR for module `jdk.hotspot.agent`
Similar cleanups in the past: https://bugs.openjdk.java.net/browse/JDK-8041679 
https://bugs.openjdk.java.net/browse/JDK-8264029

-

Commit messages:
 - [PATCH] Replace uses of StringBuffer with StringBuilder in jdk.hotspot.agent

Changes: https://git.openjdk.java.net/jdk/pull/3275/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3275=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264484
  Stats: 117 lines in 37 files changed: 0 ins; 5 del; 112 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3275.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3275/head:pull/3275

PR: https://git.openjdk.java.net/jdk/pull/3275

Re: RFR: 8264484: Replace uses of StringBuffer with StringBuilder in jdk.hotspot.agent

[actuallyasmartname]

On Tue, 30 Mar 2021 18:58:24 GMT, Andrey Turbanov 
 wrote:

> Found by IntelliJ IDEA inspection `Java | Java language level migration aids 
> | Java 5 | 'StringBuffer' may be 'StringBuilder'`
> As suggested in 
> https://github.com/openjdk/jdk/pull/1507#issuecomment-757369003 I've created 
> separate PR for module `jdk.hotspot.agent`
> Similar cleanups in the past: 
> https://bugs.openjdk.java.net/browse/JDK-8041679 
> https://bugs.openjdk.java.net/browse/JDK-8264029

Marked as reviewed by actuallyasmartn...@github.com (no known OpenJDK username).

-

PR: https://git.openjdk.java.net/jdk/pull/3275

Re: RFR: 8257831: Suspend with handshakes [v2]

[Richard Reingruber]

On Wed, 31 Mar 2021 05:27:05 GMT, David Holmes  wrote:

>> src/hotspot/share/runtime/handshake.cpp line 485:
>> 
>>> 483:   } else {
>>> 484: // Asynchronous may block so they may not execute 
>>> ~PreserveExceptionMark before safepointing
>>> 485: // in outer loop.
>> 
>> Sorry, I don't understand the comment.
>
> I think this relates to why the PEM was moved from the loop-scope to the sync 
> op case only. That said it isn't clear why we need the HM or PEM.

I guess it should be "... must not execute ~PreserveExceptionMark ..."
~PreserveExceptionMark calls _thread->pending_exception() which is an oop and 
that would be illegal as the vm could be at a safepoint when the async 
handshake returns.

-

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8257831: Suspend with handshakes [v2]

[Richard Reingruber]

On Wed, 31 Mar 2021 03:40:49 GMT, David Holmes  wrote:

>> Robbin Ehn has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains two commits:
>> 
>>  - Merge branch 'master' into SuspendInHandshake
>>  - 8257831: Suspend with handshake (review baseline)
>
> src/hotspot/share/prims/jvmtiEnv.cpp line 1009:
> 
>> 1007:   if (self_index >= 0) {
>> 1008: if (!JvmtiSuspendControl::suspend(current)) {
>> 1009:   results[self_index] = JVMTI_ERROR_THREAD_NOT_ALIVE;
> 
> Surely impossible when dealing with the current thread!

Another thread can win the race to suspend the current thread (if 
claim_handshake() in try_process() fails). Then JVMTI_ERROR_THREAD_SUSPENDED 
should be returned.

-

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8257831: Suspend with handshakes [v2]

[David Holmes]

On Mon, 29 Mar 2021 17:30:32 GMT, Richard Reingruber  wrote:

>> Robbin Ehn has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains two commits:
>> 
>>  - Merge branch 'master' into SuspendInHandshake
>>  - 8257831: Suspend with handshake (review baseline)
>
> src/hotspot/share/runtime/handshake.cpp line 485:
> 
>> 483:   } else {
>> 484: // Asynchronous may block so they may not execute 
>> ~PreserveExceptionMark before safepointing
>> 485: // in outer loop.
> 
> Sorry, I don't understand the comment.

I think this relates to why the PEM was moved from the loop-scope to the sync 
op case only. That said it isn't clear why we need the HM or PEM.

> src/hotspot/share/runtime/os.cpp line 874:
> 
>> 872: 
>> 873: void os::start_thread(Thread* thread) {
>> 874:   if (thread->is_Java_thread()) {
> 
> Then and else blocks seem to do the very same things.

Agreed - no need to distinguish between thread types here.

-

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8257831: Suspend with handshakes [v2]

[David Holmes]

On Fri, 26 Mar 2021 13:21:43 GMT, Robbin Ehn  wrote:

>> A suspend request is done by handshaking thread target thread(s). When 
>> executing the handshake operation we know the target mutator thread is in a 
>> dormant state (as in safepoint safe state). We have a guarantee that it will 
>> check it's poll before leaving the dormant state. To stop the thread from 
>> leaving the the dormant state we install a second asynchronous handshake to 
>> be executed by the targeted thread. The asynchronous handshake will wait on 
>> a monitor while the thread is suspended. The target thread cannot not leave 
>> the dormant state without a resume request.
>> 
>> Per thread suspend requests are naturally serialized by the per thread 
>> HandshakeState lock (we can only execute one handshake at a time per thread).
>> Instead of having a separate lock we use this to our advantage and use 
>> HandshakeState lock for serializing access to the suspend flag and for 
>> wait/notify. 
>> 
>> Suspend:
>> Requesting thread -> synchronous handshake -> target thread
>> Inside synchronus handshake (HandshakeState lock is locked while
>> executing any handshake):
>>  - Set suspended flag
>>  - Install asynchronous handshake
>> 
>> Target thread -> tries to leave dormant state -> Executes handshakes
>> Target only executes asynchronous handshake:
>>  - While suspended
>>  - Go to blocked
>>  - Wait on HandshakeState lock
>> 
>> Resume:
>> Resuming thread:
>>  - Lock HandshakeState lock
>>  - Clear suspended flag
>>  - Notify HandshakeState lock
>>  - Unlock HandshakeState lock
>> 
>> The "suspend requested" flag is an optimization, without it a dormant thread 
>> could be suspended and resumed many times and each would add a new 
>> asynchronous handshake. Suspend requested flag means there is already an 
>> asynchronous suspend handshake in queue which can be re-used, only the 
>> suspend flag needs to be set.
>> 
>> 
>> Some code can be simplified or done in a smarter way but I refrained from 
>> doing such changes instead tried to keep existing code as is as far as 
>> possible. This concerns especially raw monitors.
>> 
>> 
>> Regarding the changed test, the documentation says:
>> "If the calling thread is specified in the request_list array, this function 
>> will not return until some other thread resumes it."
>> 
>> But the code:
>>   LOG("suspendTestedThreads: before JVMTI SuspendThreadList");
>>   err = jvmti->SuspendThreadList(threads_count, threads, results);
>>   ...
>>   // Allow the Main thread to inspect the result of tested threads 
>> suspension
>>   agent_unlock(jni);
>>   
>> The thread will never return from SuspendThreadList until resumed, so it 
>> cannot unlock with agent_unlock().
>> Thus main thread is stuck forever on:
>>   // Block until the suspender thread competes the tested threads suspension 
>>   agent_lock(jni);
>> 
>> And never checks and resumes the threads. So I removed that lock instead 
>> just sleep and check until all thread have the expected suspended state.
>> 
>> 
>> 
>> This version already contains updates after pre-review comments from 
>> @dcubed-ojdk, @pchilano, @coleenp.
>> (Pre-review comments here:
>> https://github.com/openjdk/jdk/pull/2625)
>> 
>>  
>> Testing t1-t8, nsk_jdi/nsk_jvmti/jdk_jdi/tck, KS24, RunThese and
>> combinations like running with -XX:ZCollectionInterval=0.01 -
>> XX:ZFragmentationLimit=0.
>> Running above some of above concurrently (load ~240), slow debug,
>> etc...
>
> Robbin Ehn has updated the pull request with a new target base due to a merge 
> or a rebase. The pull request now contains two commits:
> 
>  - Merge branch 'master' into SuspendInHandshake
>  - 8257831: Suspend with handshake (review baseline)

Hi Robbin,

This is a great piece of work with a lot of code simplifications. Unfortunately 
some new complexities that need to be hidden by appropriate abstractions. Lots 
of comments, queries and suggestions below.

Thanks,
David

src/hotspot/share/prims/jvmtiEnv.cpp line 946:

> 944: // java_thread - pre-checked
> 945: jvmtiError
> 946: JvmtiEnv::SuspendThread(JavaThread* java_thread) {

The comment above this still states:

// Threads_lock NOT held, java_thread not protected by lock

but the java_thread is protected by a TLH so we should document that so we know 
it is always safe to refer to java_thread below.

src/hotspot/share/prims/jvmtiEnv.cpp line 1009:

> 1007:   if (self_index >= 0) {
> 1008: if (!JvmtiSuspendControl::suspend(current)) {
> 1009:   results[self_index] = JVMTI_ERROR_THREAD_NOT_ALIVE;

Surely impossible when dealing with the current thread!

src/hotspot/share/prims/jvmtiImpl.cpp line 767:

> 765: //
> 766: 
> 767: bool JvmtiSuspendControl::suspend(JavaThread *java_thread) {

Future RFE: JvmtiSuspendControl is no longer needed

src/hotspot/share/prims/jvmtiRawMonitor.cpp line 319:

> 317: jt = self->as_Java_thread();
> 318: while (true) {
> 319:  

Re: RFR: 8257831: Suspend with handshakes [v2]

[David Holmes]

On Tue, 30 Mar 2021 16:50:50 GMT, Daniel D. Daugherty  
wrote:

>> Robbin Ehn has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains two commits:
>> 
>>  - Merge branch 'master' into SuspendInHandshake
>>  - 8257831: Suspend with handshake (review baseline)
>
> src/hotspot/os/posix/signals_posix.cpp line 1587:
> 
>> 1585:   // destructor has completed.
>> 1586: 
>> 1587:   if (thread->is_Java_thread() && 
>> thread->as_Java_thread()->is_terminated()) {
> 
> We need @dholmes-ora to verify that this version of the code will
> still solve the bug he was fixing when he added old L1610.

Thanks @dcubed-ojdk - no it won't. The problem is that the signal can hit very 
late in a thread's termination process, after the JavaThread destructor has 
executed, so no query of JavaThread state is possible. So we needed something 
in the Thread state and the SR_lock was a good enough proxy for that. It may be 
possible to use a different Thread field (perhaps _ParkEvent but encapsulated 
in a Thread::has_terminated() helper method).

-

PR: https://git.openjdk.java.net/jdk/pull/3191

Re: RFR: 8264396: Use the blessed modifier order in jdk.internal.jvmstat [v2]

[Aleksey Shipilev]

On Tue, 30 Mar 2021 20:37:45 GMT, Alex Blewitt 
 wrote:

>> 8264396: Use the blessed modifier order in jdk.internal.jvmstat
>
> Alex Blewitt has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Updated copyright dates

Looks fine to me as well.

-

Marked as reviewed by shade (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3252