Re: RFR: 8275259: Add support for Java level DCmd

2021-10-14 Thread Thomas Stuefe
On Fri, 15 Oct 2021 01:54:16 GMT, Denghui Dong  wrote:

> Hi Thomas,
> 
> > Hi,
> > Interesting proposal. I have some questions.
> > 
> > * Will hanging java user code block the attach listener thread? If yes, how 
> > would you solve that?
> 
> In my current implementation, I use a simple timeout mechanism to solve this 
> problem.
> 
> Create a Thread to run the command, and use 
> `java.util.concurrent.Future#get(long, java.util.concurrent.TimeUnit)` to get 
> the result.
> 
> For more details please refer to Executor.java.
> 
> > * Will this require changes to the jcmd client, or will this work with any 
> > jcmd client, up- and downward the JDK versions? (the nice thing about jcmd 
> > is that all the logic resides at the hotspot and I can use any client to 
> > talk to any hotspot)
> 
> At present, I only extend the DCMD framework and do not modify the code of 
> jcmd. So it works with any jcmd client and jmx client, up and downward JDK 
> versions I think.
> 
> > Cheers, Thomas
> > P.S. I think it may be worthwhile to discuss this on the serviceability-dev 
> > mailing list first.
> 
> Denghui

Hi Denghui,

thanks for your answers. I'm ambivalent here. I worry about the growing 
complexity of jcmd, and about unforeseen consequences if we open this door. 
OTOH I can see this as a useful feature. I'll wait for a consensus.

Thanks, Thomas

-

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


Re: RFR: 8275259: Add support for Java level DCmd

2021-10-14 Thread Denghui Dong
On Thu, 14 Oct 2021 05:42:09 GMT, Denghui Dong  wrote:

> I proposed to extend DCmd to allow Java developers to customize their own 
> diagnostic commands last week.
> 
> At present, I have implemented a preliminary version.
> 
> In the current implementation, I provided some simple APIs in the Java layer 
> (under sun.management.cmd) for defining and registering commands.
> 
> - Executable interface
>   Java diagnostic commands need to implement this interface, the interface 
> only contains a simple method:
> 
> /**
>  * @param output the output when this executable is running
>  */
> void execute(PrintWriter output);
> 
> 
> - Add two annotations (@Command and @Parameter) to describe the command meta 
> info
> 
> - Use Factory API to register command, the following forms are supported
> 
> @Command(name = "My.Echo", description = "Echo description")
> class Echo implements Executable {
> 
> @Parameter(name = "text", ordinal=0, isMandatory = true)
> String text;
> 
> @Parameter(name = "repeat", isMandatory = true, defaultValue = "1")
> int repeat;
> 
> @Override
> public void execute(PrintWriter out) {
> for (int i = 0 ; i < repeat; i++) {
> out.println(text);
> }
> }
> }
> 
> Factory.register(Echo.class);
> 
> 
> 
> Factory.register("My.Date", output -> {
> output.println(new Date());
> });
> 
> 
> - When the command is running, the VM will call `Executor.executeCommand` to 
> execute the command. In the implementation of Executor, I introduced a simple 
> timeout mechanism to prevent the command channel from being blocked.
> 
> At the VM layer, I extended the existing DCmd framework(implemented JavaDCmd 
> and JavaDCmdFactoryImpl) to be compatible with existing functions (jmx, help, 
> etc.).
> 
> In terms of security, considering that the SecurityManager will be 
> deprecated, there are not too many considerations for the time being.
> 
> Any input is appreciated.
> 
> Thanks,
> Denghui

Hi Thomas,

> Hi,
> 
> Interesting proposal. I have some questions.
> 
> * Will hanging java user code block the attach listener thread? If yes, how 
> would you solve that?

In my current implementation, I use a simple timeout mechanism to solve this 
problem.

Create a Thread to run the command, and use 
`java.util.concurrent.Future#get(long, java.util.concurrent.TimeUnit)` to get 
the result.

For more details please refer to Executor.java.


> * Will this require changes to the jcmd client, or will this work with any 
> jcmd client, up- and downward the JDK versions? (the nice thing about jcmd is 
> that all the logic resides at the hotspot and I can use any client to talk to 
> any hotspot)
> 

