July 2019 - hotspot-gc-dev

RFR (XS): Optimize branch frequency of G1's write post-barrier in C2
by Man Cao 05 Aug '19

05 Aug '19

Hi all, Can I have reviews for this improvement for G1's write post-barrier? More details are stated on the JBS page. Highlight: it reduces CPU-cost-per-query by 1% for Google search frontend's production workload. Webrev: https://cr.openjdk.java.net/~manc/8225776/webrev.00/ RFE: https://bugs.openjdk.java.net/browse/JDK-8225776 Some notes: The RFE could be a duplicate of JDK-8130918. However, this patch does not improve the performance of the microbenchmark in JDK-8130918. I'm not sure if this patch fully addresses JDK-8130918. Chuck Rasbold helped me to figure out the proper fix for the basic block ordering by looking at the CFG before and after C2's PhaseBlockLayout. The out-most if branch (xor) has to have a frequency greater than 0.5 to make the BBs laid out correctly. It is also more conventional to use PROB_LIKELY_MAG than PROB_LIKELY in C2. -Man

3 5

Re: 8227226: Segmented array clearing for ZGC
by Sciampacone, Ryan 02 Aug '19

02 Aug '19

1) Yes this was a conscious decision. There was discussion on determining the optimal point for breakup but given the existing sizes this seemed sufficient. This doesn't preclude the ability to go down that path if its deemed absolutely necessary. The path for more complex decisions is now available. 2) Agree 3) I'm not clear here. Do you mean effectively going direct to ZHeap and bypassing the single function PinAllocating? Agree. Otherwise I'll ask you to be a bit clearer. 4) Agree 5) I initially had the exact same reaction but I played around with a few other versions (including breaking up initialization points between header and body to get the desired results) and this ended up looking correct. I'll try mixing in the mem clearer function again (fresh start) to see if it looks any better. On 7/8/19, 5:49 AM, "Per Liden" <per.liden(a)oracle.com> wrote: Hi Ryan, A few general comments: 1) It looks like this still only work for large pages? 2) The log_info stuff should be removed. 3) I'm not a huge fan of single-use utilities like PinAllocating, at least not when, IMO, the alternative is more straight forward and less code. 4) Please make locals const when possible. 5) Duplicating _do_zero looks odd. Injecting a "mem clearer", similar to what Stefans original patch did, seems worth exploring. cheers, /Per (Btw, I'm on vacation so I might not be super-responsive to emails) On 2019-07-08 12:42, Erik Österlund wrote: > Hi Ryan, > > This looks good in general. Just some stylistic things... > > 1) In the ZGC project we like the letter 'Z' so much that we put it in > front of everything we possibly can, including all class names. > 2) We also explicitly state things are private even though it's > bleedingly obvious. > > So: > > 39 class PinAllocating { > 40 HeapWord* _mem; > 41 public: -> 39 class ZPinAllocating { 40 private: 41 HeapWord* _mem; > 42 > 41 public: I can be your sponsor and push this change for you. I don't > think there is a need for another webrev for my small stylistic remarks, > so I can just fix that before pushing this for you. On that note, I'll > add me and StefanK to the contributed-by section as we all worked out > the right solution to this problem collaboratively. I have run through > mach5 tier1-5, and found no issues with this patch. Thanks, /Erik > > On 2019-07-05 17:18, Sciampacone, Ryan wrote: >> http://cr.openjdk.java.net/~phh/8227226/webrev.00/ >> https://bugs.openjdk.java.net/browse/JDK-8227226 >> >> This patch introduces safe point checks into array clearing during >> allocation for ZGC. The patch isolates the changes to ZGC as (in >> particular with the more modern collectors) the approach to >> incrementalizing or respecting safe point checks is going to be >> different. >> >> The approach is to keep the region holding the array in the allocating >> state (pin logic) while updating the color to the array after checks. >> >> Can I get a review? Thanks. >> >> Ryan >

5 14

RFR: 8048556: Unnecessary GCLocker-initiated young GCs
by Fairoz Matte 31 Jul '19

31 Jul '19

