September 2020 - hotspot-gc-dev

Re: RFR: 8247281: migrate ObjectMonitor::_object to OopStorage [v2]
by Daniel D.Daugherty 14 Sep '20

14 Sep '20

On Mon, 14 Sep 2020 18:33:17 GMT, Daniel D. Daugherty <dcubed(a)openjdk.org> wrote: >> @dholmes-ora and @fisk - I've taken a first pass at creating a CSR: >> JDK-8253121 migrate ObjectMonitor::_object to OopStorage >> https://bugs.openjdk.java.net/browse/JDK-8253121 >> >> Please look it over and feel free to edit as needed. Since I don't do >> CSR's often, what I've done might be all wrong. :-) > > @kimbarrett: > >> src/hotspot/share/oops/weakHandle.cpp >> 36 WeakHandle::WeakHandle(OopStorage* storage, oop obj) : >> 37 _obj(storage->allocate()) { >> 38 assert(obj != NULL, "no need to create weak null oop"); >> >> Please format this differently so the ctor-init-list is more easily >> distinguished from the body. I don't care that much which of the several >> alternatives is used. > > After discussion with Erik, I changed the indent on L37 from two space to four spaces. @kimbarrett > src/hotspot/share/runtime/objectMonitor.cpp > 244 // Check that object() and set_object() are called from the right context: > 245 static void check_object_context() { > > This seems like checking we would normally only do in a debug build. Is this > really supposed to be done in product builds too? (It's written to support > that, just wondering if that's really what we want.) Maybe these aren't > called very often so it doesn't matter? I also see that guarantee (rather > than assert) is used a fair amount in this and related code. I've changed check_object_context() to only be defined and called when ASSERT is defined. I've also changed the guarantee() calls to assert() calls. I've done a couple of Mach5 Tier[1-8] test cycles on this code so I'm no longer worried about this code or its callers in release bits. ------------- PR: https://git.openjdk.java.net/jdk/pull/135

3 8

Re: RFR: 8247281: migrate ObjectMonitor::_object to OopStorage [v2]
by Daniel D.Daugherty 14 Sep '20

14 Sep '20

On Mon, 14 Sep 2020 13:47:49 GMT, Daniel D. Daugherty <dcubed(a)openjdk.org> wrote: >> @dholmes-ora - Thanks for the review. I've think I have made all the comments >> you were concerned about as unresolved. I agree that we need a CSR for the >> change in behavior and I'll look into getting that going. > > @kimbarrett - Thanks for the review. I believe @fisk is going to > address your comments. @rkennke - Thanks for the review. I believe @fisk is going to address your comments. ------------- PR: https://git.openjdk.java.net/jdk/pull/135

3 8

Re: RFR: 8247281: migrate ObjectMonitor::_object to OopStorage [v2]
by Coleen Phillimore 14 Sep '20

14 Sep '20

On Fri, 11 Sep 2020 21:42:08 GMT, Daniel D. Daugherty <dcubed(a)openjdk.org> wrote: >> This RFE is to migrate the following field to OopStorage: >> >> class ObjectMonitor { >> <snip> >> void* volatile _object; // backward object pointer - strong root >> >> Unlike the previous patches in this series, there are a lot of collateral >> changes so this is not a trivial review. Sorry for the tedious parts of >> the review. Since Erik and I are both contributors to this patch, we >> would like at least 1 GC team reviewer and 1 Runtime team reviewer. >> >> This changeset was tested with Mach5 Tier[1-3],4,5,6,7,8 testing >> along with JDK-8252980 and JDK-8252981. I also ran it through my >> inflation stress kit for 48 hours on my Linux-X64 machine. > > Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision: > > coleenp CR - changes to resolve Coleen's comments. I approve of this without seeing any new changes. src/hotspot/share/runtime/thread.cpp line 4593: > 4591: // are used in om_flush(). > 4592: BarrierSet::barrier_set()->on_thread_detach(p); > 4593: One last question, that doesn't require a comment why it's here, but why was this moved? ------------- Marked as reviewed by coleenp (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/135

3 5

RFR: 8253030: ZGC: Change ZMarkCompleteTimeout unit to microseconds
by Per Lidén 14 Sep '20

14 Sep '20

The ZMarkCompleteTimeout is currently specified in milliseconds, and its value is 1 (i.e. as low as it can be). In preparation for using a lower time out value than 1ms we should change the ZMarkCompleteTimeout unit to be microseconds, instead of milliseconds. ------------- Commit messages: - Mark timeout Changes: https://git.openjdk.java.net/jdk/pull/120/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=120&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253030 Stats: 18 lines in 4 files changed: 6 ins; 0 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/120.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/120/head:pull/120 PR: https://git.openjdk.java.net/jdk/pull/120

5 7

Re: RFR: 8252114: Windows-AArch64: Enable and test ZGC and ShenandoahGC [v2]
by Stefan Karlsson 11 Sep '20

11 Sep '20

On Thu, 10 Sep 2020 15:30:04 GMT, Monica Beckwith <mbeckwit(a)openjdk.org> wrote: >> ZGC and ShenandoahGC are two low latency GCs in OpenJDK HotSpot. We will enable and run microbenchmarks and scaling >> tests for both. After enabling, I ran JTRegs, a few micros, and SPECJBB2015 (heap and multi-JVM) scaling tests for both >> the GCs. > > Monica Beckwith has updated the pull request incrementally with one additional commit since the last revision: > > x$OPENJDK_TARGET_CPU" = "xaarch64" > > Incorporating Aleksey's comments to incorporate 'x' Marked as reviewed by stefank (Reviewer). ------------- PR: https://git.openjdk.java.net/jdk/pull/97

1 0

Re: RFR: 8252114: Windows-AArch64: Enable and test ZGC and ShenandoahGC [v2]
by Aleksey Shipilev 11 Sep '20

11 Sep '20

On Thu, 10 Sep 2020 15:30:04 GMT, Monica Beckwith <mbeckwit(a)openjdk.org> wrote: >> ZGC and ShenandoahGC are two low latency GCs in OpenJDK HotSpot. We will enable and run microbenchmarks and scaling >> tests for both. After enabling, I ran JTRegs, a few micros, and SPECJBB2015 (heap and multi-JVM) scaling tests for both >> the GCs. > > Monica Beckwith has updated the pull request incrementally with one additional commit since the last revision: > > x$OPENJDK_TARGET_CPU" = "xaarch64" > > Incorporating Aleksey's comments to incorporate 'x' The check looks fine now. ------------- Marked as reviewed by shade (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/97

1 1

Re: RFR: 8027545: Improve object array chunking test in G1's copy_to_survivor_space
by Stefan Johansson 11 Sep '20

11 Sep '20

On Wed, 9 Sep 2020 07:24:27 GMT, Kim Barrett <kbarrett(a)openjdk.org> wrote: > This is rework of initial change from before the transition to git. > The initial RFR email is attached below. > > The primary change is to limit the number of partial array tasks in > the queues for any given array. The original change just split up an > array into N tasks that were all enqueued at once. But for a large > array this could be a lot of tasks, leading to significant and > unnecessary queue expansion. Instead we now limit the number of tasks > for a given array based on the number of workers, and only gradually > add tasks up to that limit. This gives other threads an opportunity > to steal such tasks, while still keeping queue growth under control. > > Most of the calculation for this is handled by a new helper class, so > this can later be shared with ParallelGC. > > The dispatch on array klass type for has also been changed. It now > affirmatively breaks Project Valhalla, rather than quietly doing > something that probably isn't what is actually wanted. I'll discuss > with them so there is a plan for dealing with it when they take this > update. > > Ran tier1-6 in mach5 and some local stress tests. > > Performance testing was unchanged from previous, except I wasn't able > to reproduce the small specjbb2015 critical-jops improvement > previously seen on one platform. My suspicion is that improvement was > a statistical abberation. > > --- Initial RFR email --- > > RFR: 8158045: Improve large object handling during evacuation > RFR: 8027761: Investigate fast-path for scanning only objects with references during gc > RFR: 8027545: Improve object array chunking test in G1's copy_to_survivor_space > > Please review this change to type dispatching and handling in G1's > evacuation copying, in order to improve the hot paths and improve array > handling. This change addresses several closely co-located enhancement > requests; it seemed difficult to split them up in a sensible way. > > do_copy_to_survivor_space now gets the klass of the object being copied > once, up front, for use in multiple places. This avoids fetching (including > re-decoding when compressed) the klass multiple times. This addresses part > of JDK-8027545. > > Moved check for and handling of string deduplication later, only applying it > after the special array cases have been dealt with, since strings are not > arrays. (They are header objects pointing to an array of character values.) > > Special case typeArray, doing nothing other than the copy, since they > contain no oops that need to be processed. This addresses JDK-8027761. > > Changed handling of objArray, pushing all of the partial array tasks up > front, rather than processing the current chunk after pushing a single task > for the remaining work. This addresses JDK-8158045. > > As part of these, cached some more frequently accessed values in > G1ParScanThreadState member variables. This addresses part of part of > JDK-8027545. > > While both the old and new code will work for Project Valhalla, the handling > of arrays should be updated for that project, which introduces new array > types. > > Deleted a lingering reference to G1ParScanPartialArrayClosure that was > deleted long ago (JDK-8035330, JDK 9). > > CR: > https://bugs.openjdk.java.net/browse/JDK-8158045 > https://bugs.openjdk.java.net/browse/JDK-8027761 > https://bugs.openjdk.java.net/browse/JDK-8027545 > > Webrev: > https://cr.openjdk.java.net/~kbarrett/8158045/open.00/ > > Testing: > tier1-3 > > performance testing - seems to be at worst performance neutral, with a > statistically significant 3% improvement in specjbb2015 critical-jops seen > on one platform. Overall a very nice improvement. Some comments and ideas below. src/hotspot/share/gc/g1/g1ParScanThreadState.hpp line 84: > 82: bool _old_gen_is_full; > 83: // Size (in elements) of a partial objArray task chunk. > 84: int _partial_objarray_chunk_size; I would prefer to skip the "obj"-part here to have more consistent naming or, as mentioned above, include it in the stepper instead. src/hotspot/share/gc/g1/g1ParScanThreadState.hpp line 165: > 163: private: > 164: inline void do_partial_array(PartialArrayScanTask task); > 165: inline void start_partial_objArray(G1HeapRegionAttr dest_dir, oop from, oop to); Same here, drop "obj" for consistent naming and avoiding the camel-case. src/hotspot/share/gc/shared/partialArrayTaskStepper.cpp line 34: > 32: // that maximizes available parallelism. > 33: return n_workers; > 34: } In preparation for a more advanced logic for the limit, or why not just use the input at the call-site? src/hotspot/share/gc/shared/partialArrayTaskStepper.inline.hpp line 71: > 69: // The from-space object contains the real length. > 70: int length = from->length(); > 71: assert(start < length, "invariant: start %d, length %d", start, length); Just so I'm not missing anything. There will never be more tasks than chunks left, so there is no risk that we have a race for the last chunk and thus hit this assertion? src/hotspot/share/gc/g1/g1ParScanThreadState.cpp line 68: > 66: _old_gen_is_full(false), > 67: _partial_objarray_chunk_size(ParGCArrayScanChunk), > 68: _partial_array_stepper(n_workers), What do you think about saving the chunk size in the stepper instead? Then we don't need to pass it in to `start()` and `next()`. To avoid needing it for the call to `oop_iterate_range()` we could instead have the `Step` include both the start and end index. src/hotspot/share/gc/shared/partialArrayTaskStepper.hpp line 57: > 55: int _index; // Array index for the step. > 56: uint _ncreate; // Number of new tasks to create. > 57: }; I wouldn't mind having getters for those, but it's not a hard request :) src/hotspot/share/gc/shared/partialArrayTaskStepper.inline.hpp line 103: > 101: uint ncreate = MIN2(_task_fannout, MIN2(remaining_tasks, _task_limit + 1) - pending); > 102: Step result = { start, ncreate }; > 103: return result; Similar to my comment above, what do you think about trying to write some test for this to verify we never get to many "tasks". ------------- Changes requested by sjohanss (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/90

3 9

Re: RFR: 8252114: Windows-AArch64: Enable and test ZGC and ShenandoahGC [v2]
by Monica Beckwith 11 Sep '20

11 Sep '20

On Thu, 10 Sep 2020 17:09:00 GMT, Monica Beckwith <mbeckwit(a)openjdk.org> wrote: >> This looks good to me with Alexsey's changes applied. > > Ref: https://github.com/openjdk/jdk/pull/97#issuecomment-690077743 > Hi David, > Thanks. I wanted to start a conversation here as it does modify common code (the aarch64-linux combo check) and also > wanted to highlight the difference between Shenandoah and ZGC checks. IMHO, Shenandoah checks are cleaner and if people > agree, I would like to apply those to ZGC as well. (When that happens, it *will* be outside of the windows-aarch64 port > and I will change the info in JBS accordingly) Please let me know what you think. Ref: https://github.com/openjdk/jdk/pull/97#issuecomment-690811308 I empathize with your comment, David. Here's a bit of a background for all - When I started the port, both ZGC, and Shenandoah had the "xlinux-aarch64" check whereas the "xx86" check was more elaborate for ZGC. At that time I chose the ZGC route for both the GCs and added separate checks for each OS. This is what the windows-aarch64 port has as well. (Since then Shenandoah has moved on to just checking the arch.) I already had a JBS entry, and the (partial) patch, I decided to use that for my first GitHub PR. and was hoping to start the conversation of the simpler route that Shenandoah took in the comments mentioning that as my preference and modify the JBS entry (and the patch) accordingly. Now coming back to this - @stooart-mon, do you think it is OK to just check for aarch64 (similar to Shenandoah)? @stefank what do you think of simplifying the x86 checks (similar to Shenandoah)? Thanks all. ------------- PR: https://git.openjdk.java.net/jdk/pull/97

1 0

Re: RFR: 8252114: Windows-AArch64: Enable and test ZGC and ShenandoahGC [v2]
by David Holmes 11 Sep '20

11 Sep '20

Hi Monica, On 11/09/2020 3:11 am, Monica Beckwith wrote: > Ref: https://github.com/openjdk/jdk/pull/97#issuecomment-690077743 > Hi David, > Thanks. I wanted to start a conversation here as it does modify common code (the aarch64-linux combo check) and also > wanted to highlight the difference between Shenandoah and ZGC checks. IMHO, Shenandoah checks are cleaner and if people > agree, I would like to apply those to ZGC as well. (When that happens, it *will* be outside of the windows-aarch64 port > and I will change the info in JBS accordingly) Please let me know what you think. I think you are going about this in a bit of a confusing way :) If you wanted to propose the ZGC feature check should be OS agnostic then you should have just proposed that rather than proposing a Windows-Aarch64 change that clearly belongs with the port work. You'll need to discuss the ZGC check with the ZGC folk on hotspot-gc-dev (now cc'd) but I'm pretty sure the OS check is deliberate, so that only those platforms known to actually work will be enabled. Thanks, David > ------------- > > PR: https://git.openjdk.java.net/jdk/pull/97 >

1 0

RFR: 8252995: Non-PCH builds broken by JDK-8250961
by Kim Barrett 10 Sep '20

10 Sep '20

8252995: Non-PCH builds broken by JDK-8250961 Please review this trivial fix for no-PCH build failures, introduced by JDK-8250961. Only one file _requires_ an additional #include. There are other files that were modified by that change that probably should have had an include added, on the "include what you use" theory. But in the interest of faster turn-around to fix the build failure, I'm not dealing with any of those in this change. Testing: local (linux-x64) no-pch build with shenandoah included mach5 builds-tier1 (in progress) ------------- Commit messages: - add missing include Changes: https://git.openjdk.java.net/jdk/pull/107/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=107&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8252995 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/107.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/107/head:pull/107 PR: https://git.openjdk.java.net/jdk/pull/107

4 4