October 2018 - hotspot-gc-dev

RFR: 8212921: Relax code cache oop verification in ZGC
by Erik Österlund 31 Oct '18

31 Oct '18

Hi, With concurrent class unloading, the normal state of affairs for ZGC is that nmethods have messed up oops in all GC safepoints, and fix them up during concurrent phases. Therefore, Universe::verify needs to relax the assumption that oops are okay for ZGC. Note that during Universe::verify, the oops in the nmethods were redundantly verified. Once by walking the oops in the code cache, and once when calling nmethod::verify(), which did the same verification again. I removed the latter. Webrev: http://cr.openjdk.java.net/~eosterlund/8212921/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8212921 Thanks, /Erik

2 3

RFR (S): 8212911: Unify and micro-optimize handling of non-in-collection set references in oop closures
by Thomas Schatzl 30 Oct '18

30 Oct '18

Hi all, can I have reviews for this small change that does some minor cleanup and micro-optimization of our main g1 stw/concurrent gc closures? - at the moment G1 randomly either adds a reference directly to the remembered set or puts it into the DCQS. I propose to, at least for closures called during stw to always put the reference into the DCQS and let the concurrent phase (or the next GC phase) deal with that as the current remembered set implementation is pretty bad. We may or may not get to fixing this for 12, but for now it seems to be better to do so. (No significant benchmark differences except for one 3% improvement of specjvm2008-xml, but overall it seems to make no difference) - enqueuing references always checks whether the from-region is in young to skip them. For that purpose, the closures get passed the from- region. Now the problem with that is that actually we do not need the from-region, but the result that the from-region is a young region (then we do not need to push that reference onto the DCQS), so the code calculates that result, i.e. from.is_young(), for every such reference. However at the caller of oop_iterate(), we often already know whether the from region is young (from other calculations), or specifically determine the from-region just for the iteration. The idea here is to pass from->is_young() directly, so we do not need to recalculate it for every reference. Now for the G1ScanEvacuatedObjClosure one could imagine to specialize the closure itself based on the from-region (e.g. having both a G1ScanEvacuatedObjFromYoungClosure and G1ScanEvacuatedObjFromOldClosure), however that results in code duplication at the caller (at least without splitting the task queue) the need for exactly that distinction there as well. Further, measurements did not yield a difference, so I kept the code a little more compact for now (there is an RFE out for this). We may re-evalute the latter as soon as Scan RS time variance has been improved a bit (for which this change helps a bit, and I have some changes in the pipeline. Actually this change has been broken out from these). - unify on using HeapRegion::is_in_same_region() instead of comparing HeapRegion* to avoid the table lookup to get the to-HeapRegion* as the memory load is presumably slower than some arithmetic on two local values. But the main driver for that change was that it allows just passing the is_young() result directly. CR: https://bugs.openjdk.java.net/browse/JDK-8212911 Webrev: http://cr.openjdk.java.net/~tschatzl/8212911/webrev/ Testing: hs-tier1-3, several hs-tier1-5 runs with other changes, benchmark suite etc. Thanks, Thomas

3 6

RFR (S): 6490394: G1: Allow heap shrinking / memory unmapping after reclaiming regions during Remark
by Thomas Schatzl 30 Oct '18

30 Oct '18

Hi all, allow me to provide a fix for the oldest g1-specific open issue on the bug tracker (from 2006 :)): resize the heap (eg. uncommit) according to existing sizing policies at remark time. It does exactly what the subject of the CR says. The feature is enabled by default - I did not see any reason to disable it as the functionality seems to actually be what would be expected of the concurrent cycle (as it mirrors the functionality of a full gc). Needs the change for JDK-8071913 currently out for review applied to work (i.e. not crash after uncommit). The current work for "JDK-8204089: Timely Reducing Unused Committed Memory" also depends on this change. CR: https://bugs.openjdk.java.net/browse/JDK-6490394 Webrev: http://cr.openjdk.java.net/~tschatzl/6490394/webrev/ Testing: hs-tier1-5, jdk-tier1-3, perf testing ongoing but not expecting a difference Thanks, Thomas

3 6

RFR (S): 8211388: Make OtherRegionsTable independent of the region it is for
by Thomas Schatzl 30 Oct '18

30 Oct '18

