Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v8]
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Yi Yang has updated the pull request incrementally with one additional commit since the last revision: tests rely on IOOBE exception message - Changes: - all: https://git.openjdk.java.net/jdk/pull/4507/files - new: https://git.openjdk.java.net/jdk/pull/4507/files/2330ee38..ab1b509d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4507=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4507=06-07 Stats: 104 lines in 2 files changed: 18 ins; 2 del; 84 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
Integrated: 8268368: Adopt cast notation for JavaThread conversions
On Tue, 22 Jun 2021 01:11:30 GMT, Guoxiong Li wrote: > Hi all, > > Considering the consistency of `JavaThread` and other threads, such as > WorkerThread and CompilerThread, `JavaThread` could use a method named `cast` > to replace the method `Thread::as_Java_thread()`. It can reduce the Thread's > knowledge about the subtypes. > > This patch removes two methods, `JavaThread* Thread::as_Java_thread()` and > `const JavaThread* Thread::as_Java_thread() const`, of the class `Thread` and > adds two static methods, `JavaThread* cast(Thread* t)` and `const JavaThread* > cast(const Thread* t)`, to the class `JavaThread`. Correspondingly, the code > of the method `JavaThread::current()` need to be adjusted and many places > where the method `Thread::as_Java_thread()` is used need to use > `JavaThread::cast` instead. > > Test: > tier1 passed locally. > > Thanks for taking the time to review. > > Best Regards, > -- Guoxiong This pull request has now been integrated. Changeset: cd678a38 Author:Guoxiong Li Committer: David Holmes URL: https://git.openjdk.java.net/jdk/commit/cd678a383f7b23ea40132b207ddfc041394ba4c1 Stats: 158 lines in 64 files changed: 13 ins; 19 del; 126 mod 8268368: Adopt cast notation for JavaThread conversions Reviewed-by: dholmes, stefank - PR: https://git.openjdk.java.net/jdk/pull/4546
Integrated: Merge jdk17
On Wed, 23 Jun 2021 00:21:57 GMT, Jesper Wilhelmsson wrote: > Forwardport JDK 17 -> JDK 18 This pull request has now been integrated. Changeset: b6cfca8a Author:Jesper Wilhelmsson URL: https://git.openjdk.java.net/jdk/commit/b6cfca8a89810c7ed63ebc34ed9855b66ebcb5d9 Stats: 1931 lines in 60 files changed: 653 ins; 1061 del; 217 mod Merge - PR: https://git.openjdk.java.net/jdk/pull/4562
Re: RFR: Merge jdk17 [v2]
> Forwardport JDK 17 -> JDK 18 Jesper Wilhelmsson has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 59 commits: - Merge - 8268290: Improve LockFreeQueue<> utility Reviewed-by: iwalulya, tschatzl - 8264941: Remove CodeCache::mark_for_evol_deoptimization() method Reviewed-by: kvn, vlivanov, sspitsyn - 8269031: linux x86_64 check for binutils 2.25 or higher after 8265783 Reviewed-by: ihse, erikj - 8267657: Add missing PrintC1Statistics before incrementing counters Reviewed-by: iveresov - 8268857: Merge VM_PrintJNI and VM_PrintThreads and remove the unused field 'is_deadlock' of DeadlockCycle Reviewed-by: dholmes - 8269077: TestSystemGC uses "require vm.gc.G1" for large pages subtest Reviewed-by: tschatzl, kbarrett - Merge - 8268458: Add verification type for evacuation failures Reviewed-by: kbarrett, iwalulya - 8268952: Automatically update heap sizes in G1MonitoringScope Reviewed-by: kbarrett, iwalulya - ... and 49 more: https://git.openjdk.java.net/jdk/compare/35e4c272...7bf4b35f - Changes: https://git.openjdk.java.net/jdk/pull/4562/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4562=01 Stats: 16267 lines in 261 files changed: 13210 ins; 2433 del; 624 mod Patch: https://git.openjdk.java.net/jdk/pull/4562.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4562/head:pull/4562 PR: https://git.openjdk.java.net/jdk/pull/4562
RFR: 8269188: [BACKOUT] Remove CodeCache::mark_for_evol_deoptimization() method
This reverts commit 33c23a1cf2aa81551eee4a2acf271edf573558aa. Building locally. See bug for details. - Commit messages: - Revert "8264941: Remove CodeCache::mark_for_evol_deoptimization() method" Changes: https://git.openjdk.java.net/jdk/pull/4563/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4563=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269188 Stats: 78 lines in 7 files changed: 73 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/4563.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4563/head:pull/4563 PR: https://git.openjdk.java.net/jdk/pull/4563
RFR: Merge jdk17
Forwardport JDK 17 -> JDK 18 - Commit messages: - Merge - 8268404: [TESTBUG] tools/jpackage/windows/WinInstallerIconTest.java failed "AssertionError: Failed: Check icon" - 8267652: c2 loop unrolling by 8 results in reading memory past array - 8267399: C2: java/text/Normalizer/ConformanceTest.java test failed with assertion - 826: Upstream 8268230: Foreign Linker API & Windows user32/kernel32: String conversion seems broken - 8268524: nmethod::post_compiled_method_load_event racingly called on zombie - 8266631: StandardJavaFileManager: getJavaFileObjects() impl violates the spec - 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling - 8268349: Provide clear run-time warnings about Security Manager deprecation - 8268293: VectorAPI cast operation on mask and shuffle is broken - ... and 1 more: https://git.openjdk.java.net/jdk/compare/0c693e2f...7bf4b35f The merge commit only contains trivial merges, so no merge-specific webrevs have been generated. Changes: https://git.openjdk.java.net/jdk/pull/4562/files Stats: 1931 lines in 60 files changed: 653 ins; 1061 del; 217 mod Patch: https://git.openjdk.java.net/jdk/pull/4562.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4562/head:pull/4562 PR: https://git.openjdk.java.net/jdk/pull/4562
Re: RFR: 8256306: ObjectMonitor::_contentions field should not be 'jint' [v2]
On Tue, 22 Jun 2021 21:07:04 GMT, Coleen Phillimore wrote: >> I changed the _contentions and _waiters fields from jint to int and ran >> tests tier1-3. Tested tier1 with linux, mac, windows platforms. Also >> changed the _previous_owner_tid to unintptr_t from jlong, since that's what >> the cast did. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Fix INT32_MIN and cache padding. Looks good! Good catch on the padding @dcubed-ojdk ! Thanks, David - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3980
Re: RFR: 8256306: ObjectMonitor::_contentions field should not be 'jint' [v2]
On Tue, 22 Jun 2021 21:07:04 GMT, Coleen Phillimore wrote: >> I changed the _contentions and _waiters fields from jint to int and ran >> tests tier1-3. Tested tier1 with linux, mac, windows platforms. Also >> changed the _previous_owner_tid to unintptr_t from jlong, since that's what >> the cast did. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Fix INT32_MIN and cache padding. Re-reviewed v01 incremental. Thumbs up! - Marked as reviewed by dcubed (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3980
RFR: 8260540: serviceability/jdwp/AllModulesCommandTest.java failed with "Debuggee error: 'ERROR: transport error 202: bind failed: Address already in use'"
Updated AllModulesCommandTest to use dynamic port launching debuggee. Parsing debuggee listening address functionality is required in several tests (and we have other tests which need to be fixed the same way), so moved the code to new class in jdk.test.lib - Commit messages: - JDK-8260540 Changes: https://git.openjdk.java.net/jdk/pull/4560/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4560=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8260540 Stats: 121 lines in 7 files changed: 62 ins; 33 del; 26 mod Patch: https://git.openjdk.java.net/jdk/pull/4560.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4560/head:pull/4560 PR: https://git.openjdk.java.net/jdk/pull/4560
Re: RFR: 8256306: ObjectMonitor::_contentions field should not be 'jint' [v2]
> I changed the _contentions and _waiters fields from jint to int and ran tests > tier1-3. Tested tier1 with linux, mac, windows platforms. Also changed the > _previous_owner_tid to unintptr_t from jlong, since that's what the cast did. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Fix INT32_MIN and cache padding. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3980/files - new: https://git.openjdk.java.net/jdk/pull/3980/files/7c287908..07138653 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3980=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3980=00-01 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/3980.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3980/head:pull/3980 PR: https://git.openjdk.java.net/jdk/pull/3980
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v7]
On Tue, 22 Jun 2021 02:58:28 GMT, Yi Yang wrote: > I found that after solving the problem that Preconditions cannot be used > during the VM startup, a series of functions such as > String.checkIndex/checkOffset/.. can also be harmlessly replaced, but this > changeset is somewhat large and may overwrite the previous review progress. > If it will confuse the current review progress for reviewers involving in > this PR, I'd like to file a new PR to do this change later. Yes, I think that helps to review. It's often easier, review-wise, to have separate PRs for specific areas, rather than keep expanding an existing PR. - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8264941: Remove CodeCache::mark_for_evol_deoptimization() method
On Wed, 16 Jun 2021 12:52:46 GMT, Coleen Phillimore wrote: > This change removes the mark_for_evol_deoptimization method and removes the > flag that all dependencies are recorded. Before the change to walk the > entire nmethod looking for "old" (redefined) methods with metadata_do(), we > used to find methods in the code cache to deoptimize based on evol_method > dependencies. If the dependencies weren't yet recorded, we had to deoptimize > all of the methods. A long time ago, we had a customer who was unhappy with > the pause for this when they had late attach. Now we don't have this problem. > The evol_method dependencies are still used by the compiler to check for old > methods during compilation. I didn't change this but it might be something > someone who knows the compiler better can do differently and remove these > dependencies too. > Tested with tier1-6. Thanks Serguei. Thanks for the code reviews. - PR: https://git.openjdk.java.net/jdk/pull/4509
Integrated: 8264941: Remove CodeCache::mark_for_evol_deoptimization() method
On Wed, 16 Jun 2021 12:52:46 GMT, Coleen Phillimore wrote: > This change removes the mark_for_evol_deoptimization method and removes the > flag that all dependencies are recorded. Before the change to walk the > entire nmethod looking for "old" (redefined) methods with metadata_do(), we > used to find methods in the code cache to deoptimize based on evol_method > dependencies. If the dependencies weren't yet recorded, we had to deoptimize > all of the methods. A long time ago, we had a customer who was unhappy with > the pause for this when they had late attach. Now we don't have this problem. > The evol_method dependencies are still used by the compiler to check for old > methods during compilation. I didn't change this but it might be something > someone who knows the compiler better can do differently and remove these > dependencies too. > Tested with tier1-6. This pull request has now been integrated. Changeset: 33c23a1c Author:Coleen Phillimore URL: https://git.openjdk.java.net/jdk/commit/33c23a1cf2aa81551eee4a2acf271edf573558aa Stats: 78 lines in 7 files changed: 0 ins; 73 del; 5 mod 8264941: Remove CodeCache::mark_for_evol_deoptimization() method Reviewed-by: kvn, vlivanov, sspitsyn - PR: https://git.openjdk.java.net/jdk/pull/4509
Re: RFR: 8256306: ObjectMonitor::_contentions field should not be 'jint'
On Tue, 22 Jun 2021 15:45:19 GMT, Daniel D. Daugherty wrote: >> I changed the _contentions and _waiters fields from jint to int and ran >> tests tier1-3. Tested tier1 with linux, mac, windows platforms. Also >> changed the _previous_owner_tid to unintptr_t from jlong, since that's what >> the cast did. > > src/hotspot/share/runtime/objectMonitor.hpp line 151: > >> 149: #define DEFLATER_MARKER reinterpret_cast(-1) >> 150: void* volatile _owner;// pointer to owning thread OR >> BasicLock >> 151: volatile uintptr_t _previous_owner_tid; // thread id of the previous >> owner of the monitor > > Since you are changing the type for `_previous_owner_tid`, you also > need to change the `DEFINE_PAD_MINUS_SIZE` below: > > > DEFINE_PAD_MINUS_SIZE(1, OM_CACHE_LINE_SIZE, sizeof(void* volatile) + > sizeof(volatile uintptr_t)); Okay, yes, thank you for pointing this out. - PR: https://git.openjdk.java.net/jdk/pull/3980
Re: RFR: 8256306: ObjectMonitor::_contentions field should not be 'jint'
On Tue, 22 Jun 2021 15:43:34 GMT, Daniel D. Daugherty
wrote:
>> src/hotspot/share/runtime/objectMonitor.cpp line 575:
>>
>>> 573: // Make a zero contentions field negative to force any contending
>>> threads
>>> 574: // to retry. This is the second part of the async deflation dance.
>>> 575: if (Atomic::cmpxchg(&_contentions, (int)0, INT32_MIN) != 0) {
>>
>> INT_MIN
>>
>> Do we really need the cast on 0?
>
> The original cast of `(jint)0` was to get the right `cmpxchg()` selected.
> I _think_ that with the switch to INT32_MIN, a regular `0` instead of `(int)0`
> should compile.
Yes, changing to INT_MIN is more correct and I don't need the cast to int now
for zero. Recompiling on all platforms, including the picky ones for
verification. Thanks!
-
PR: https://git.openjdk.java.net/jdk/pull/3980
Re: RFR: 8256306: ObjectMonitor::_contentions field should not be 'jint'
On Mon, 21 Jun 2021 23:55:45 GMT, David Holmes wrote: >> I changed the _contentions and _waiters fields from jint to int and ran >> tests tier1-3. Tested tier1 with linux, mac, windows platforms. Also >> changed the _previous_owner_tid to unintptr_t from jlong, since that's what >> the cast did. > > src/hotspot/share/runtime/objectMonitor.cpp line 548: > >> 546: set_owner_from(NULL, DEFLATER_MARKER); >> 547: assert(contentions() >= 0, "must be non-negative: contentions=%d", >> contentions()); >> 548: _contentions = INT32_MIN; // minimum negative int > > INT_MIN (unless you want to change the type to int32_t) Fixed. - PR: https://git.openjdk.java.net/jdk/pull/3980
Re: RFR: 8256306: ObjectMonitor::_contentions field should not be 'jint'
On Mon, 21 Jun 2021 23:56:36 GMT, David Holmes wrote:
>> I changed the _contentions and _waiters fields from jint to int and ran
>> tests tier1-3. Tested tier1 with linux, mac, windows platforms. Also
>> changed the _previous_owner_tid to unintptr_t from jlong, since that's what
>> the cast did.
>
> src/hotspot/share/runtime/objectMonitor.cpp line 575:
>
>> 573: // Make a zero contentions field negative to force any contending
>> threads
>> 574: // to retry. This is the second part of the async deflation dance.
>> 575: if (Atomic::cmpxchg(&_contentions, (int)0, INT32_MIN) != 0) {
>
> INT_MIN
>
> Do we really need the cast on 0?
The original cast of `(jint)0` was to get the right `cmpxchg()` selected.
I _think_ that with the switch to INT32_MIN, a regular `0` instead of `(int)0`
should compile.
-
PR: https://git.openjdk.java.net/jdk/pull/3980
Re: RFR: 8256306: ObjectMonitor::_contentions field should not be 'jint'
On Tue, 11 May 2021 15:01:18 GMT, Coleen Phillimore wrote: > I changed the _contentions and _waiters fields from jint to int and ran tests > tier1-3. Tested tier1 with linux, mac, windows platforms. Also changed the > _previous_owner_tid to unintptr_t from jlong, since that's what the cast did. Changes requested by dcubed (Reviewer). src/hotspot/share/runtime/objectMonitor.hpp line 151: > 149: #define DEFLATER_MARKER reinterpret_cast(-1) > 150: void* volatile _owner;// pointer to owning thread OR > BasicLock > 151: volatile uintptr_t _previous_owner_tid; // thread id of the previous > owner of the monitor Since you are changing the type for `_previous_owner_tid`, you also need to change the `DEFINE_PAD_MINUS_SIZE` below: DEFINE_PAD_MINUS_SIZE(1, OM_CACHE_LINE_SIZE, sizeof(void* volatile) + sizeof(volatile uintptr_t)); - PR: https://git.openjdk.java.net/jdk/pull/3980
Re: RFR: 8256306: ObjectMonitor::_contentions field should not be 'jint'
On Tue, 22 Jun 2021 04:09:48 GMT, Thomas Stuefe wrote:
>> I changed the _contentions and _waiters fields from jint to int and ran
>> tests tier1-3. Tested tier1 with linux, mac, windows platforms. Also
>> changed the _previous_owner_tid to unintptr_t from jlong, since that's what
>> the cast did.
>
> src/hotspot/share/runtime/objectMonitor.hpp line 244:
>
>> 242: int cnts = contentions(); // read once
>> 243: if (cnts > 0) {
>> 244: ret_code |= intptr_t(cnts);
>
> I know it has nothing to do with your patch, but I am just curious. IIUC this
> is a strange way of saying "if A!=0 || B!=0 || C!=0..." . Is this for
> performance reasons? Also, why the "if (cnts > 0)", would that not be
> superfluous?
I think this code exists like this from when is_busy returned an uintptr_t. It
should be nicely rewritten with if != 0s now and likely would have better
performance. I won't do it with this change because these fields are volatile
and I think should have Atomic::loads, which would lead to a lot of discussion
that should happen independently. I'll file an RFE.
-
PR: https://git.openjdk.java.net/jdk/pull/3980
Re: RFR: 8268857: Merge VM_PrintJNI and VM_PrintThreads and remove the unused field 'is_deadlock' of DeadlockCycle [v7]
On Fri, 18 Jun 2021 06:51:53 GMT, Denghui Dong wrote: >> Hi, >> >> Could I have a review of this change that merges three vm >> operations(VM_PrintThreads, VM_PrintJNI, VM_FindDeadlocks) in thread_dump >> and signal_thread_entry. >> >> `jstack` is a very common command, even in the production environment. >> >> In addition to reduce the cost of entering safepoint, I think this patch >> also could ensure the consistency of the results of VM_PrintThreads and >> VM_FindDeadlocks. > > Denghui Dong has updated the pull request incrementally with one additional > commit since the last revision: > > update This PR should also have waiting for a review from the Serviceability team. - PR: https://git.openjdk.java.net/jdk/pull/4504
Re: RFR: 8268368: Adopt cast notation for JavaThread conversions [v3]
On Tue, 22 Jun 2021 12:56:27 GMT, David Holmes wrote: > I can sponsor, but as per hotspot integration guidelines we should wait at > least 24 hours to give people in different timezones a chance to comment. Got it. Thanks a lot! - PR: https://git.openjdk.java.net/jdk/pull/4546
Re: RFR: 8268368: Adopt cast notation for JavaThread conversions [v3]
On Tue, 22 Jun 2021 10:53:06 GMT, Guoxiong Li wrote: >> Hi all, >> >> Considering the consistency of `JavaThread` and other threads, such as >> WorkerThread and CompilerThread, `JavaThread` could use a method named >> `cast` to replace the method `Thread::as_Java_thread()`. It can reduce the >> Thread's knowledge about the subtypes. >> >> This patch removes two methods, `JavaThread* Thread::as_Java_thread()` and >> `const JavaThread* Thread::as_Java_thread() const`, of the class `Thread` >> and adds two static methods, `JavaThread* cast(Thread* t)` and `const >> JavaThread* cast(const Thread* t)`, to the class `JavaThread`. >> Correspondingly, the code of the method `JavaThread::current()` need to be >> adjusted and many places where the method `Thread::as_Java_thread()` is used >> need to use `JavaThread::cast` instead. >> >> Test: >> tier1 passed locally. >> >> Thanks for taking the time to review. >> >> Best Regards, >> -- Guoxiong > > Guoxiong Li has updated the pull request incrementally with one additional > commit since the last revision: > > Remove inline specifier I can sponsor, but as per hotspot integration guidelines we should wait at least 24 hours to give people in different timezones a chance to comment. Thanks, David - PR: https://git.openjdk.java.net/jdk/pull/4546
Re: RFR: 8268368: Adopt cast notation for JavaThread conversions
On Tue, 22 Jun 2021 11:36:02 GMT, David Holmes wrote: > The error is in ZGC code and ZGC is not tested in tier-1 or pre-submit. Got it. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/4546
Re: RFR: 8268368: Adopt cast notation for JavaThread conversions [v3]
On Tue, 22 Jun 2021 11:34:58 GMT, David Holmes wrote: >> Guoxiong Li has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove inline specifier > > Updates look fine. > > Thanks, > David @dholmes-ora @stefank Thanks for your reviews. Could I get your help to sponsor this patch? - PR: https://git.openjdk.java.net/jdk/pull/4546
Re: RFR: 8268857: Merge VM_PrintJNI and VM_PrintThreads and remove the unused field 'is_deadlock' of DeadlockCycle [v7]
On Fri, 18 Jun 2021 06:51:53 GMT, Denghui Dong wrote: >> Hi, >> >> Could I have a review of this change that merges three vm >> operations(VM_PrintThreads, VM_PrintJNI, VM_FindDeadlocks) in thread_dump >> and signal_thread_entry. >> >> `jstack` is a very common command, even in the production environment. >> >> In addition to reduce the cost of entering safepoint, I think this patch >> also could ensure the consistency of the results of VM_PrintThreads and >> VM_FindDeadlocks. > > Denghui Dong has updated the pull request incrementally with one additional > commit since the last revision: > > update > _Mailing list message from [David Holmes](mailto:david.hol...@oracle.com) on > [serviceability-dev](mailto:serviceability-...@mail.openjdk.java.net):_ > > Hi, > > Please note that hotspot changes require two reviews before integrating > (unless a trivial change). > > David > > On 22/06/2021 6:31 pm, Denghui Dong wrote: Sorry about this, I will be more aware next time. Denghui. - PR: https://git.openjdk.java.net/jdk/pull/4504
Re: RFR: 8268368: Adopt cast notation for JavaThread conversions [v3]
On Tue, 22 Jun 2021 10:53:06 GMT, Guoxiong Li wrote: >> Hi all, >> >> Considering the consistency of `JavaThread` and other threads, such as >> WorkerThread and CompilerThread, `JavaThread` could use a method named >> `cast` to replace the method `Thread::as_Java_thread()`. It can reduce the >> Thread's knowledge about the subtypes. >> >> This patch removes two methods, `JavaThread* Thread::as_Java_thread()` and >> `const JavaThread* Thread::as_Java_thread() const`, of the class `Thread` >> and adds two static methods, `JavaThread* cast(Thread* t)` and `const >> JavaThread* cast(const Thread* t)`, to the class `JavaThread`. >> Correspondingly, the code of the method `JavaThread::current()` need to be >> adjusted and many places where the method `Thread::as_Java_thread()` is used >> need to use `JavaThread::cast` instead. >> >> Test: >> tier1 passed locally. >> >> Thanks for taking the time to review. >> >> Best Regards, >> -- Guoxiong > > Guoxiong Li has updated the pull request incrementally with one additional > commit since the last revision: > > Remove inline specifier Updates look fine. Thanks, David - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4546
Re: RFR: 8268368: Adopt cast notation for JavaThread conversions [v2]
On 22/06/2021 5:24 pm, Guoxiong Li wrote:
On Tue, 22 Jun 2021 01:59:17 GMT, David Holmes wrote:
Guoxiong Li has updated the pull request incrementally with one additional
commit since the last revision:
Fix incorrect use of the method cast
Hi Guoxiong,
Thanks for picking up this enhancement request.
I wasn't sure if this would be worth the churn/disruption to the source code,
but I think it is ok and preferable to use the cast notation.
The changes look good except for one mistake flagged below.
Note you need at least two reviewers before integrating this.
Thanks,
David
@dholmes-ora Thanks for your review. I updated the code just now.
I am surprised that the `tier1` (locally and the `Pre-submit tests`) can't find
the mistake you pointed out.
Maybe we can improve the `tier1` or the `Pre-submit tests` in the future.
The error is in ZGC code and ZGC is not tested in tier-1 or pre-submit.
Cheers,
David
src/hotspot/share/gc/z/zFuture.inline.hpp line 49:
47: // Wait for notification
48: Thread* const thread = Thread::current();
49: if (JavaThread::cast(thread)) {
This is wrong - we still need the is_Java_thread() query; and cast is not a
boolean operator.
Fixed. It's a wrong use of the method `cast`. Thanks for finding it. I re-read
my patch to avoid the similar mistake.
-
PR: https://git.openjdk.java.net/jdk/pull/4546
Re: Integrated: 8268857: Merge VM_PrintJNI and VM_PrintThreads and remove the unused field 'is_deadlock' of DeadlockCycle
Hi, Please note that hotspot changes require two reviews before integrating (unless a trivial change). David On 22/06/2021 6:31 pm, Denghui Dong wrote: On Wed, 16 Jun 2021 03:33:14 GMT, Denghui Dong wrote: Hi, Could I have a review of this change that merges three vm operations(VM_PrintThreads, VM_PrintJNI, VM_FindDeadlocks) in thread_dump and signal_thread_entry. `jstack` is a very common command, even in the production environment. In addition to reduce the cost of entering safepoint, I think this patch also could ensure the consistency of the results of VM_PrintThreads and VM_FindDeadlocks. This pull request has now been integrated. Changeset: 1f0ea7c3 Author:Denghui Dong Committer: Yi Yang URL: https://git.openjdk.java.net/jdk/commit/1f0ea7c3d62221405de982ba484c0ee985fa9d7b Stats: 51 lines in 8 files changed: 5 ins; 33 del; 13 mod 8268857: Merge VM_PrintJNI and VM_PrintThreads and remove the unused field 'is_deadlock' of DeadlockCycle Reviewed-by: dholmes - PR: https://git.openjdk.java.net/jdk/pull/4504
Re: RFR: 8268368: Adopt cast notation for JavaThread conversions [v2]
On Tue, 22 Jun 2021 10:21:45 GMT, Stefan Karlsson wrote: >> Guoxiong Li has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix incorrect use of the method cast > > src/hotspot/share/runtime/thread.hpp line 1432: > >> 1430: assert(t->is_Java_thread(), "incorrect cast to const JavaThread"); >> 1431: return static_cast(t); >> 1432: } > > Now that you've written the code in-place, you could remove the `inline` > specifier. Fixed. Thanks @stefank for your review. - PR: https://git.openjdk.java.net/jdk/pull/4546
Re: RFR: 8268368: Adopt cast notation for JavaThread conversions [v3]
> Hi all, > > Considering the consistency of `JavaThread` and other threads, such as > WorkerThread and CompilerThread, `JavaThread` could use a method named `cast` > to replace the method `Thread::as_Java_thread()`. It can reduce the Thread's > knowledge about the subtypes. > > This patch removes two methods, `JavaThread* Thread::as_Java_thread()` and > `const JavaThread* Thread::as_Java_thread() const`, of the class `Thread` and > adds two static methods, `JavaThread* cast(Thread* t)` and `const JavaThread* > cast(const Thread* t)`, to the class `JavaThread`. Correspondingly, the code > of the method `JavaThread::current()` need to be adjusted and many places > where the method `Thread::as_Java_thread()` is used need to use > `JavaThread::cast` instead. > > Test: > tier1 passed locally. > > Thanks for taking the time to review. > > Best Regards, > -- Guoxiong Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision: Remove inline specifier - Changes: - all: https://git.openjdk.java.net/jdk/pull/4546/files - new: https://git.openjdk.java.net/jdk/pull/4546/files/259730be..c0347b6c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4546=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4546=01-02 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/4546.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4546/head:pull/4546 PR: https://git.openjdk.java.net/jdk/pull/4546
Re: RFR: 8236212: CompiledMethodLoad and CompiledMethodUnload events can be posted in START phase
On Fri, 18 Jun 2021 22:57:41 GMT, Alex Menkov wrote: > Accordingly the spec CompiledMethodLoad/CompiledMethodUnload events may be > sent in the start and live phases. > VM implementation send them only in the live phase, but tests should expect > them in the start phase too Hi Alex, This looks good to me. Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4535
Re: RFR: 8268368: Adopt cast notation for JavaThread conversions [v2]
On Tue, 22 Jun 2021 07:17:00 GMT, Guoxiong Li wrote: >> Hi all, >> >> Considering the consistency of `JavaThread` and other threads, such as >> WorkerThread and CompilerThread, `JavaThread` could use a method named >> `cast` to replace the method `Thread::as_Java_thread()`. It can reduce the >> Thread's knowledge about the subtypes. >> >> This patch removes two methods, `JavaThread* Thread::as_Java_thread()` and >> `const JavaThread* Thread::as_Java_thread() const`, of the class `Thread` >> and adds two static methods, `JavaThread* cast(Thread* t)` and `const >> JavaThread* cast(const Thread* t)`, to the class `JavaThread`. >> Correspondingly, the code of the method `JavaThread::current()` need to be >> adjusted and many places where the method `Thread::as_Java_thread()` is used >> need to use `JavaThread::cast` instead. >> >> Test: >> tier1 passed locally. >> >> Thanks for taking the time to review. >> >> Best Regards, >> -- Guoxiong > > Guoxiong Li has updated the pull request incrementally with one additional > commit since the last revision: > > Fix incorrect use of the method cast Marked as reviewed by stefank (Reviewer). src/hotspot/share/runtime/thread.hpp line 1432: > 1430: assert(t->is_Java_thread(), "incorrect cast to const JavaThread"); > 1431: return static_cast(t); > 1432: } Now that you've written the code in-place, you could remove the `inline` specifier. - PR: https://git.openjdk.java.net/jdk/pull/4546
Re: RFR: 8268368: Adopt cast notation for JavaThread conversions [v2]
On Tue, 22 Jun 2021 07:21:29 GMT, Guoxiong Li wrote: > Maybe we can improve the tier1 or the Pre-submit tests in the future. I meant to improve the test coverage. - PR: https://git.openjdk.java.net/jdk/pull/4546
Re: RFR: 8268433: serviceability/dcmd/framework/VMVersionTest.java fails with Unable to send object throw not established PipeIO Listener Thread connection
On Fri, 18 Jun 2021 20:20:53 GMT, Alex Menkov wrote:
> Main logic of the tests is:
>
> TestProcessLauncher t = ...;
> try {
> t.launch();
> .. perform testing ...
> } finally {
>t.quit();
> }
>
> We have some problem with the tests, but the exception from
> TestProcessLauncher.quit() masks it.
> The failures are very intermittent, so need to fix this excption to get more
> information about real error.
Marked as reviewed by sspitsyn (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/4532
Re: RFR: JDK-8268893: jcmd to trim the glibc heap [v2]
On Thu, 17 Jun 2021 08:53:55 GMT, Thomas Stuefe wrote:
>> Proposal to add a Linux+glibc-only jcmd to manually induce malloc_trim(3) in
>> the VM process.
>>
>>
>> The glibc is somewhat notorious for retaining released C Heap memory:
>> calling free(3) returns memory to the glibc, and most libc variants will
>> return at least a portion of it back to the Operating System, but the glibc
>> often does not.
>>
>> This depends on the granularity of the allocations and a number of other
>> factors, but we found that many small allocations in particular may cause
>> the process heap segment (hence RSS) to get bloaty. This can cause the VM to
>> not recover from C-heap usage spikes.
>>
>> The glibc offers an API, "malloc_trim", which can be used to cause the glibc
>> to return free'd memory back to the Operating System.
>>
>> This may cost performance, however, and therefore I hesitate to call
>> malloc_trim automatically. That may be an idea for another day.
>>
>> Instead of an automatic trim I propose to add a jcmd which allows to
>> manually trigger a libc heap trim. Such a command would have two purposes:
>> - when analyzing cases of high memory footprint, it allows to distinguish
>> "real" footprint, e.g. leaks, from a cases where the glibc just holds on to
>> memory
>> - as a stop gap measure it allows to release pressure from a high footprint
>> scenario.
>>
>> Note that this command also helps with analyzing libc peaks which had
>> nothing to do with the VM - e.g. peaks created by customer code which just
>> happens to share the same process as the VM. Such memory does not even have
>> to show up in NMT.
>>
>> I propose to introduce this command for Linux only. Other OSes (apart maybe
>> AIX) do not seem to have this problem, but Linux is arguably important
>> enough in itself to justify a Linux specific jcmd.
>>
>> If this finds agreement, I will file a CSR.
>>
>> Note that an alternative to a Linux-only jcmd would be a command which would
>> trim the C-heap on all platforms, with implementations to be filled out
>> later.
>>
>> =
>>
>> This patch:
>>
>> - introduces a new jcmd, "VM.trim_libc_heap", no arguments, which trims the
>> glibc heap on glibc platforms.
>> - includes a (rather basic) test
>> - the command calls malloc_trim(3), and additionally prints out its effect
>> (changes caused in virt size, rss and swap space)
>> - I refactored some code in os_linux.cpp to factor out scanning
>> /proc/self/status to get kernel memory information.
>>
>> =
>>
>> Example:
>>
>> A programm causes a temporary peak in C-heap usage (in this case, triggered
>> via Unsafe.allocateMemory), right away frees the memory again, so its not
>> leaky. The peak in RSS was ~8G (even though the user allocation was way
>> smaller - glibc has a lot of overhead). The effects of this peak linger even
>> after returning that memory to the glibc:
>>
>>
>>
>> thomas@starfish:~$ jjjcmd AllocCHeap VM.info | grep Resident
>> Resident Set Size: 8685896K (peak: 8685896K) (anon: 8648680K, file: 37216K,
>> shmem: 0K)
>>
>>
>>
>> We execute the new trim command via jcmd:
>>
>>
>> thomas@starfish:~$ jjjcmd AllocCHeap VM.trim_libc_heap
>> 18770:
>> Attempting trim...
>> Done.
>> Virtual size before: 28849744k, after: 28849724k, (-20k)
>> RSS before: 8685896k, after: 920740k, (-7765156k)
>> Swap before: 0k, after: 0k, (0k)
>>
>>
>> It prints out reduction in virtual size, rss and swap. The virtual size did
>> not decrease since no mappings had been unmapped by the glibc. However, the
>> process heap was shrunk heavily by the glibc, resulting in a large drop in
>> RSS (8.5G->900M), freeing >7G of memory:
>>
>>
>> thomas@starfish:~$ jjjcmd AllocCHeap VM.info | grep Resident
>> Resident Set Size: 920740K (peak: 8686004K) (anon: 883460K, file: 37280K,
>> shmem: 0K)
>>^^^
>>
>>
>> When the VM is started with -Xlog:os, this is also logged:
>>
>>
>> [139,068s][info][os] malloc_trim:
>> [139,068s][info][os] Virtual size before: 28849744k, after: 28849724k, (-20k)
>> RSS before: 8685896k, after: 920740k, (-7765156k)
>> Swap before: 0k, after: 0k, (0k)
>
> Thomas Stuefe has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Feedback Severin; renamed query function
src/hotspot/os/linux/trimCHeapDCmd.cpp line 70:
> 68: _output->print_raw(ss_report.base());
> 69: log_info(os)("malloc_trim: ");
> 70: log_info(os)("%s", ss_report.base());
Might also make sense to put this into a single log line like
`log_info(os)("malloc_trim:\n%s", ss_report.base());` to get something like:
[139,068s][info][os] malloc_trim:
Virtual size before: 28849744k, after: 28849724k, (-20k)
RSS before: 8685896k, after: 920740k, (-7765156k)
Swap before: 0k, after: 0k, (0k)
instead of:
[139,068s][info][os] malloc_trim:
[139,068s][info][os] Virtual size before: 28849744k, after:
Re: RFR: 8268368: Adopt cast notation for JavaThread conversions [v2]
On Tue, 22 Jun 2021 01:59:17 GMT, David Holmes wrote:
>> Guoxiong Li has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix incorrect use of the method cast
>
> Hi Guoxiong,
>
> Thanks for picking up this enhancement request.
>
> I wasn't sure if this would be worth the churn/disruption to the source code,
> but I think it is ok and preferable to use the cast notation.
>
> The changes look good except for one mistake flagged below.
>
> Note you need at least two reviewers before integrating this.
>
> Thanks,
> David
@dholmes-ora Thanks for your review. I updated the code just now.
I am surprised that the `tier1` (locally and the `Pre-submit tests`) can't find
the mistake you pointed out.
Maybe we can improve the `tier1` or the `Pre-submit tests` in the future.
> src/hotspot/share/gc/z/zFuture.inline.hpp line 49:
>
>> 47: // Wait for notification
>> 48: Thread* const thread = Thread::current();
>> 49: if (JavaThread::cast(thread)) {
>
> This is wrong - we still need the is_Java_thread() query; and cast is not a
> boolean operator.
Fixed. It's a wrong use of the method `cast`. Thanks for finding it. I re-read
my patch to avoid the similar mistake.
-
PR: https://git.openjdk.java.net/jdk/pull/4546
Re: RFR: 8268368: Adopt cast notation for JavaThread conversions [v2]
> Hi all, > > Considering the consistency of `JavaThread` and other threads, such as > WorkerThread and CompilerThread, `JavaThread` could use a method named `cast` > to replace the method `Thread::as_Java_thread()`. It can reduce the Thread's > knowledge about the subtypes. > > This patch removes two methods, `JavaThread* Thread::as_Java_thread()` and > `const JavaThread* Thread::as_Java_thread() const`, of the class `Thread` and > adds two static methods, `JavaThread* cast(Thread* t)` and `const JavaThread* > cast(const Thread* t)`, to the class `JavaThread`. Correspondingly, the code > of the method `JavaThread::current()` need to be adjusted and many places > where the method `Thread::as_Java_thread()` is used need to use > `JavaThread::cast` instead. > > Test: > tier1 passed locally. > > Thanks for taking the time to review. > > Best Regards, > -- Guoxiong Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision: Fix incorrect use of the method cast - Changes: - all: https://git.openjdk.java.net/jdk/pull/4546/files - new: https://git.openjdk.java.net/jdk/pull/4546/files/3c9f6dbe..259730be Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4546=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4546=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4546.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4546/head:pull/4546 PR: https://git.openjdk.java.net/jdk/pull/4546