At present, I only extend the DCMD framework and do not modify the code of 
jcmd. So it works with any jcmd client and jmx client, up and downward JDK 
versions I think.

> Cheers, Thomas
> 
> P.S. I think it may be worthwhile to discuss this on the serviceability-dev 
> mailing list first.

Denghui

Hi Erik,
> To understand what is needed (namespacing, error handling, safety, lifecycle 
> etc.) and if this is actually useful for Java, I think you need to write 2-3 
> commands for a few popular frameworks, for example Spring.
> 
> If we can't come up with 5-10 commands where this functionality would solve 
> real problems users face, it's unlikely somebody will use it. It will just be 
> a maintenance burden in the JDK. Once an API is added, it's hard to remove.
> 
> The scope of this seem to be JEP level.
> 
> (JFR is not a valid use case, since we don't want to add a dependency on 
> annotation classes in the jdk.jcmd module).

Thanks for the comments.

I will investigate whether there is a scenario to apply this extension in the 
current mainstream framework.

Denghui

-

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


Integrated: 8275240: Change nested classes in jdk.attach to static nested classes

2021-10-14 Thread Andrey Turbanov
On Tue, 12 Oct 2021 07:20:14 GMT, Andrey Turbanov  wrote:

> Non-static classes hold a link to their parent classes, which in many cases 
> can be avoided.
> Similar cleanup in java.base - 
> [JDK-8261880](https://bugs.openjdk.java.net/browse/JDK-8261880)

This pull request has now been integrated.

Changeset: 21df412b
Author:Andrey Turbanov 
Committer: Serguei Spitsyn 
URL:   
https://git.openjdk.java.net/jdk/commit/21df412bd9a02f0c3f351467951415141d920e03
Stats: 7 lines in 4 files changed: 0 ins; 2 del; 5 mod

8275240: Change nested classes in jdk.attach to static nested classes

Reviewed-by: cjplummer, dholmes, sspitsyn

-

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


Re: RFR: 8266936: Add a finalization JFR event [v16]

2021-10-14 Thread Markus Grönlund
On Thu, 14 Oct 2021 21:46:36 GMT, Coleen Phillimore  wrote:

>> Markus Grönlund has refreshed the contents of this pull request, and 
>> previous commits have been removed. The incremental views will show 
>> differences compared to the previous content of the PR.
>
> src/hotspot/share/jfr/support/jfrSymbolTable.hpp line 68:
> 
>> 66:   template  class, 
>> typename, size_t>
>> 67:   friend class HashTableHost;
>> 68:   typedef HashTableHost> JfrSymbolTable> SymbolTable;
> 
> Oh here it is.  Since it's an enclosed type, can you rename it something 
> simpler like Symbols and the other Strings?

Maybe :)

-

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


Re: RFR: 8266936: Add a finalization JFR event [v10]

2021-10-14 Thread Markus Grönlund
On Thu, 14 Oct 2021 22:02:43 GMT, Coleen Phillimore  wrote:

>> Ok, I see - grow the table.
>> 
>> I'm not sure if you need to ask ik->is_loader_alive() or not, but I think 
>> you do.  The InstanceKlass is removed from your table during class unloading 
>> but before that during concurrent class unloading, the class might not be 
>> alive while you look at it and concurrent class unloading hasn't gotten 
>> around to removing it yet.  Since you save classes regardless of CLD, you 
>> have to check if it's alive.  See classLoaderDataGraph.cpp 
>> ClassLoaderDataGraphIterator for example.  The CLDG_lock only keeps the 
>> graph from not getting modified, but the classes in it might be dead.
>
> That said, I don't see where you return an InstanceKlass in the table, which 
> would need this check.  Not in class_unloading_do because this follows the 
> _unloading list.

So there is already support for concurrent class unloading today in JFR, and 
the protocol is built around the CLDG_lock. If JFR holds it, concurrent class 
unloading cannot run. If GC holds it, JFR cannot inspect classes. That's why 
the table inspection is guarded via the CLDG_lock, for mutual exclusion to 
avoid this problem. I.e. if concurrent class unloading is in progress, JFR will 
not inspect the table.

-

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


Re: RFR: 8266936: Add a finalization JFR event [v10]

