Re: RFR: 8274620: resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java is timing out
On Fri, 8 Oct 2021 21:05:16 GMT, Chris Plummer wrote: >> This PR fix the issue of JDK-8274620 >> (resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java is timing out) >> The root cause is that `BufferedOutputStream` should be used when writting >> data to file. > > Marked as reviewed by cjplummer (Reviewer). > Dear @plummercj, Thanks a lot for your quick review, since this is a trivial > patch, do you think it needs one more approval before push？ I'd prefer that it got a second review. - PR: https://git.openjdk.java.net/jdk/pull/5860
Re: RFR: 8274620: resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java is timing out
On Fri, 8 Oct 2021 21:05:16 GMT, Chris Plummer wrote: >> This PR fix the issue of JDK-8274620 >> (resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java is timing out) >> The root cause is that `BufferedOutputStream` should be used when writting >> data to file. > > Marked as reviewed by cjplummer (Reviewer). Dear @plummercj, Thanks a lot for your quick review, since this is a trivial patch, do you think it needs one more approval before push？ BRs, Lin - PR: https://git.openjdk.java.net/jdk/pull/5860
Re: RFR: 8274620: resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java is timing out
On Fri, 8 Oct 2021 06:00:03 GMT, Lin Zang wrote: > This PR fix the issue of JDK-8274620 > (resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java is timing out) > The root cause is that `BufferedOutputStream` should be used when writting > data to file. Marked as reviewed by cjplummer (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5860
Re: RFR: 8274898: Cleanup usages of StringBuffer in jdk tools modules
On Thu, 9 Sep 2021 06:53:13 GMT, Andrey Turbanov
wrote:
> StringBuffer is a legacy synchronized class. StringBuilder is a direct
> replacement to StringBuffer which generally have better performance
src/jdk.jpackage/share/classes/jdk/jpackage/internal/Log.java line 117:
> 115:
> 116: for (String s : strings) {
> 117: sb.append(" " + s);
Shouldn't it be `sb.append(" ").append(s)`?
-
PR: https://git.openjdk.java.net/jdk/pull/5433
Re: RFR: 8274004: Change 'nonleaf' rank name [v2]
On Fri, 8 Oct 2021 00:48:05 GMT, David Holmes wrote: >> Coleen Phillimore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix overlap error message printing and add a test. > > test/hotspot/gtest/runtime/test_mutex.cpp line 292: > >> 290: Monitor* monitor_rank_broken = new Monitor(Mutex::oopstorage-4, >> "monitor_rank_broken"); >> 291: monitor_rank_broken->lock_without_safepoint_check(); >> 292: monitor_rank_broken->unlock(); > > This is dead code right - the assertion failure will stop us getting here. Yes, it is dead code. I'll remove it and retest to make sure nothing surprising happens. > test/hotspot/gtest/runtime/test_mutex.cpp line 302: > >> 300: Monitor* monitor_rank_broken = new Monitor(Mutex::safepoint-40, >> "monitor_rank_broken"); >> 301: monitor_rank_broken->lock_without_safepoint_check(); >> 302: monitor_rank_broken->unlock(); > > Ditto - dead code removed. > test/hotspot/gtest/runtime/test_mutex.cpp line 310: > >> 308: ThreadInVMfromNative invm(THREAD); >> 309: >> 310: Monitor* monitor_rank_broken = new Monitor(Mutex::safepoint-1, >> "monitor_rank_broken"); > > This rank is not actually broken is it - otherwise we won't get to the next > line. right. It's not broken. I'll rename it to 'monitor_rank_ok'. > test/hotspot/gtest/runtime/test_mutex.cpp line 313: > >> 311: Monitor* monitor_rank_also_broken = new >> Monitor(monitor_rank_broken->rank()-39, "monitor_rank_also_broken"); >> 312: monitor_rank_also_broken->lock_without_safepoint_check(); >> 313: monitor_rank_also_broken->unlock(); > > Dead code removed. - PR: https://git.openjdk.java.net/jdk/pull/5845
Re: RFR: 8274004: Change 'nonleaf' rank name [v2]
On Thu, 7 Oct 2021 15:28:31 GMT, Coleen Phillimore wrote: >> Also fixes: 8273956: Add checking for rank values >> >> This change does 3 things. I could separate them but this has all been >> tested together and most of the change is mechanical. The first is a simple >> rename of nonleaf => safepoint. The second change is to add the enum class >> Rank which only allows subtraction from a base rank, and some additional >> type checking so that rank arithmetic doesn't overlap with a different rank. >> Ie if you say nosafepoint-n, that doesn't overlap with oopstorage. The >> third change is to remove the _safepoint_check_always/never flag. That is >> now intrinsic in the ranking, as all nosafepoint ranks are lower than all >> safepoint ranks. >> >> Future changes could add rank names in the middle of nosafepoint and >> safepoint, like handshake. As of now, the rank subtractions are not >> unmanageable. There are actually not many nested Mutex. >> >> This is the last of the planned subtasks for Mutex ranking cleanup. If you >> have other ideas or suggestions, please let me know. >> >> Tested with tier1-8 at one point in time and just retested with tier1-6. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Fix overlap error message printing and add a test. I removed the dead test code and retested. Thanks for the code review David, and all the discussions leading to this change. - PR: https://git.openjdk.java.net/jdk/pull/5845
Integrated: 8274004: Change 'nonleaf' rank name
On Wed, 6 Oct 2021 23:27:17 GMT, Coleen Phillimore wrote: > Also fixes: 8273956: Add checking for rank values > > This change does 3 things. I could separate them but this has all been > tested together and most of the change is mechanical. The first is a simple > rename of nonleaf => safepoint. The second change is to add the enum class > Rank which only allows subtraction from a base rank, and some additional type > checking so that rank arithmetic doesn't overlap with a different rank. Ie > if you say nosafepoint-n, that doesn't overlap with oopstorage. The third > change is to remove the _safepoint_check_always/never flag. That is now > intrinsic in the ranking, as all nosafepoint ranks are lower than all > safepoint ranks. > > Future changes could add rank names in the middle of nosafepoint and > safepoint, like handshake. As of now, the rank subtractions are not > unmanageable. There are actually not many nested Mutex. > > This is the last of the planned subtasks for Mutex ranking cleanup. If you > have other ideas or suggestions, please let me know. > > Tested with tier1-8 at one point in time and just retested with tier1-6. This pull request has now been integrated. Changeset: 6364719c Author:Coleen Phillimore URL: https://git.openjdk.java.net/jdk/commit/6364719cd1c57220769ea580d958da8dc2fdf7f9 Stats: 452 lines in 41 files changed: 105 ins; 117 del; 230 mod 8274004: Change 'nonleaf' rank name 8273956: Add checking for rank values Reviewed-by: dholmes, pchilanomate - PR: https://git.openjdk.java.net/jdk/pull/5845
Re: RFR: 8274004: Change 'nonleaf' rank name [v3]
> Also fixes: 8273956: Add checking for rank values > > This change does 3 things. I could separate them but this has all been > tested together and most of the change is mechanical. The first is a simple > rename of nonleaf => safepoint. The second change is to add the enum class > Rank which only allows subtraction from a base rank, and some additional type > checking so that rank arithmetic doesn't overlap with a different rank. Ie > if you say nosafepoint-n, that doesn't overlap with oopstorage. The third > change is to remove the _safepoint_check_always/never flag. That is now > intrinsic in the ranking, as all nosafepoint ranks are lower than all > safepoint ranks. > > Future changes could add rank names in the middle of nosafepoint and > safepoint, like handshake. As of now, the rank subtractions are not > unmanageable. There are actually not many nested Mutex. > > This is the last of the planned subtasks for Mutex ranking cleanup. If you > have other ideas or suggestions, please let me know. > > Tested with tier1-8 at one point in time and just retested with tier1-6. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Remove dead code in test. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5845/files - new: https://git.openjdk.java.net/jdk/pull/5845/files/59bfd945..e8df15de Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5845=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5845=01-02 Stats: 8 lines in 1 file changed: 0 ins; 6 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5845.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5845/head:pull/5845 PR: https://git.openjdk.java.net/jdk/pull/5845
Re: Extend jcmd to java application level
To make an application command usable, it must provide metadata (name and
description of the command, its options’ data types, units, default values, if
it is mandatory etc.) and error handling. This will make the API surface larger
and trickier to get right.
(Not a full overlap, but we are thinking of adding more interactive
capabilities to JFR now that we have event streaming. The design hasn’t
started, but one could imagine a command to see object allocation events
aggregated in a histogram. Such a command may be able to operate on
user-defined events as well, including requestable/periodic events.)
Erik
That seems an ideal solution. I think there are some potential code
consolidation work further. With this change, some existing C++ JFR Jcmd
structure definitions(and other Jcmd commands) in VM level can also be lifted
to Java level because they simply forward request to Java level by
JavaCalls::call_static.
--
From:Ioi Lam
Send Time:2021 Oct. 8 (Fri.) 15:22
To:David Holmes ; dong denghui
; serviceability-dev
; hotspot-runtime-...@openjdk.java.net
Subject:Re: Extend jcmd to java application level
On 10/7/21 6:25 PM, David Holmes wrote:
> Hi Denghui,
>
> On 7/10/2021 11:58 pm, Denghui Dong wrote:
>> Hi team,
>>
>> The `jcmd` command can be used to call some built-in diagnostic commands in
>> vm.
>>
>> Can we consider extending it to the java layer like perf data, so that Java
>> developers can
>>
>> customize their diagnostic commands and then call them through `jcmd`?
>>
>> One application scenario I can think of for this extension is that some
>> statistical information
>>
>> may be collected in a java application. Triggering the output of this
>> statistical information through
>>
>> the `jcmd` command seems to me relative to other mechanisms that trigger
>> output (such as through
>>
>> an HTTP service, or periodic Printing) is more convenient.
>>
>> Any input is appreciated.
>
> If the intent is that you could issue a jcmd:
>
> jcmd MyClass.foo
>
> to have it run/use a Java thread to execute arbitrary code then I
> think a lot of careful consideration would need to be given to making
> this facility safe and secure. I can easily imagine that the thread
> used, and the timing, could cause failures. Executing arbitrary code
> may be far too general, so it might mean we need a new "service"
> interface defined that the target class has to implement.
>
> It might well be useful but will need some deep thought IMO.
>
If I understood correctly, the app would need to call an API like:
JcmdProvider.register("mycmd1", new JcmdHandler() {
public void handleCommand(String args[], PrintStream out) {
out.print("my response is ");
... }
});
and then the user can issue:
jcmd mycmd1 args .
which will reach the handleCommand() method provided by the app.
Thanks
- Ioi
> Cheers,
> David
>
>> Thanks,
>> Denghui Dong
Re: Extend jcmd to java application level
That seems an ideal solution. I think there are some potential code
consolidation work further. With this change, some existing C++ JFR Jcmd
structure definitions(and other Jcmd commands) in VM level can also be lifted
to Java level because they simply forward request to Java level by
JavaCalls::call_static.
--
From:Ioi Lam
Send Time:2021 Oct. 8 (Fri.) 15:22
To:David Holmes ; dong denghui
; serviceability-dev
; hotspot-runtime-...@openjdk.java.net
Subject:Re: Extend jcmd to java application level
On 10/7/21 6:25 PM, David Holmes wrote:
> Hi Denghui,
>
> On 7/10/2021 11:58 pm, Denghui Dong wrote:
>> Hi team,
>>
>> The `jcmd` command can be used to call some built-in diagnostic commands in
>> vm.
>>
>> Can we consider extending it to the java layer like perf data, so that Java
>> developers can
>>
>> customize their diagnostic commands and then call them through `jcmd`?
>>
>> One application scenario I can think of for this extension is that some
>> statistical information
>>
>> may be collected in a java application. Triggering the output of this
>> statistical information through
>>
>> the `jcmd` command seems to me relative to other mechanisms that trigger
>> output (such as through
>>
>> an HTTP service, or periodic Printing) is more convenient.
>>
>> Any input is appreciated.
>
> If the intent is that you could issue a jcmd:
>
> jcmd MyClass.foo
>
> to have it run/use a Java thread to execute arbitrary code then I
> think a lot of careful consideration would need to be given to making
> this facility safe and secure. I can easily imagine that the thread
> used, and the timing, could cause failures. Executing arbitrary code
> may be far too general, so it might mean we need a new "service"
> interface defined that the target class has to implement.
>
> It might well be useful but will need some deep thought IMO.
>
If I understood correctly, the app would need to call an API like:
JcmdProvider.register("mycmd1", new JcmdHandler() {
public void handleCommand(String args[], PrintStream out) {
out.print("my response is ");
... }
});
and then the user can issue:
jcmd mycmd1 args .
which will reach the handleCommand() method provided by the app.
Thanks
- Ioi
> Cheers,
> David
>
>> Thanks,
>> Denghui Dong
Re: Extend jcmd to java application level
On 10/7/21 6:25 PM, David Holmes wrote:
Hi Denghui,
On 7/10/2021 11:58 pm, Denghui Dong wrote:
Hi team,
The `jcmd` command can be used to call some built-in diagnostic commands in vm.
Can we consider extending it to the java layer like perf data, so that Java developers can
customize their diagnostic commands and then call them through `jcmd`?
One application scenario I can think of for this extension is that some statistical information
may be collected in a java application. Triggering the output of this statistical information through
the `jcmd` command seems to me relative to other mechanisms that trigger output (such as through
an HTTP service, or periodic Printing) is more convenient.
Any input is appreciated.
If the intent is that you could issue a jcmd:
jcmd MyClass.foo
to have it run/use a Java thread to execute arbitrary code then I
think a lot of careful consideration would need to be given to making
this facility safe and secure. I can easily imagine that the thread
used, and the timing, could cause failures. Executing arbitrary code
may be far too general, so it might mean we need a new "service"
interface defined that the target class has to implement.
It might well be useful but will need some deep thought IMO.
If I understood correctly, the app would need to call an API like:
JcmdProvider.register("mycmd1", new JcmdHandler() {
public void handleCommand(String args[], PrintStream out) {
out.print("my response is ");
... }
});
and then the user can issue:
jcmd mycmd1 args .
which will reach the handleCommand() method provided by the app.
Thanks
- Ioi
Cheers,
David
Thanks,
Denghui Dong
RFR: 8274620: resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java is timing out
This PR fix the issue of JDK-8274620 (resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java is timing out) The root cause is that `BufferedOutputStream` should be used when writting data to file. - Commit messages: - Undo problem list of the test case - 8274620: TestHeapDumpForLargeArray.java is timing out Changes: https://git.openjdk.java.net/jdk/pull/5860/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5860=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274620 Stats: 4 lines in 2 files changed: 0 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5860.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5860/head:pull/5860 PR: https://git.openjdk.java.net/jdk/pull/5860