Hi, the OtherRegionsTable is a set of a set of cards that comprises the remembered set. It stores the HeapRegion it collects cards for to do filtering in the FromCardCache. The set of cards should not have knowledge about what region it is for, from an abstraction POV it should just be a dumb container of cards. Move out the HeapRegion id from OtherRegionsTable (the "set of cards") into the enclosing HeapRegionRememberedSet (managing set of cards), separating the FCC functionality from the raw data storage. This will make code that assigns a HeapRegionRememberedSet to multiple regions in the future a bit simpler too. CR: https://bugs.openjdk.java.net/browse/JDK-8211388 Webrev: http://cr.openjdk.java.net/~tschatzl/8211388/webrev/ Testing: hs-tier1-3 Thanks, Thomas

3 6

RFR (M): 8071913: Filter out entries to free/uncommitted regions during iteration
by Thomas Schatzl 30 Oct '18

30 Oct '18

Hi all, can I have reviews for this change that makes sure that remembered set entries to free or uncommitted regions are not touched during GC? In order to implement uncommitting regions at remark we need to make sure that these regions and their corresponding side data structures are not touched by GC any more. This change adds a simple filter in the remembered set iterator plus some careful changes in various locations of the code to avoid just that. There are alternatives to this changes that have been considered but ultimately rejected: - drop remembered sets when freeing regions: this would require us to scan all remembered sets for such regions which is slow. - do the filtering directly in the iterator: while this has been prototyped, and even gives a significant improvement in pause times (~5%), the change would be much larger (1500 LOC). Since I am currently reworking the remembered sets completely, I found it a waste of reviewer's time to let you do this review now, and shortly afterwards throw away all that code. I will incorporate the improvements into the new code though. :) If the remembered set changes won't make it e.g. for 12, we might consider taking these changes in at a later time. CR: https://bugs.openjdk.java.net/browse/JDK-8071913 Webrev: http://cr.openjdk.java.net/~tschatzl/8071913/webrev/ Testing: hs-tier1-5,jdk-tier1-3, perf tested, no change in pause times or (throughput) scores Quite a few gc tests do extensive commit/uncommit of regions with the corresponding exercise of the code. Thanks, Thomas

3 5

RFR(S) : 8157728 : Covert GCTimer_test to GTest
by Igor Ignatyev 29 Oct '18

29 Oct '18

http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/index.html > 450 lines changed: 238 ins; 211 del; 1 mod; Hi all, could you please review this small (and hopefully trivial) patch which converts internal GCTimer_test to gtest? webrev: http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/index.html JBS: https://bugs.openjdk.java.net/browse/JDK-8157728 testing: - converted tests on linux-x64, windows-x64, macosx-x64, solaris-sparcv9 in product and fastdebug variants - build w/ precompiled-headers enabled and disabled PS the patch has been originally created by Kirill Zh, but hasn't been sent out for official review Thanks, -- Igor

4 7

RFR (T): 8212974: Update RS Skipped cards uses wrong enum to register to phase
by Thomas Schatzl 29 Oct '18

29 Oct '18

Hi all, can I have a review for this trivial change that fixes a benign typo? During initialization of the phase time records the code uses the wrong enum to register the "Skipped Cards" to the "Update RS" logging structure. I.e. _gc_par_phases[UpdateRS]- >link_thread_work_items(_update_rs_skipped_cards, ScanRSSkippedCards); which should be _gc_par_phases[UpdateRS]- >link_thread_work_items(_update_rs_skipped_cards, UpdateRSSkippedCards); Since the value of ScanRSSkippedCards equals UpdateRSSkippedCards there is no harm. CR: https://bugs.openjdk.java.net/browse/JDK-8212974 Webrev: http://cr.openjdk.java.net/~tschatzl/8212974/webrev/ Testing: local compilation; there is no way to create a test for that for above reasons. Thanks, Thomas

2 2

RFR (XS): 8212766: TestPromotionEventWithG1.java failed due to "RuntimeException: PLAB size is smaller than object size."
by Thomas Schatzl 25 Oct '18

25 Oct '18

Hi, can I have reviews for this change that fixes a bug introduced in "JDK-8210492 PLAB object promotion events report object sizes in words"? In JDK-8210492, for some JFR event, the object size has been changed to be passed in bytes. However the JFR event gets passed another size, the PLAB size, which has been forgotten to be fixed accordingly. So it happens that PLAB size (in words) is compared to object size (in bytes) which fails if PLAB size happens accidentally to be smaller than object size. This change fixes that. CR: https://bugs.openjdk.java.net/browse/JDK-8212766 Webrev: http://cr.openjdk.java.net/~tschatzl/8212766/ Testing: Test fails with -XX:MinTLABSize=576 -XX:TLABSize=576 without the patch, passes otherwise. Thanks, Thomas