2021-10-14 Thread Coleen Phillimore
On Thu, 14 Oct 2021 21:58:42 GMT, Coleen Phillimore  wrote:

>> "Since you remove entries on unloading, I don't see any reason to have any 
>> concurrent cleanup."
>> Thank you, that is true. The only concurrent work required will be to grow 
>> the table.
>> 
>> "You do however need in the lookup code, some code that doesn't find the 
>> InstanceKlass if !ik->is_loader_alive()"
>> 
>> A precondition is that the code doing the lookup hold the 
>> ClassLoaderDataGraph_lock or is at a safepoint. Is that still the case? 
>> Would not purge_unloaded() take out the InstanceKlass from the table, as 
>> part of unloading, before !ik->is_loader_alive() is published to the outside 
>> world?
>
> Ok, I see - grow the table.
> 
> I'm not sure if you need to ask ik->is_loader_alive() or not, but I think you 
> do.  The InstanceKlass is removed from your table during class unloading but 
> before that during concurrent class unloading, the class might not be alive 
> while you look at it and concurrent class unloading hasn't gotten around to 
> removing it yet.  Since you save classes regardless of CLD, you have to check 
> if it's alive.  See classLoaderDataGraph.cpp ClassLoaderDataGraphIterator for 
> example.  The CLDG_lock only keeps the graph from not getting modified, but 
> the classes in it might be dead.

That said, I don't see where you return an InstanceKlass in the table, which 
would need this check.  Not in class_unloading_do because this follows the 
_unloading list.

-

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


Re: RFR: 8266936: Add a finalization JFR event [v10]

2021-10-14 Thread Coleen Phillimore
On Mon, 13 Sep 2021 10:54:18 GMT, Markus Grönlund  wrote:

>> src/hotspot/share/services/finalizerService.cpp line 44:
>> 
>>> 42: _ik(ik),
>>> 43: _objects_on_heap(0),
>>> 44: _total_finalizers_run(0) {}
>> 
>> Is this hashtable for every InstanceKlass that defines a finalizer?  How 
>> many are there generally?  Why couldn't this be a simple hashtable like 
>> ResourceHashtable (soon to be renamed) which you can write in only a few 
>> lines of code?
>
> This hashtable holds a FinalizerEntry for every InstanceKlass that provides a 
> non-empty finalizer method and have allocated at least one object. How many 
> there are in general depends on the application. A ResourceHashtable does not 
> have the concurrency property required, as lookups and inserts will happen as 
> part of object allocation.

ok.

>> src/hotspot/share/services/finalizerService.cpp line 331:
>> 
>>> 329:   }
>>> 330:   Thread* const thread = Thread::current();
>>> 331:   // We use current size
>> 
>> Since you remove entries on unloading, I don't see any reason to have any 
>> concurrent cleanup.
>> You do however need in the lookup code, some code that doesn't find the 
>> InstanceKlass if !ik->is_loader_alive()
>
> "Since you remove entries on unloading, I don't see any reason to have any 
> concurrent cleanup."
> Thank you, that is true. The only concurrent work required will be to grow 
> the table.
> 
> "You do however need in the lookup code, some code that doesn't find the 
> InstanceKlass if !ik->is_loader_alive()"
> 
> A precondition is that the code doing the lookup hold the 
> ClassLoaderDataGraph_lock or is at a safepoint. Is that still the case? Would 
> not purge_unloaded() take out the InstanceKlass from the table, as part of 
> unloading, before !ik->is_loader_alive() is published to the outside world?

Ok, I see - grow the table.

I'm not sure if you need to ask ik->is_loader_alive() or not, but I think you 
do.  The InstanceKlass is removed from your table during class unloading but 
before that during concurrent class unloading, the class might not be alive 
while you look at it and concurrent class unloading hasn't gotten around to 
removing it yet.  Since you save classes regardless of CLD, you have to check 
if it's alive.  See classLoaderDataGraph.cpp ClassLoaderDataGraphIterator for 
example.  The CLDG_lock only keeps the graph from not getting modified, but the 
classes in it might be dead.

-

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


Re: RFR: 8266936: Add a finalization JFR event [v16]

