Re: RFR: 8252104: parallel heap inspection for ShenandoahHeap [v3]
> - enable parallel heap inspecton for ShenandoahHeap > - preliminary evaluation: > Time of jmap histo on (8GB heap with 4GB objects) > * before: 5.186s > * after : 1.698s Lin Zang 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 one new commit since the last revision: 8252104: parallel heap inspection for ShenandoahHeap - enable parallel heap inspecton for ShenandoahHeap - preliminary evaluation: Time of jmap histo on (8GB heap with 4GB objects) * before: 5.186s * after : 1.698s - Changes: - all: https://git.openjdk.java.net/jdk/pull/67/files - new: https://git.openjdk.java.net/jdk/pull/67/files/6645cc27..313d6cb5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=67&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=67&range=01-02 Stats: 16 lines in 2 files changed: 7 ins; 3 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/67.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/67/head:pull/67 PR: https://git.openjdk.java.net/jdk/pull/67
Re: RFR: 8252104: parallel heap inspection for ShenandoahHeap [v3]
On Fri, 11 Sep 2020 13:54:38 GMT, Zhengyu Gu wrote: >> Thank you, we'll take a look next week. > > Hi Lin, > > 1) The patch does not compile with assertion on > @@ -1490,7 +1490,7 @@ private: >cl->do_object(obj); >obj->oop_iterate(&oops); > } > -assert(q.is_empty(), "should be empty"); > +assert(q->is_empty(), "should be empty"); > > 2) I don't like to export following methods as public. Instead, you can > declare ShenandoahParallelObjectIterator as > friend class of ShenandoahHeap. >void scan_roots_for_iteration(Stack* oop_stack, > ObjectIterateScanRootClosure* oops); >bool prepare_aux_bitmap_for_iteration(); >void reclaim_aux_bitmap_for_iteration(); > > 3) Please rename ObjectIterateParScanClosure to > ShenandoahObjectIterateParScanClosure for code convention > > 4) There is no point to seed mark roots if prepare_aux_bitmap_for_iteration() > failed in > ShenandoahParallelObjectIterator constructor. Hi @zhengyu123 , Thanks for your comments! I have refined the code and update the PR. -Lin - PR: https://git.openjdk.java.net/jdk/pull/67
Integrated: 8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize…
On Thu, 10 Sep 2020 23:38:45 GMT, Leonid Mesnik wrote: > The NULL oops are corrupted by CheckUnhandledOops and should be re-written > with NULL to pass testing > with -XX:+CheckUnhandledOops. This pull request has now been integrated. Changeset: 306b1663 Author:Leonid Mesnik URL: https://git.openjdk.java.net/jdk/commit/306b1663 Stats: 3 lines in 1 file changed: 1 ins; 2 del; 0 mod 8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize… Reviewed-by: coleenp, dholmes - PR: https://git.openjdk.java.net/jdk/pull/123
Re: RFR: 8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize… [v2]
On Fri, 11 Sep 2020 18:26:03 GMT, Coleen Phillimore wrote:
>> This is a specific of "CheckUnhandledOops"
>> I've written in bug comment "Another possible fix would be to disable
>> corruption of NULL unhandled oops. They couldn't
>> be changed really."
>> We discussed it with Coleen and seems that moving NULL oops out of possible
>> safepoint or handling them seems easier
>> option than changing UnhandledOops.cpp to don't corrupt NULL. It is here:
>> https://github.com/openjdk/jdk/blob/77bdc3065057b07a676b010562c89bb0f21512b7/src/hotspot/share/runtime/unhandledOops.cpp#L113
>
> ThreadService::get_current_contended_monitor calls
> Thread::check_for_dangling_thread_pointer calls
> ThreadsSMRSupport::is_a_protected_JavaThread_with_lock((JavaThread *) thread),
> The potential safepoint is here, where CheckUnhandledOops puts junk in any
> oop on the stack.
>
> inline bool ThreadsSMRSupport::is_a_protected_JavaThread_with_lock(JavaThread
> *thread) {
> MutexLocker ml(Threads_lock->owned_by_self() ? NULL : Threads_lock);
> return is_a_protected_JavaThread(thread);
> }
Thanks Coleen. I'm still not sure that CheckUnhandledOops should be touching
NULL oops but ...
Leonid the workaround seems okay.
-
PR: https://git.openjdk.java.net/jdk/pull/123
Re: RFR: 8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize… [v2]
On Fri, 11 Sep 2020 02:56:26 GMT, Leonid Mesnik wrote: >> The NULL oops are corrupted by CheckUnhandledOops and should be re-written >> with NULL to pass testing >> with -XX:+CheckUnhandledOops. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > 8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize(...) Marked as reviewed by dholmes (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/123
Re: RFR: 8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize… [v2]
On Fri, 11 Sep 2020 03:37:55 GMT, Leonid Mesnik wrote:
>> I'm missing something. How can a NULL oop get corrupted even if there is a
>> GC?
>
> This is a specific of "CheckUnhandledOops"
> I've written in bug comment "Another possible fix would be to disable
> corruption of NULL unhandled oops. They couldn't
> be changed really."
> We discussed it with Coleen and seems that moving NULL oops out of possible
> safepoint or handling them seems easier
> option than changing UnhandledOops.cpp to don't corrupt NULL. It is here:
> https://github.com/openjdk/jdk/blob/77bdc3065057b07a676b010562c89bb0f21512b7/src/hotspot/share/runtime/unhandledOops.cpp#L113
ThreadService::get_current_contended_monitor calls
Thread::check_for_dangling_thread_pointer calls
ThreadsSMRSupport::is_a_protected_JavaThread_with_lock((JavaThread *) thread),
The potential safepoint is here, where CheckUnhandledOops puts junk in any oop
on the stack.
inline bool ThreadsSMRSupport::is_a_protected_JavaThread_with_lock(JavaThread
*thread) {
MutexLocker ml(Threads_lock->owned_by_self() ? NULL : Threads_lock);
return is_a_protected_JavaThread(thread);
}
-
PR: https://git.openjdk.java.net/jdk/pull/123
Re: RFR: 8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize… [v2]
On Fri, 11 Sep 2020 02:56:26 GMT, Leonid Mesnik wrote: >> The NULL oops are corrupted by CheckUnhandledOops and should be re-written >> with NULL to pass testing >> with -XX:+CheckUnhandledOops. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > 8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize(...) Marked as reviewed by coleenp (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/123
Re: RFR: 8252933: com.sun.tools.jdi.ObjectReferenceImpl#validateAssignment always requests referenceType
On Fri, 11 Sep 2020 04:00:35 GMT, Chris Plummer wrote: >> The change fixes the regression introduced by >> https://bugs.openjdk.java.net/browse/JDK-8241080. >> >> Method validateAssignment() in com.sun.tools.jdi.ObjectReferenceImpl now >> always retrieves the reference type and that >> requires one more JDWP packet if the value is not cached yet. Before >> https://bugs.openjdk.java.net/browse/JDK-8241080 >> this happened for arrays only. Testing: tier1-tier3 tests passes. > > The changes look good to me. Can I ask how you noticed the extra unnecessary > JDWP packet? Thanks @plummercj for the review. The issue with an extra JDWP packet was reported in serviceability-dev mail list : https://mail.openjdk.java.net/pipermail/serviceability-dev/2020-September/032960.html . - PR: https://git.openjdk.java.net/jdk/pull/124
Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase
On Fri, 11 Sep 2020 07:15:26 GMT, Dmitriy Dumanskiy wrote: >> 1) This is un-necessary churn. >> 2) I can't even be sure I am finding the ones in my area because there's so >> much here >> 3) The ones I can find have no need of whatever performance improvement this >> might bring. >> I think this whole PR should be withdrawn, and there may an attempt at >> measuring the benefits of this for the various >> cases before submitting in appropriate smaller chunks. But I'll tell you now >> I don't see a need for the test updates >> you are making. > > Ok, sorry for the distraction. Our local Santuario maintainer says: In general, changes to Apache Santuario should also be made at Apache so we stay in sync. - PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: 8252104: parallel heap inspection for ShenandoahHeap [v2]
On Fri, 11 Sep 2020 10:54:42 GMT, Aleksey Shipilev wrote: >> Hi @shipilev, >> I have update a PR that trying to reuse code in serial and parallel >> heap iteration. >> would you like to review again? Thanks >> -Lin > > Thank you, we'll take a look next week. Hi Lin, 1) The patch does not compile with assertion on @@ -1490,7 +1490,7 @@ private: cl->do_object(obj); obj->oop_iterate(&oops); } -assert(q.is_empty(), "should be empty"); +assert(q->is_empty(), "should be empty"); 2) I don't like to export following methods as public. Instead, you can declare ShenandoahParallelObjectIterator as friend class of ShenandoahHeap. void scan_roots_for_iteration(Stack* oop_stack, ObjectIterateScanRootClosure* oops); bool prepare_aux_bitmap_for_iteration(); void reclaim_aux_bitmap_for_iteration(); 3) Please rename ObjectIterateParScanClosure to ShenandoahObjectIterateParScanClosure for code convention 4) There is no point to seed mark roots if prepare_aux_bitmap_for_iteration() failed in ShenandoahParallelObjectIterator constructor. - PR: https://git.openjdk.java.net/jdk/pull/67
Re: RFR: 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
On Fri, 11 Sep 2020 09:54:50 GMT, Erik Helin wrote: > > > Sorry, now I see. Yes, please remove `, 8233915` from the title! Thanks for helping. The commit message does look better now. - PR: https://git.openjdk.java.net/jdk/pull/119
Re: RFR: 8252104: parallel heap inspection for ShenandoahHeap [v2]
On Fri, 11 Sep 2020 09:05:53 GMT, Lin Zang wrote: >> Thanks @shipilev， This is shenandoah support based on parallel heap >> inspection enabling in serviceability (tracked with >> https://bugs.openjdk.java.net/browse/JDK-8215624). hope it is helpful for >> your review. > > Hi @shipilev, > I have update a PR that trying to reuse code in serial and parallel heap > iteration. > would you like to review again? Thanks > -Lin Thank you, we'll take a look next week. - PR: https://git.openjdk.java.net/jdk/pull/67
Re: RFR: 8252104: parallel heap inspection for ShenandoahHeap [v2]
> - enable parallel heap inspecton for ShenandoahHeap > - preliminary evaluation: > Time of jmap histo on (8GB heap with 4GB objects) > * before: 5.186s > * after : 1.698s Lin Zang 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 one new commit since the last revision: 8252104: parallel heap inspection for ShenandoahHeap - enable parallel heap inspecton for ShenandoahHeap - preliminary evaluation: Time of jmap histo on (8GB heap with 4GB objects) * before: 5.186s * after : 1.698s - Changes: - all: https://git.openjdk.java.net/jdk/pull/67/files - new: https://git.openjdk.java.net/jdk/pull/67/files/7b400d2c..6645cc27 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=67&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=67&range=00-01 Stats: 138 lines in 2 files changed: 56 ins; 71 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/67.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/67/head:pull/67 PR: https://git.openjdk.java.net/jdk/pull/67
Re: RFR: 8252104: parallel heap inspection for ShenandoahHeap [v2]
On Wed, 9 Sep 2020 03:00:26 GMT, Lin Zang wrote: >> Thanks, this looks interesting. >> >> I need more time to digest this. It feels like we can merge together the >> parallel (new) and serial (old) heap iteration >> code for better maintainability. I could try to do so later. > > Thanks @shipilev， This is shenandoah support based on parallel heap > inspection enabling in serviceability (tracked with > https://bugs.openjdk.java.net/browse/JDK-8215624). hope it is helpful for > your review. Hi @shipilev, I have update a PR that trying to reuse code in serial and parallel heap iteration. would you like to review again? Thanks -Lin - PR: https://git.openjdk.java.net/jdk/pull/67
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v2]
On Tue, 8 Sep 2020 23:44:58 GMT, David Holmes wrote:
>> Aleksei Voitylov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> JDK-8247589: Implementation of Alpine Linux/x64 Port
>
> make/autoconf/platform.m4 line 536:
>
>> 534: AC_SUBST(HOTSPOT_$1_CPU_DEFINE)
>> 535:
>> 536: if test "x$OPENJDK_$1_LIBC" = "xmusl"; then
>
> I'm not clear why we only check for musl when setting the HOTSPOT_$1_LIBC
> variable
this check is removed in the updated version. As a consequence, LIBC variable
is added to the release file for all
platforms, which is probably a good thing.
> src/hotspot/os/linux/os_linux.cpp line 624:
>
>> 622: // confstr() from musl libc returns EINVAL for
>> 623: // _CS_GNU_LIBC_VERSION and _CS_GNU_LIBPTHREAD_VERSION
>> 624: os::Linux::set_libc_version("unknown");
>
> This should be "musl - unknown" as we don't know an exact version but we do
> know that it is musl.
Right, this should be more consistent with glibc which here returns name and
version. Updated as suggested.
> src/hotspot/os/linux/os_linux.cpp line 625:
>
>> 623: // _CS_GNU_LIBC_VERSION and _CS_GNU_LIBPTHREAD_VERSION
>> 624: os::Linux::set_libc_version("unknown");
>> 625: os::Linux::set_libpthread_version("unknown");
>
> This should be "musl - unknown" as we don't know an exact version but we do
> know that it is musl.
The pthread version is also updated to "musl - unknown". Reason being, pthread
functionality for musl is built into the
library.
> src/hotspot/share/runtime/abstract_vm_version.cpp line 263:
>
>> 261: #define LIBC_STR "-" XSTR(LIBC)
>> 262: #else
>> 263: #define LIBC_STR ""
>
> Again I'm not clear why we do nothing in the non-musl case? Shouldn't we be
> reporting glibc or musl?
Unlike the case above, I think it's best to keep it as is. I'd expect there to
be a bunch of scripts in the wild which
parse it and may get broken when facing a triplet for existing platforms.
> src/jdk.hotspot.agent/linux/native/libsaproc/ps_proc.c line 284:
>
>> 282: // To improve portability across platforms and avoid conflicts
>> 283: // between GNU and XSI versions of strerror_r, plain strerror is
>> used.
>> 284: // It's safe because this code is not used in any multithreaded
>> environment.
>
> I still question this assertion. The issue is not that the current code path
> that leads to strerror use may be executed
> concurrently but that any other strerror use could be concurrent with this
> one. I would consider this a "must fix" if
> not for the fact we already use strerror in the code and so this doesn't
> really change the exposure to the problem.
You are right! The updated version #ifdefs the XSI or GNU versions of
strerror_r in this place. Note to self: file a
bug to address the usage of strerror in other places, at least for HotSpot.
> test/hotspot/jtreg/runtime/StackGuardPages/exeinvoke.c line 282:
>
>> 280:
>> 281: pthread_attr_init(&thread_attr);
>> 282: pthread_attr_setstacksize(&thread_attr, stack_size);
>
> Just a comment in response to the explanation as to why this change is
> needed. If the default thread stacksize under
> musl is insufficient to successfully attach such a thread to the VM then this
> will cause problems for applications that
> embed the VM directly (or which otherwise directly attach existing threads).
This fix
https://git.musl-libc.org/cgit/musl/commit/src/aio/aio.c?id=1a6d6f131bd60ec2a858b34100049f0c042089f2
addresses the problem for recent versions of musl. The test passes on a recent
Alpine Linux 3.11.6 (musl 1.1.24) and
fails on Alpine Linux 3.8.2 (musl 1.1.19) without this test fix.
There are still older versions of the library in the wild, hence the test fix.
The mitigation for such users would be a
distro upgrade.
> test/hotspot/jtreg/runtime/TLS/exestack-tls.c line 60:
>
>> 58: }
>> 59:
>> 60: #if defined(__GLIBC)
>
> Why do we use this form here but at line 30 we have:
> #ifdef __GLIBC__
> ?
Fixed to be consistent.
-
PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: 8252105: parallel heap inspection for ZCollectedHeap [v2]
> - enable parallel heap inspection for ZCollectedHeap > - preliminary evaluation: > Time of jmap histo on 8GB heap with ~5GB objects > * before: 7.103s > * after : 2.734s (with 4 parallel threads) Lin Zang 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 one new commit since the last revision: 8252105: parallel heap inspection for ZCollectedHeap - enable parallel heap inspection for ZCollectedHeap - preliminary evaluation: Time of jmap histo on 8GB heap with ~5GB objects * before: 7.103s * after : 2.734s (with 4 parallel threads) - Changes: - all: https://git.openjdk.java.net/jdk/pull/103/files - new: https://git.openjdk.java.net/jdk/pull/103/files/b3f33db7..a9b616a5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=103&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=103&range=00-01 Stats: 483 lines in 7 files changed: 116 ins; 343 del; 24 mod Patch: https://git.openjdk.java.net/jdk/pull/103.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/103/head:pull/103 PR: https://git.openjdk.java.net/jdk/pull/103
Re: RFR: 8252103: Parallel heap inspection for ParallelScavengeHeap
On Sun, 6 Sep 2020 01:13:48 GMT, Lin Zang wrote: > - Parallel heap iteration support for PSS > - JBS: https://bugs.openjdk.java.net/browse/JDK-8252103 Dear All, May I ask your help to review this PR? Thanks! -Lin - PR: https://git.openjdk.java.net/jdk/pull/25
Re: RFR: 8252105: parallel heap inspection for ZCollectedHeap [v2]
On Thu, 10 Sep 2020 09:57:29 GMT, Lin Zang wrote: >> Just looked at this briefly. My initial comment is that we need to avoid all >> the code duplicated from ZHeapIterator and >> isolate all this a bit better. Perhaps folding everything into one iterator, >> that can be used by both object_iterate() >> and parallel_object_iterator(). I'll take a closer look, and perhaps try >> some alternatives, when I get a chance. > > Hi @pliden, > Thanks for your comments, I will try to merge ZHeapIterator and > ZHeapParIterator and update the pr then. > -Lin Hi @pliden, I updated the PR which merge the changed code into ZHeapIterator. May I ask your help to review it ? Thanks. -Lin - PR: https://git.openjdk.java.net/jdk/pull/103
Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase
On Thu, 10 Sep 2020 23:57:38 GMT, Phil Race wrote: >> **isEmpty** is faster + has less byte code + easier to read. Benchmarks >> could be found >> >> [here](https://medium.com/javarevisited/micro-optimizations-in-java-string-equals-22be19fd8416). > > 1) This is un-necessary churn. > 2) I can't even be sure I am finding the ones in my area because there's so > much here > 3) The ones I can find have no need of whatever performance improvement this > might bring. > I think this whole PR should be withdrawn, and there may an attempt at > measuring the benefits of this for the various > cases before submitting in appropriate smaller chunks. But I'll tell you now > I don't see a need for the test updates > you are making. Ok, sorry for the distraction. - PR: https://git.openjdk.java.net/jdk/pull/29
Withdrawn: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase
On Sun, 6 Sep 2020 13:57:19 GMT, Dmitriy Dumanskiy wrote: > **isEmpty** is faster + has less byte code + easier to read. Benchmarks could > be found > > [here](https://medium.com/javarevisited/micro-optimizations-in-java-string-equals-22be19fd8416). This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v2]
> continuing the review thread from here > https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/068546.html > >> The download side of using JNI in these tests is that it complicates the >> setup a bit for those that run jtreg directly and/or just build the JDK >> and not the test libraries. You could reduce this burden a bit by >> limiting the load library/isMusl check to Linux only, meaning isMusl >> would not be called on other platforms. >> >> The alternative you suggest above might indeed be better. I assume you >> don't mean splitting the tests but rather just adding a second @test >> description so that the vm.musl case runs the test with a system >> property that allows the test know the expected load library path behavior. > > I have updated the PR to split the two tests in multiple @test s. > >> The updated comment in java_md.c in this looks good. A minor comment on >> Platform.isBusybox is Files.isSymbolicLink returning true implies that >> the link exists so no need to check for exists too. Also the >> if-then-else style for the new class in ProcessBuilder/Basic.java is >> inconsistent with the rest of the test so it stands out. > > Thank you, these changes are done in the updated PR. > >> Given the repo transition this weekend then I assume you'll create a PR >> for the final review at least. Also I see JEP 386 hasn't been targeted >> yet but I assume Boris, as owner, will propose-to-target and wait for it >> to be targeted before it is integrated. > > Yes. How can this be best accomplished with the new git workflow? > - we can continue the review process till the end and I will request the > integration to happen only after the JEP is > targeted. I guess this step is now done by typing "slash integrate" in a > comment. > - we can pause the review process now until the JEP is targeted. > > In the first case I'm kindly asking the Reviewers who already chimed in on > that to re-confirm the review here. Aleksei Voitylov has updated the pull request incrementally with one additional commit since the last revision: JDK-8247589: Implementation of Alpine Linux/x64 Port - Changes: - all: https://git.openjdk.java.net/jdk/pull/49/files - new: https://git.openjdk.java.net/jdk/pull/49/files/f61f546a..d5994cb5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=49&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=49&range=00-01 Stats: 19 lines in 4 files changed: 7 ins; 4 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/49.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/49/head:pull/49 PR: https://git.openjdk.java.net/jdk/pull/49
