Re: RFR: 8269934: RunThese24H.java failed with EXCEPTION_ACCESS_VIOLATION in java_lang_Thread::get_thread_status
On Tue, 3 Aug 2021 06:11:37 GMT, Serguei Spitsyn wrote: >> src/hotspot/share/services/threadService.cpp line 879: >> >>> 877: // If thread is still attaching then threadObj will be NULL. >>> 878: _thread_status = threadObj == NULL ? JavaThreadStatus::NEW >>> 879: : >>> java_lang_Thread::get_thread_status(threadObj); >> >> I like the use of `JavaThreadStatus::NEW` here. >> >> Since L867 creates the _threadObj OopHandle, do you want to change both >> uses of `threadObj` to `_threadObj()`? Is that still how we fetch the oop >> from >> an OopHandle? Or even use `threadObj()`... Then we don't have to reason >> about whether the `threadObj` oop is still good... > > I was thinking about the same. If we always use _threadObj() then there is no > need to make sure the threadObj is still valid. I'm not quite sure what point is being made here. We don't have to do anything to "make sure the threadObj is still valid" as we don't have any code here that could expose a "naked oop". We only need/want to use a handle when we have to ensure the oop is preserved across calls. That is not the case here. - PR: https://git.openjdk.java.net/jdk/pull/4921
Re: RFR: 8269934: RunThese24H.java failed with EXCEPTION_ACCESS_VIOLATION in java_lang_Thread::get_thread_status
On Tue, 3 Aug 2021 06:12:35 GMT, Serguei Spitsyn wrote:
>> If a thread is attaching via JNI and has not yet created its Thread object
>> it can be caught in a ThreadSnapshot during a thread dump (VM_DumpThreads)
>> of all threads**, and the threadObj() will be NULL, so we can't pass it to
>> get_thread_status().
>>
>> ** JMM dumpThreads API
>>
>> The initial fix simply checks for a NULL threadObj() and reports thread
>> status NEW in that case.
>>
>> Alternatively we could filter the attaching thread out completely in
>> VM_DumpThreads::doit by expanding:
>>
>> if (jt->is_exiting() ||
>> jt->is_hidden_from_external_view()) {
>> // skip terminating threads and hidden threads
>> continue;
>> }
>>
>> to also check jt->is_attaching_via_jni().
>>
>> Note that higher-level code already filters out ThreadSnapshots with NULL
>> threadObj() anyway so we could go either way.
>>
>> Testing: manual hacks - see bug report.
>> - tier 1-3 sanity testing
>>
>> Thanks,
>> David
>
> Hi David,
> The fix looks fine to me.
> Thanks,
> Serguei
Thanks for the review @sspitsyn !
-
PR: https://git.openjdk.java.net/jdk/pull/4921
Re: RFR: 8269934: RunThese24H.java failed with EXCEPTION_ACCESS_VIOLATION in java_lang_Thread::get_thread_status
On Wed, 28 Jul 2021 15:17:19 GMT, Daniel D. Daugherty
wrote:
>> If a thread is attaching via JNI and has not yet created its Thread object
>> it can be caught in a ThreadSnapshot during a thread dump (VM_DumpThreads)
>> of all threads**, and the threadObj() will be NULL, so we can't pass it to
>> get_thread_status().
>>
>> ** JMM dumpThreads API
>>
>> The initial fix simply checks for a NULL threadObj() and reports thread
>> status NEW in that case.
>>
>> Alternatively we could filter the attaching thread out completely in
>> VM_DumpThreads::doit by expanding:
>>
>> if (jt->is_exiting() ||
>> jt->is_hidden_from_external_view()) {
>> // skip terminating threads and hidden threads
>> continue;
>> }
>>
>> to also check jt->is_attaching_via_jni().
>>
>> Note that higher-level code already filters out ThreadSnapshots with NULL
>> threadObj() anyway so we could go either way.
>>
>> Testing: manual hacks - see bug report.
>> - tier 1-3 sanity testing
>>
>> Thanks,
>> David
>
> src/hotspot/share/services/threadService.cpp line 879:
>
>> 877: // If thread is still attaching then threadObj will be NULL.
>> 878: _thread_status = threadObj == NULL ? JavaThreadStatus::NEW
>> 879: :
>> java_lang_Thread::get_thread_status(threadObj);
>
> I like the use of `JavaThreadStatus::NEW` here.
>
> Since L867 creates the _threadObj OopHandle, do you want to change both
> uses of `threadObj` to `_threadObj()`? Is that still how we fetch the oop from
> an OopHandle? Or even use `threadObj()`... Then we don't have to reason
> about whether the `threadObj` oop is still good...
I was thinking about the same. If we always use _threadObj() then there is no
need to make sure the threadObj is still valid.
-
PR: https://git.openjdk.java.net/jdk/pull/4921
Re: RFR: 8269934: RunThese24H.java failed with EXCEPTION_ACCESS_VIOLATION in java_lang_Thread::get_thread_status
On Wed, 28 Jul 2021 10:03:10 GMT, David Holmes wrote:
> If a thread is attaching via JNI and has not yet created its Thread object it
> can be caught in a ThreadSnapshot during a thread dump (VM_DumpThreads) of
> all threads**, and the threadObj() will be NULL, so we can't pass it to
> get_thread_status().
>
> ** JMM dumpThreads API
>
> The initial fix simply checks for a NULL threadObj() and reports thread
> status NEW in that case.
>
> Alternatively we could filter the attaching thread out completely in
> VM_DumpThreads::doit by expanding:
>
> if (jt->is_exiting() ||
> jt->is_hidden_from_external_view()) {
> // skip terminating threads and hidden threads
> continue;
> }
>
> to also check jt->is_attaching_via_jni().
>
> Note that higher-level code already filters out ThreadSnapshots with NULL
> threadObj() anyway so we could go either way.
>
> Testing: manual hacks - see bug report.
> - tier 1-3 sanity testing
>
> Thanks,
> David
Hi David,
The fix looks fine to me.
Thanks,
Serguei
-
Marked as reviewed by sspitsyn (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4921
Re: RFR: 8213714: AttachingConnector/attach/attach001 failed due to "bind failed: Address already in use"
On Sat, 17 Jul 2021 00:38:44 GMT, Alex Menkov wrote: > The fix updates the tests to use dynamic port launching debuggee and get the > listening port from the debugee output Hi Alex, The fix looks good to me. While you are a this code, may I ask you to do a minor cleanup and get rid of several commented out lines that were there before your fix? Thanks, Serguei - Changes requested by sspitsyn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4817
Re: RFR: 8213714: AttachingConnector/attach/attach001 failed due to "bind failed: Address already in use"
On Sat, 17 Jul 2021 00:38:44 GMT, Alex Menkov wrote: > The fix updates the tests to use dynamic port launching debuggee and get the > listening port from the debugee output test/hotspot/jtreg/vmTestbase/nsk/share/IORedirector.java line 74: > 72: this(); > 73: this.bin = in; > 74: addProcessor( s -> log.println(prefix + s)); Nit: unneeded space before "s ->". - PR: https://git.openjdk.java.net/jdk/pull/4817
Integrated: Merge jdk17
On Mon, 2 Aug 2021 23:30:55 GMT, Jesper Wilhelmsson wrote: > Forwardport JDK 17 -> JDK 18 This pull request has now been integrated. Changeset: c8add223 Author:Jesper Wilhelmsson URL: https://git.openjdk.java.net/jdk/commit/c8add223a10030e40ccef42e081fd0d8f00e0593 Stats: 534 lines in 36 files changed: 449 ins; 1 del; 84 mod Merge Reviewed-by: mikael - PR: https://git.openjdk.java.net/jdk/pull/4964
Re: RFR: Merge jdk17 [v2]
On Mon, 2 Aug 2021 23:53:59 GMT, Jesper Wilhelmsson wrote: >> Forwardport JDK 17 -> JDK 18 > > Jesper Wilhelmsson has updated the pull request incrementally with one > additional commit since the last revision: > > Revert "8271150: Remove EA from JDK 17 version string starting with Initial > RC promotion on Aug 5, 2021(B34)" > > This reverts commit f8fb5713074b8960f5530d7aca954f84d57c1f30. Marked as reviewed by mikael (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4964
Re: RFR: Merge jdk17 [v2]
> Forwardport JDK 17 -> JDK 18 Jesper Wilhelmsson has updated the pull request incrementally with one additional commit since the last revision: Revert "8271150: Remove EA from JDK 17 version string starting with Initial RC promotion on Aug 5, 2021(B34)" This reverts commit f8fb5713074b8960f5530d7aca954f84d57c1f30. - Changes: - all: https://git.openjdk.java.net/jdk/pull/4964/files - new: https://git.openjdk.java.net/jdk/pull/4964/files/a95b06a1..e353ddcc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4964&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4964&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4964.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4964/head:pull/4964 PR: https://git.openjdk.java.net/jdk/pull/4964
RFR: Merge jdk17
Forwardport JDK 17 -> JDK 18 - Commit messages: - Merge - 8067223: [TESTBUG] Rename Whitebox API package - 8271150: Remove EA from JDK 17 version string starting with Initial RC promotion on Aug 5, 2021(B34) The webrevs contain the adjustments done while merging with regards to each parent branch: - master: https://webrevs.openjdk.java.net/?repo=jdk&pr=4964&range=00.0 - jdk17: https://webrevs.openjdk.java.net/?repo=jdk&pr=4964&range=00.1 Changes: https://git.openjdk.java.net/jdk/pull/4964/files Stats: 535 lines in 37 files changed: 449 ins; 1 del; 85 mod Patch: https://git.openjdk.java.net/jdk/pull/4964.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4964/head:pull/4964 PR: https://git.openjdk.java.net/jdk/pull/4964
Re: RFR: 8269934: RunThese24H.java failed with EXCEPTION_ACCESS_VIOLATION in java_lang_Thread::get_thread_status
On Wed, 28 Jul 2021 10:03:10 GMT, David Holmes wrote:
> If a thread is attaching via JNI and has not yet created its Thread object it
> can be caught in a ThreadSnapshot during a thread dump (VM_DumpThreads) of
> all threads**, and the threadObj() will be NULL, so we can't pass it to
> get_thread_status().
>
> ** JMM dumpThreads API
>
> The initial fix simply checks for a NULL threadObj() and reports thread
> status NEW in that case.
>
> Alternatively we could filter the attaching thread out completely in
> VM_DumpThreads::doit by expanding:
>
> if (jt->is_exiting() ||
> jt->is_hidden_from_external_view()) {
> // skip terminating threads and hidden threads
> continue;
> }
>
> to also check jt->is_attaching_via_jni().
>
> Note that higher-level code already filters out ThreadSnapshots with NULL
> threadObj() anyway so we could go either way.
>
> Testing: manual hacks - see bug report.
> - tier 1-3 sanity testing
>
> Thanks,
> David
Still looking for a serviceability review please - @sspitsyn , @plummercj ?
Thanks
-
PR: https://git.openjdk.java.net/jdk/pull/4921
Re: [jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package [v2]
On 3/08/2021 2:25 am, Igor Ignatyev wrote: On Sat, 31 Jul 2021 20:42:10 GMT, Igor Ignatyev wrote: Hi all, could you please review this big tedious and trivial(-ish) patch which moves `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package? the majority of the patch is the following substitutions: - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g` - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g` - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g` - `s/sun.hotspot.code/jdk.test.whitebox.code/g` - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g` - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g` testing: tier1-4 Thanks, -- Igor Igor Ignatyev 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. The pull request contains 12 new commits since the last revision: - fixed ctw build - updated runtime/cds/appcds/JarBuilder to copy j.t.w.WhiteBox's inner class - updated requires.VMProps - updated TEST.ROOT - adjusted hotspot source - added test - moved and adjusted WhiteBox tests (test/lib-test/sun/hotspot/whitebox) - updated ClassFileInstaller to copy j.t.w.WhiteBox's inner class - removed sun/hotspot/parser/DiagnosticCommand - deprecated sun/hotspot classes disallowed s.h.WhiteBox w/ security manager - ... and 2 more: https://git.openjdk.java.net/jdk17/compare/8f12f2cf...237e8860 Hi David, This set of changes seems much more manageable to me. thank you for your review, David. Not sure about the new deprecation warnings for the old WB classes .. might that not itself trigger some failures? If not then I don't see how the deprecation warnings help with transitioning to the new WB class? the deprecation warnings (hopefully) will help people not to forget that they should use the new WB class in new tests. If the test passes it is unlikely people will actually notice these in the jtr file - and even if they see them they may just ignore them thinking they are similar to all the security manager warnings that we ignore. But as long as it does no harm. Cheers, David Thanks, -- Igor Hi Jie, Shall we also update the copyright year like test/lib/sun/hotspot/cpuinfo/CPUInfo.java? we most certainly shall, just pushed the commit that updates the copyright years in the touched files. Cheers, -- Igor - PR: https://git.openjdk.java.net/jdk17/pull/290
[jdk17] Integrated: 8067223: [TESTBUG] Rename Whitebox API package
On Wed, 28 Jul 2021 17:13:49 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this big tedious and trivial(-ish) patch which moves > `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package? > > the majority of the patch is the following substitutions: > - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g` > - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g` > - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g` > - `s/sun.hotspot.code/jdk.test.whitebox.code/g` > - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g` > - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g` > > testing: tier1-4 > > Thanks, > -- Igor This pull request has now been integrated. Changeset: ada58d13 Author:Igor Ignatyev URL: https://git.openjdk.java.net/jdk17/commit/ada58d13f78eb8a240220c45c573335eeb47cf07 Stats: 532 lines in 36 files changed: 449 ins; 0 del; 83 mod 8067223: [TESTBUG] Rename Whitebox API package Reviewed-by: dholmes, kvn - PR: https://git.openjdk.java.net/jdk17/pull/290
Re: RFR: 8271514: support JFR use of new ThreadsList::Iterator [v2]
> A trivial fix to support JFR use of new ThreadsList::Iterator. > > This fix was tested with Mach5 Tier[1-3]. Daniel D. Daugherty 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 three additional commits since the last revision: - Merge branch 'pull/4948' into JDK-8271514 - Merge branch 'pull/4948' into JDK-8271514 - 8271514: support JFR use of new ThreadsList::Iterator - Changes: - all: https://git.openjdk.java.net/jdk/pull/4949/files - new: https://git.openjdk.java.net/jdk/pull/4949/files/056badc2..3c64ea23 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4949&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4949&range=00-01 Stats: 31265 lines in 73 files changed: 29925 ins; 630 del; 710 mod Patch: https://git.openjdk.java.net/jdk/pull/4949.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4949/head:pull/4949 PR: https://git.openjdk.java.net/jdk/pull/4949
Re: [jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package [v2]
On Mon, 2 Aug 2021 15:56:39 GMT, Vladimir Kozlov wrote: > I agree with these revised changes for JDK 17. Thanks for your review, Vladimir. I'll rerun my testing before integrating (just for good luck). -- Igor - PR: https://git.openjdk.java.net/jdk17/pull/290
Re: [jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package [v2]
On Sat, 31 Jul 2021 20:42:10 GMT, Igor Ignatyev wrote: >> Hi all, >> >> could you please review this big tedious and trivial(-ish) patch which moves >> `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package? >> >> the majority of the patch is the following substitutions: >> - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g` >> - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g` >> - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g` >> - `s/sun.hotspot.code/jdk.test.whitebox.code/g` >> - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g` >> - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g` >> >> testing: tier1-4 >> >> Thanks, >> -- Igor > > Igor Ignatyev 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. The pull request contains 12 new > commits since the last revision: > > - fixed ctw build > - updated runtime/cds/appcds/JarBuilder to copy j.t.w.WhiteBox's inner class > - updated requires.VMProps > - updated TEST.ROOT > - adjusted hotspot source > - added test > - moved and adjusted WhiteBox tests (test/lib-test/sun/hotspot/whitebox) > - updated ClassFileInstaller to copy j.t.w.WhiteBox's inner class > - removed sun/hotspot/parser/DiagnosticCommand > - deprecated sun/hotspot classes >disallowed s.h.WhiteBox w/ security manager > - ... and 2 more: > https://git.openjdk.java.net/jdk17/compare/8f12f2cf...237e8860 Hi David, > This set of changes seems much more manageable to me. thank you for your review, David. > Not sure about the new deprecation warnings for the old WB classes .. might > that not itself trigger some failures? If not then I don't see how the > deprecation warnings help with transitioning to the new WB class? the deprecation warnings (hopefully) will help people not to forget that they should use the new WB class in new tests. Thanks, -- Igor Hi Jie, > Shall we also update the copyright year like > test/lib/sun/hotspot/cpuinfo/CPUInfo.java? we most certainly shall, just pushed the commit that updates the copyright years in the touched files. Cheers, -- Igor - PR: https://git.openjdk.java.net/jdk17/pull/290
Re: [jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package [v3]
> Hi all, > > could you please review this big tedious and trivial(-ish) patch which moves > `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package? > > the majority of the patch is the following substitutions: > - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g` > - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g` > - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g` > - `s/sun.hotspot.code/jdk.test.whitebox.code/g` > - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g` > - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g` > > testing: tier1-4 > > Thanks, > -- Igor Igor Ignatyev has updated the pull request incrementally with two additional commits since the last revision: - copyright update - fixed typo in ClassFileInstaller - Changes: - all: https://git.openjdk.java.net/jdk17/pull/290/files - new: https://git.openjdk.java.net/jdk17/pull/290/files/237e8860..fcaf41f8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17&pr=290&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk17&pr=290&range=01-02 Stats: 10 lines in 10 files changed: 0 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jdk17/pull/290.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/290/head:pull/290 PR: https://git.openjdk.java.net/jdk17/pull/290
Re: [jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package [v2]
On Sat, 31 Jul 2021 20:42:10 GMT, Igor Ignatyev wrote: >> Hi all, >> >> could you please review this big tedious and trivial(-ish) patch which moves >> `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package? >> >> the majority of the patch is the following substitutions: >> - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g` >> - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g` >> - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g` >> - `s/sun.hotspot.code/jdk.test.whitebox.code/g` >> - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g` >> - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g` >> >> testing: tier1-4 >> >> Thanks, >> -- Igor > > Igor Ignatyev 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. I agree with these revised changes for JDK 17. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/290
Re: RFR: 8269685: Optimize HeapHprofBinWriter implementation [v3]
On Mon, 5 Jul 2021 12:04:17 GMT, Lin Zang wrote: >> This PR rewrite the implementation of the HeapHprofBinWriter, which could >> simplify the logic of current implementation. >> please see detail description at >> https://bugs.openjdk.java.net/browse/JDK-8269685. > > Lin Zang has updated the pull request incrementally with one additional > commit since the last revision: > > fix write size issue Dear All, May I ask your help to review this PR? it fix the issue JDK-8262386. I believe this could be a replacement for #2803. I am planing to close it if this one looks good. BRs, Lin - PR: https://git.openjdk.java.net/jdk/pull/4666
Re: RFR: 8269909: getStack method in hprof.parser.Reader should use try-with-resource [v4]
On Thu, 15 Jul 2021 17:22:31 GMT, Serguei Spitsyn wrote: >> Lin Zang 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 five additional commits >> since the last revision: >> >> - fix indentation issue >> - Merge branch 'master' into try >> - revise code to handle the closing of embeded streams >> - Merge branch 'master' into try >> - 8269909: getStack method in hprof.parser.Reader should use >> try-with-resource > > Hi Lin, > These local names with extra numbers look strange. > You introduced these numbers in order to fix naming conflicts. > You also can avoid these conflicts by refactoring the code. > Some of these fragments can be refactored to become a separate methods. > I do not want to push hard on you with this but it is just something to > consider to simplify the code and avoid such naming problems. > Thanks, > Serguei Hi @sspitsyn, May I ask your help to see whether this pr is ok for you? Thanks! -Lin - PR: https://git.openjdk.java.net/jdk/pull/4717