2021-10-14 Thread Coleen Phillimore
On Wed, 13 Oct 2021 18:03:25 GMT, Markus Grönlund  wrote:

>> Greetings,
>> 
>> Object.finalize() was deprecated in JDK9. There is an ongoing effort to 
>> replace and mitigate Object.finalize() uses in the JDK libraries; please see 
>> https://bugs.openjdk.java.net/browse/JDK-8253568 for more information. 
>> 
>> We also like to assist users in replacing and mitigating uses in non-JDK 
>> code.
>> 
>> Hence, this changeset adds a periodic JFR event to help identify which 
>> classes are overriding Object.finalize().
>> 
>> Thanks
>> Markus
>
> Markus Grönlund has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR.

src/hotspot/share/jfr/support/jfrSymbolTable.cpp line 75:

> 73: 
> 74: JfrSymbolTable::JfrSymbolTable() :
> 75:   _symbol_table(new SymbolTable(this)),

I'm confused about which symbol table this is.  Is this the same SymbolTable as 
classfile/symbolTable.cpp? that instance is assumed to be a singleton.  Is this 
a different SymbolTable and can it have a different name (thought it was 
JfrSymbolTable).

src/hotspot/share/jfr/support/jfrSymbolTable.hpp line 68:

> 66:   template  class, 
> typename, size_t>
> 67:   friend class HashTableHost;
> 68:   typedef HashTableHost JfrSymbolTable> SymbolTable;

Oh here it is.  Since it's an enclosed type, can you rename it something 
simpler like Symbols and the other Strings?

-

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


Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]

2021-10-14 Thread Chris Plummer
On Thu, 14 Oct 2021 07:44:36 GMT, Richard Reingruber  wrote:

>> Ok, so you need to hold threadLock from the time `blockOnDebuggerSuspend()` 
>> is done waiting on it until after `trackAppResume()` is done, and since 
>> `trackAppResume()` needs to grab the handlerLock, and you need to grab the 
>> handerLock before the threadLock, you need to jump through some lock 
>> grabbing/release hoops. However, you are in fact releasing the threadLock 
>> for a short time in `blockOnDebuggerSuspend()` after the wait completes. 
>> Doesn't this expose the resumee to potential suspending after the wait has 
>> completed and before `trackAppResume()` has been called?
>
>> Ok, so you need to hold threadLock from the time `blockOnDebuggerSuspend()` 
>> is done waiting on it until after `trackAppResume()` is done, and since 
>> `trackAppResume()` needs to grab the handlerLock, and you need to grab the 
>> handerLock before the threadLock, you need to jump through some lock 
>> grabbing/release hoops.
> 
> That's correct.
> 
>> However, you are in fact releasing the threadLock for a short time in 
>> `blockOnDebuggerSuspend()` after the wait completes. Doesn't this expose the 
>> resumee to potential suspending after the wait has completed and before 
>> `trackAppResume()` has been called?
> 
> That's correct too. I wouldn't see an issue with it though. I think this is 
> even a preexisting condition. The current thread can lose the race grabbing 
> threadLock after it was notified to the debugger trying to suspend again (if 
> there wasn't the deadlock of course) and consequently suspendCount can 
> (again) be greater than 0 right after the wait. In that case we simply retry.

Ok. So you just need to reacquire the threadLock before the `findThread()` call 
and before exiting the while loop, and hold it until after the 
`trackAppResume()` call. I guess it ok then. But this exiting of the 
handlerLock here and in `blockOnDebuggerSuspend()` is always going to arouse 
suspicion for anyone that reads the code. The first question will be if the 
lock does not need to be held here, why grab it in the first place? We know the 
answer is this lock ordering issue that turns up when `trackAppResume()` is 
later called, but this will be far from obvious to the reader. It might be 
possible to make this a bit clearer with some code restructuring, like maybe 
combining `blockOnDebuggerSuspend()` and `trackAppResume()` into one function 
(I'm not sure that this will actually help, but maybe something to consider), 
but at the very least we need some comments calling out why handlerLock is held 
in the first place and why it is ok to release it.

-

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


Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes

2021-10-14 Thread Daniel D . Daugherty
On Sun, 4 Jul 2021 23:39:00 GMT, David Holmes  wrote:

>> A fix to reduce ThreadsListHandle overhead in relation to handshakes and
>> we add sanity checks for ThreadsListHandles higher in the call stack.
>> 
>> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running.
>
> Hi Dan,
> 
> I just updated the bug report. This really isn't addressing the reasons the 
> RFE was filed.
> 
> David

@dholmes-ora, @coleenp and @sspitsyn - I've finished reworking the patch
and I'm finally happy with how it works. This version has passed Mach5 Tier[1-7]
and has mostly finished Mach5 Tier8 (it's in the 24-hour test phase)...

-

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


Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]

2021-10-14 Thread Daniel D . Daugherty
> A fix to reduce ThreadsListHandle overhead in relation to handshakes and
> we add sanity checks for ThreadsListHandles higher in the call stack.
> 
> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running.

Daniel D. Daugherty has updated the pull request incrementally with one 
additional commit since the last revision:

  8249004.cr1.patch

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4677/files
  - new: https://git.openjdk.java.net/jdk/pull/4677/files/a214b284..4e207e13

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

  Stats: 83 lines in 5 files changed: 65 ins; 2 del; 16 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4677.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4677/head:pull/4677

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


Re: RFR: 8275240: Change nested classes in jdk.attach to static nested classes [v2]

2021-10-14 Thread Serguei Spitsyn
On Thu, 14 Oct 2021 06:59:12 GMT, Andrey Turbanov  wrote:

>> Non-static classes hold a link to their parent classes, which in many cases 
>> can be avoided.
>> Similar cleanup in java.base - 
>> [JDK-8261880](https://bugs.openjdk.java.net/browse/JDK-8261880)
>
> Andrey Turbanov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8275240: Change nested classes in jdk.attach to static nested classes
>   update copyright year

Marked as reviewed by sspitsyn (Reviewer).

-

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


Re: RFR: 8275035: Clean up worker thread infrastructure [v2]

2021-10-14 Thread Per Liden
On Thu, 14 Oct 2021 09:03:18 GMT, Per Liden  wrote:

>> I propose that we clean up our GangWorker/WorkGang and related classes, to 
>> remove abstractions we no longer need (after CMS was removed, 
>> MutexDispatcher was removed, Parallel is now using WorkGang, etc) and 
>> adjusting names as follows:
>> 
>> * Rename AbstractGangTask to WorkerTask
>> * Rename WorkGang to WorkerThreads
>> * Fold GangWorker into WorkerThread
>> * Fold WorkManager into WorkerThreads
>> * Move SubTaskDone and friends to a new workerUtils.hpp/cpp
>> 
>> I've split things up into several commits to make it easier to review.
>> 
>> Testing: Passes Tier 1-3 on all Oracle platforms.
>
> Per Liden has updated the pull request with a new target base due to a merge 
> or a rebase. The pull request now contains 21 commits:
> 
>  - Merge master
>  - Clean up naming in ReferenceProcessor
>  - Clean up naming in Shenandoah
>  - Clean up naming in HeapDumper/HeapInspector
>  - Clean up naming in G1
>  - Clean up naming in pretouch
>  - Clean up naming in WeakProcessor
>  - Clean up naming in ZGC
>  - Remove unused log tag
>  - Rename workgroup.hpp/cpp to workerThread.hpp/cpp
>  - ... and 11 more: 
> https://git.openjdk.java.net/jdk/compare/8b1b6f9f...9a89f785

Thanks all for reviewing!

-

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


Integrated: 8275035: Clean up worker thread infrastructure

2021-10-14 Thread Per Liden
On Mon, 11 Oct 2021 09:21:21 GMT, Per Liden  wrote:

> I propose that we clean up our GangWorker/WorkGang and related classes, to 
> remove abstractions we no longer need (after CMS was removed, MutexDispatcher 
> was removed, Parallel is now using WorkGang, etc) and adjusting names as 
> follows:
> 
> * Rename AbstractGangTask to WorkerTask
> * Rename WorkGang to WorkerThreads
> * Fold GangWorker into WorkerThread
> * Fold WorkManager into WorkerThreads
> * Move SubTaskDone and friends to a new workerUtils.hpp/cpp
> 
> I've split things up into several commits to make it easier to review.
> 
> Testing: Passes Tier 1-3 on all Oracle platforms.

This pull request has now been integrated.

Changeset: 54b88707
Author:Per Liden 
URL:   
https://git.openjdk.java.net/jdk/commit/54b887076612c0eaa410a849178f8ba0c4ed3eeb
Stats: 2415 lines in 105 files changed: 881 ins; 1083 del; 451 mod

8275035: Clean up worker thread infrastructure

Reviewed-by: stefank, ayang

-

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


Re: RFR: 8275259: Add support for Java level DCmd

2021-10-14 Thread Erik Gahlin
On Thu, 14 Oct 2021 05:42:09 GMT, Denghui Dong  wrote:

> I proposed to extend DCmd to allow Java developers to customize their own 
> diagnostic commands last week.
> 
> At present, I have implemented a preliminary version.
> 
> In the current implementation, I provided some simple APIs in the Java layer 
> (under sun.management.cmd) for defining and registering commands.
> 
> - Executable interface
>   Java diagnostic commands need to implement this interface, the interface 
> only contains a simple method:
> 
> /**
>  * @param output the output when this executable is running
>  */
> void execute(PrintWriter output);
> 
> 
> - Add two annotations (@Command and @Parameter) to describe the command meta 
> info
> 
> - Use Factory API to register command, the following forms are supported
> 
> @Command(name = "My.Echo", description = "Echo description")
> class Echo implements Executable {
> 
> @Parameter(name = "text", ordinal=0, isMandatory = true)
> String text;
> 
> @Parameter(name = "repeat", isMandatory = true, defaultValue = "1")
> int repeat;
> 
> @Override
> public void execute(PrintWriter out) {
> for (int i = 0 ; i < repeat; i++) {
> out.println(text);
> }
> }
> }
> 
> Factory.register(Echo.class);
> 
> 
> 
> Factory.register("My.Date", output -> {
> output.println(new Date());
> });
> 
> 
> - When the command is running, the VM will call `Executor.executeCommand` to 
> execute the command. In the implementation of Executor, I introduced a simple 
> timeout mechanism to prevent the command channel from being blocked.
> 
> At the VM layer, I extended the existing DCmd framework(implemented JavaDCmd 
> and JavaDCmdFactoryImpl) to be compatible with existing functions (jmx, help, 
> etc.).
> 
> In terms of security, considering that the SecurityManager will be 
> deprecated, there are not too many considerations for the time being.
> 
> Any input is appreciated.
> 
> Thanks,
> Denghui

To understand what is needed (namespacing, error handling, safety, lifecycle 
etc.) and if this is actually useful for Java, I think you need to write 2-3 
commands for a few popular frameworks, for example Spring.

If we can't come up with 5-10 commands where this functionality would solve 
real problems users face, it's unlikely somebody will use it. It will just be a 
maintenance burden in the JDK. Once an API is added, it's hard to remove.

The scope of this seem to be JEP level.

(JFR is not a valid use case, since we don't want to add a dependency on 
annotation classes in the jdk.jcmd module).

-

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


Re: RFR: 8275259: Add support for Java level DCmd

2021-10-14 Thread Thomas Stuefe
On Thu, 14 Oct 2021 05:42:09 GMT, Denghui Dong  wrote:

> I proposed to extend DCmd to allow Java developers to customize their own 
> diagnostic commands last week.
> 
> At present, I have implemented a preliminary version.
> 
> In the current implementation, I provided some simple APIs in the Java layer 
> (under sun.management.cmd) for defining and registering commands.
> 
> - Executable interface
>   Java diagnostic commands need to implement this interface, the interface 
> only contains a simple method:
> 
> /**
>  * @param output the output when this executable is running
>  */
> void execute(PrintWriter output);
> 
> 
> - Add two annotations (@Command and @Parameter) to describe the command meta 
> info
> 
> - Use Factory API to register command, the following forms are supported
> 
> @Command(name = "My.Echo", description = "Echo description")
> class Echo implements Executable {
> 
> @Parameter(name = "text", ordinal=0, isMandatory = true)
> String text;
> 
> @Parameter(name = "repeat", isMandatory = true, defaultValue = "1")
> int repeat;
> 
> @Override
> public void execute(PrintWriter out) {
> for (int i = 0 ; i < repeat; i++) {
> out.println(text);
> }
> }
> }
> 
> Factory.register(Echo.class);
> 
> 
> 
> Factory.register("My.Date", output -> {
> output.println(new Date());
> });
> 
> 
> - When the command is running, the VM will call `Executor.executeCommand` to 
> execute the command. In the implementation of Executor, I introduced a simple 
> timeout mechanism to prevent the command channel from being blocked.
> 
> At the VM layer, I extended the existing DCmd framework(implemented JavaDCmd 
> and JavaDCmdFactoryImpl) to be compatible with existing functions (jmx, help, 
> etc.).
> 
> In terms of security, considering that the SecurityManager will be 
> deprecated, there are not too many considerations for the time being.
> 
> Any input is appreciated.
> 
> Thanks,
> Denghui

Hi,

Interesting proposal. I have some questions.

- Will hanging java user code block the attach listener thread? If yes, how 
would you solve that?
- Will this require changes to the jcmd client, or will this work with any jcmd 
client, up- and downward the JDK versions? (the nice thing about jcmd is that 
all the logic resides at the hotspot and I can use any client to talk to any 
hotspot)

Cheers, Thomas

P.S. I think it may be worthwhile to discuss this on the serviceability-dev 
mailing list first.

-

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


Re: RFR: 8275035: Clean up worker thread infrastructure [v2]

2021-10-14 Thread Per Liden
> I propose that we clean up our GangWorker/WorkGang and related classes, to 
> remove abstractions we no longer need (after CMS was removed, MutexDispatcher 
> was removed, Parallel is now using WorkGang, etc) and adjusting names as 
> follows:
> 
> * Rename AbstractGangTask to WorkerTask
> * Rename WorkGang to WorkerThreads
> * Fold GangWorker into WorkerThread
> * Fold WorkManager into WorkerThreads
> * Move SubTaskDone and friends to a new workerUtils.hpp/cpp
> 
> I've split things up into several commits to make it easier to review.
> 
> Testing: Passes Tier 1-3 on all Oracle platforms.

Per Liden has updated the pull request with a new target base due to a merge or 
a rebase. The pull request now contains 21 commits:

 - Merge master
 - Clean up naming in ReferenceProcessor
 - Clean up naming in Shenandoah
 - Clean up naming in HeapDumper/HeapInspector
 - Clean up naming in G1
 - Clean up naming in pretouch
 - Clean up naming in WeakProcessor
 - Clean up naming in ZGC
 - Remove unused log tag
 - Rename workgroup.hpp/cpp to workerThread.hpp/cpp
 - ... and 11 more: https://git.openjdk.java.net/jdk/compare/8b1b6f9f...9a89f785

-

Changes: https://git.openjdk.java.net/jdk/pull/5886/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5886=01
  Stats: 2415 lines in 105 files changed: 881 ins; 1083 del; 451 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5886.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5886/head:pull/5886

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


Re: RFR: 8275259: Add support for Java level DCmd

2021-10-14 Thread Denghui Dong
On Thu, 14 Oct 2021 07:40:20 GMT, David Holmes  wrote:

> Ah right - so you're really looking at extending the capabilities of the
> DiagnosticCommandMBean to add a way to register a Java diagnostic command.

Yes, but we could not add registration API to DiagnosticCommandMBean directly.

There are already some VM diagnostic commands that depend on Java methods 
directly, this extension could also bring benefit to this scenario I think.

Hope folks from serviceability could take a look at it:)

Denghui

-

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


Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]

2021-10-14 Thread Richard Reingruber
On Wed, 13 Oct 2021 20:59:44 GMT, Chris Plummer  wrote:

> Ok, so you need to hold threadLock from the time `blockOnDebuggerSuspend()` 
> is done waiting on it until after `trackAppResume()` is done, and since 
> `trackAppResume()` needs to grab the handlerLock, and you need to grab the 
> handerLock before the threadLock, you need to jump through some lock 
> grabbing/release hoops.

That's correct.

> However, you are in fact releasing the threadLock for a short time in 
> `blockOnDebuggerSuspend()` after the wait completes. Doesn't this expose the 
> resumee to potential suspending after the wait has completed and before 
> `trackAppResume()` has been called?

That's correct too. I wouldn't see an issue with it though. I think this is 
even a preexisting condition. The current thread can lose the race grabbing 
threadLock after it was notified to the debugger trying to suspend again (if 
there wasn't the deadlock of course) and consequently suspendCount can (again) 
be greater than 0 right after the wait. In that case we simply retry.

-

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


Re: RFR: 8275259: Add support for Java level DCmd

2021-10-14 Thread David Holmes

On 14/10/2021 5:14 pm, Denghui Dong wrote:

On Thu, 14 Oct 2021 06:43:20 GMT, David Holmes  wrote:


I'm not sure exactly where this API would need to go. IIUC jcmd doesn't

exist at the Java level it is just a tool, so introducing an API to
interact with it seems problematic to me.

IMO, `jcmd` is a client of `dcmd`, the user also can use JMX to execute `dcmd`, 
this means there already are Java APIs interact with `dcmd`.


Ah right - so you're really looking at extending the capabilities of the 
DiagnosticCommandMBean to add a way to register a Java diagnostic command.


David


Denghui

-

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



Re: RFR: 8275259: Add support for Java level DCmd

2021-10-14 Thread Denghui Dong
On Thu, 14 Oct 2021 06:43:20 GMT, David Holmes  wrote:

> I'm not sure exactly where this API would need to go. IIUC jcmd doesn't
exist at the Java level it is just a tool, so introducing an API to
interact with it seems problematic to me. 

IMO, `jcmd` is a client of `dcmd`, the user also can use JMX to execute `dcmd`, 
this means there already are Java APIs interact with `dcmd`.

Denghui

-

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


Re: RFR: 8275240: Change nested classes in jdk.attach to static nested classes [v2]

2021-10-14 Thread Andrey Turbanov
> Non-static classes hold a link to their parent classes, which in many cases 
> can be avoided.
> Similar cleanup in java.base - 
> [JDK-8261880](https://bugs.openjdk.java.net/browse/JDK-8261880)

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

  8275240: Change nested classes in jdk.attach to static nested classes
  update copyright year

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5904/files
  - new: https://git.openjdk.java.net/jdk/pull/5904/files/70cb33a9..06fcaa48

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

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

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


Re: RFR: 8275259: Add support for Java level DCmd

2021-10-14 Thread David Holmes

On 14/10/2021 4:32 pm, Denghui Dong wrote:

On Thu, 14 Oct 2021 06:17:13 GMT, David Holmes  wrote:


In the current implementation, I provided some simple APIs in the Java layer 
(under sun.management.cmd)


An API that is expected to be used by application code can't go in the 
unsupported/internal-use-only sun.management namespace. This would have to be a 
new external facing supported API.

Cheers, David


Thanks.
  I consider putting the API into `javax.management.dcmd`. What do you think?


There are very specific rules related to the use of the javax namespace.

I'm not sure exactly where this API would need to go. IIUC jcmd doesn't 
exist at the Java level it is just a tool, so introducing an API to 
interact with it seems problematic to me. Other folk in serviceability 
(both VM and JDK sides) will need to weigh in here.


David


-

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



Re: RFR: 8275259: Add support for Java level DCmd

2021-10-14 Thread Denghui Dong
On Thu, 14 Oct 2021 06:17:13 GMT, David Holmes  wrote:

> > In the current implementation, I provided some simple APIs in the Java 
> > layer (under sun.management.cmd)
> 
> An API that is expected to be used by application code can't go in the 
> unsupported/internal-use-only sun.management namespace. This would have to be 
> a new external facing supported API.
> 
> Cheers, David

Thanks.
 I consider putting the API into `javax.management.dcmd`. What do you think?

-

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


Re: RFR: 8275259: Add support for Java level DCmd

2021-10-14 Thread David Holmes
On Thu, 14 Oct 2021 05:42:09 GMT, Denghui Dong  wrote:

> In the current implementation, I provided some simple APIs in the Java layer 
> (under sun.management.cmd)

An API that is expected to be used by application code can't go in the 
unsupported/internal-use-only sun.management namespace. This would have to be a 
new external facing supported API.

Cheers,
David

-

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