3 4

RFR (XS): 8212753: Improve oopDesc::forward_to_atomic
by Thomas Schatzl 24 Oct '18

24 Oct '18

Hi all, can I have reviews for this micro-optimization that de-clutters oopDesc::forward_to_atomic used in CMS and G1? Reasons for this change: - the mark is reloaded in the method although all collectors could pass in their current copy (the mark is a volatile member, so it can't be optimized away) - it checks whether the mark is already set (i.e. already forwarded) before doing the CAS, and after doing the CAS. The former is waste of time because forwarding undo-statistics show that the number of failures (i.e. an object has already been forwarded) is extremely rate. - the check after the CAS whether the mark is set is superfluous, because after the CAS, independent of whether it was successful or not, we know that the mark must have been forwarded. There is no particular performance change. Just removes needlessly executed code. (Just before somebody asks: the two asserts in the changed forward_to_atomic() method are conditionalized on CMS because it will CAS in a special value into the mark word that do not fit the rest of the condition to "reserve" it for old gen promotion). CR: https://bugs.openjdk.java.net/browse/JDK-8212753 Webrev: http://cr.openjdk.java.net/~tschatzl/8212753/webrev/ Testing: hs-tier1-3 Thanks, Thomas

3 6

RFR: 8196989: Revamp G1 JMX MemoryPool and GarbageCollector MXBean definitions
by Hohensee, Paul 24 Oct '18

24 Oct '18

Bug: https://bugs.openjdk.java.net/browse/JDK-8196989 CSR: https://bugs.openjdk.java.net/browse/JDK-8196991 Webrev: http://cr.openjdk.java.net/~phh/8196989/webrev.02/ As requested, I split the jstat counter update off from the MXBean update. This is the MXBean update. The jstat counter RFE is https://bugs.openjdk.java.net/browse/JDK-8210965 and its CSR is https://bugs.openjdk.java.net/browse/JDK-8210966. The MXBean CSR is in draft state, I’d greatly appreciate review and sign-off. It’s been suggested that we add another pool to represent the free region set, but doing so would be incompatible with existing MXBean use invariants for all GCs. These are: 1. The sum of the pools’ MemoryUsage.max properties is the total reserved heap size. 2. The sum of the pools’ MemoryUsage.committed properties is the total committed size. 3. The sum of the pools’ MemoryUsage.used properties is the total size of the memory containing objects, live and dead-and-yet-to-be-collected, as the case might be, plus intentional gaps between them. 4. The total free space is (sum of the max properties – sum of the used properties). 5. The total uncommitted space is (sum of the max properties – sum of the committed properties). 6. The total committed free space is (2) – (3). To keep invariants 1, 2 and 3, the free region pool’s “max” property should be “undefined” (i.e., -1). The intuitive, to me, “used” property value would be the total free space, but that would violate invariant 4 above. Defining the “committed” property as the total committed free space would violate invariants 2 and 6. The patch passes the submit repo, hotspot tier1, and, separately, the serviceability, jfr, and gc jtreg tests. I’m uncertain how to construct a test that checks for valid MXBean content: the existing tests don’t. Any such test will be fragile due to possible future Hotspot changes that affect the values, and to run-to-run variability. I’ve done by-hand comparisons between the old and new MXBean content using the SwingSet2 demo, including using App CDS, and the numbers look reasonable. The guts of the change are in G1MonitoringSupport::recalculate_sizes, initialize_serviceability, memory_managers, memory_pools, and G1MonitoringScope. I also defined TraceConcMemoryManagerStats to track the concurrent cycle in a way analogous to TraceCMSMemoryManagerStats. The changes to the includes in g1FullGCOopClosures.inline.hpp and g1HeapVerifier.cpp are to satisfy compiler complaints. I changed the 3rd argument to the G1MonitoringScope constructor to a mixed_gc flag, and use accessor methods instead of direct field accesses when accessor methods exist. I believe I’ve minimized the latter. I updated the copyright date to 2018 in memoryService.hpp because I neglected to do so in my previous G1 MXBean patch. Thanks, Paul

5 11