Hi, Please review the change to avoid Unnecessary GCLocker Initiated GC's. JBS: https://bugs.openjdk.java.net/browse/JDK-8048556 Webrev: http://cr.openjdk.java.net/~fmatte/8048556/webrev.00/ Background: This fix is proposed by "Tony Printezis" in the comments section. GC locker-initiated young GC happens immediately after another young GC. The fix is to send the right gc counts and fixing the vm gc op not to signal "full" Reading right GC count from "GC_locker::jni_unlock(JavaThread* thread)" method and provide implementations for GCH, PS, and G1 to get that count in Collect() method. I hope I have done justification to Tony's approach. I have modified a regression test (again from Tony's work) that verifies when "GCLocker Initiated GC" occurs, It runs gc only if young generation occupancy is > 75%. Thanks, Fairoz

4 11

RFR(S) 228777: Shenandoah: Cleanup STW weak root processing
by Zhengyu Gu 31 Jul '19

31 Jul '19

Shenandoah pre-evacuates or updates weak roots at STW pauses. Therefore, weak roots should not have forwarded references during mark phase. Current has_forwarded_objects() branch does not break things, but does unncessary works, let's prune it. Bug: https://bugs.openjdk.java.net/browse/JDK-8228777 Webrev: http://cr.openjdk.java.net/~zgu/JDK-8228777/webrev.00/ Test: hotspot_gc_shenandoah (fastdebug and release) Thanks, -Zhengyu

2 5

[8u] Request for backporting JDK-8131600
by chengjingwei 31 Jul '19

31 Jul '19

Hi, all Would someone help to approve the backport of JDK-8131600 to 8u? Bug: https://bugs.openjdk.java.net/browse/JDK-8131600 Here is the discussion on this issue: http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2015-July/014176.html This issue decreases the stability of our applications. It's always reproducible with jdk built from the latest jdk8u master repo.

2 1

RFR: JDK-8228775: Shenandoah: Remove useless null-input-verification in Shenandoah/C2 verifier
by Roman Kennke 30 Jul '19

30 Jul '19

Post-LRB it doesn't make a difference whether or not inputs to intrinsics have non-null-types or not because no barriers are feeding directly into intrinsics. We can remove the whole check. In sh/jdk11, nightlies are failing because of that, in jdk14 this is only a bunch of useless code. Bug: https://bugs.openjdk.java.net/browse/JDK-8228775 Webrev: http://cr.openjdk.java.net/~rkennke/JDK-8228775/webrev.00/ Testing: hotspot_gc_shenandoah Good? Roman

2 1

RFR (S) 8228672: [TESTBUG] gc/metaspace/TestSizeTransitions.java fails on 32-bit platforms
by Aleksey Shipilev 30 Jul '19

30 Jul '19

Bug: https://bugs.openjdk.java.net/browse/JDK-8228672 Fix: http://cr.openjdk.java.net/~shade/8228672/webrev.02/ Instead of duplicating @run sections even more, decided to test for Platform.is64bit, like other tests in vicinity do. Testing: affected test {x86_64, x86_32}, jdk-submit (running) -- Thanks, -Aleksey

2 4

RFR: 8228657: ZGC: ZObjectAllocator::used() should take undone allocations into account
by Per Liden 30 Jul '19

30 Jul '19

In ZObjectAllocator, when we allocate a page we increment _used. However, we might later undo that page allocation, but we then leave _used as is. This results in a sometimes slightly incorrect value being reported by ZCollectedHeap::tlab_used(), which is fed into the TLAB heuristics. This incorrectness is not that big of a deal, but we should still fix this. We can't safely decrement _used (because we might be executing on a different CPU now), but we can track the amount of undos we've done, and later subtract that from _used. Bug: https://bugs.openjdk.java.net/browse/JDK-8228657 Webrev: http://cr.openjdk.java.net/~pliden/8228657/webrev.0 /Per

2 2

Re: 8227226: Segmented array clearing for ZGC
by Hohensee, Paul 29 Jul '19

29 Jul '19

+1. Pretty much all the time the GC thread doesn't own the lines, so getting them into its cache(s) can produce coherence traffic, though that's usually through the LLC because there aren't many multi-socket systems in use. The main effect we've always seen when we turn on ZeroTLAB is LLC eviction of lines being actively used by other threads. Later when an app thread wants the lines, more coherence traffic might happen. ZeroTLAB hasn't been removed because every once in awhile someone enables it to see if current hardware has done something new to fix the problem. C2 goes to some trouble to eliminate zeroing by noticing when zero-stores are dead because of initializing stores in a constructor. For scalar and reference fields the zero-store usually goes away, but arrays are more difficult because their content is usually built up during app execution rather than in the constructor. Parenthetically, on the allocation side we'd love to have an machine instruction that allocates a cache line and zeros it rather than go to memory for it. Such an instruction could replace the current allocation prefetch scheme and significantly reduce off-chip memory bandwidth utilization. Thanks, Paul On 7/26/19, 4:58 PM, "hotspot-gc-dev on behalf of Andrew Haley" <hotspot-gc-dev-bounces(a)openjdk.java.net on behalf of aph(a)redhat.com> wrote: On 7/26/19 6:56 PM, Erik Osterlund wrote: > I like the direction of [clearing pages when they are reclaimed], as > I think it will help us down the road. Are you sure this will help? If the cache lines for a page are already in exclusive state (i.e. they've been written) when reclaimed this will be fine, otherwise you're generating extra cache coherence traffic. You're also evicting the cache of the GC thread by filling it with zeroed lines. I guess some careful benchmarking will tell us. -- Andrew Haley (he/him) Java Platform Lead Engineer Red Hat UK Ltd. <https://www.redhat.com> https://keybase.io/andrewhaley EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

1 0

Re: RFR: 8224661: Parallel GC: Use WorkGang (3: UpdateDensePrefixAndCompactionTask)
by Thomas Schatzl 28 Jul '19

28 Jul '19

Hi, http://cr.openjdk.java.net/~lkorinth/workgang/1/_8224661-Parallel-GC-Use-Wo… looks good. Thanks, Thomas ----- Original Message ----- From: leo.korinth(a)oracle.com To: kim.barrett(a)oracle.com Cc: hotspot-gc-dev(a)openjdk.java.net Sent: Wednesday, July 3, 2019 4:47:21 AM GMT -08:00 US/Canada Pacific Subject: Re: RFR: 8224661: Parallel GC: Use WorkGang (3: UpdateDensePrefixAndCompactionTask) On 03/07/2019 01:55, Kim Barrett wrote: >> On May 27, 2019, at 4:21 AM, Leo Korinth <leo.korinth(a)oracle.com> wrote: >> […] >> Enhancement: >> https://bugs.openjdk.java.net/browse/JDK-8224661 >> >> Webrev: >> http://cr.openjdk.java.net/~lkorinth/workgang/0/_8224661-Parallel-GC-Use-Wo… >> http://cr.openjdk.java.net/~lkorinth/workgang/0/all/ >> >> Testing (on the whole patch series): >> mach5 remote-build-and-test --build-profiles linux-x64,linux-x64-debug,macosx-x64,solaris-sparcv9,windows-x64 --test open/test/hotspot/jtreg/:hotspot_gc -a -XX:+UseParallelGC >> gc test suite >> >> Thanks, >> Leo > > http://cr.openjdk.java.net/~lkorinth/workgang/1/_8224661-Parallel-GC-Use-Wo… > http://cr.openjdk.java.net/~lkorinth/workgang/1/_8224661-Parallel-GC-Use-Wo… > > Looks good. > > A few minor comments. > > ------------------------------------------------------------------------------ > src/hotspot/share/gc/parallel/psParallelCompact.hpp > 33 #include "gc/shared/taskqueue.hpp" > > I don't see any changes to this file that need this #include. > Presumably it's needed by some .cpp files, but they should be doing > the #include. Old artifact from older version where I used the shared task queue, thanks for finding this, I will remove it as it is not needed. > ------------------------------------------------------------------------------ > src/hotspot/share/gc/parallel/psParallelCompact.cpp > 2467 guarantee(_backing_array != NULL, "alloc failure"); > > Not needed; NEW_C_HEAP_ARRAY uses EXIT_OOM for the allocation failure > mode. Good catch, copied from other code that did it, will fix. I have (also) created https://bugs.openjdk.java.net/browse/JDK-8227168 to remove the source from where I got it. And fix some related problems. > ------------------------------------------------------------------------------ > src/hotspot/share/gc/parallel/psParallelCompact.cpp > 2618 static void compaction_with_stealing_task(ParallelTaskTerminator* terminator, uint which) { > > s/which/worker_id/ will fix. > > ------------------------------------------------------------------------------ > src/hotspot/share/gc/parallel/psParallelCompact.cpp > 2684 TaskQueue task_queue(last_space_id*(active_gc_threads * PAR_OLD_DENSE_PREFIX_OVER_PARTITIONING + 1)); > > Some breadcrumbs to explain that calculation would be really helpful. > Maybe comments, or splitting up the calculation with helpful variable > names, or something. will add some comment. > > Anyway, I think it's correct, but harder to verify than it could be. > > ------------------------------------------------------------------------------ > Thanks, Leo

1 0