Hi Paul, Erik may not have time in the next few months to review such a large change. But it would also be better to do the changes in steps for other reviewers. Also see below.
On Mon, 2018-07-23 at 21:33 +0000, Hohensee, Paul wrote: > Please review. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8196989 I may have missed this in the previous discussion (which has been a while), but has there been any discussion about a "Free (Region) Space" for the committed but free regions? It seems a bit random to assign free region to the "old space", seemingly just a repeat of the current behavior (where everything has been put into "old gen"). Also, imho the second survivor space should preferably be dropped completely. :) > CSR: https://bugs.openjdk.java.net/browse/JDK-8196991 > Webrev: http://cr.openjdk.java.net/~phh/8196989/webrev.00 > > This webrev is marked ‘L’ because it’s a behavioral change (CSR in > draft state, may I have a review of that too please?) and because the > test change fanout is large. The actual code changes are ‘M’. > > Passes the submit repo, Hotspot tier1, the JFR gc event tests and any > other test set with ‘gc’ or ‘serviceability’ in the test directory > name. I found it difficult to verify the accuracy of the reported > values other than manually, since they can vary from run to run of > the same program. I’d appreciate suggestions for how to go about > writing accuracy tests. > > I set out originally to revamp only the MXBeans, but decided it would > be incomplete if I didn’t include the jstat counters and the output > of the GC.heap_info jcmd option. I can separate the latter two into > their own RFEs, but I find it easier understand it all in a single > webrev and hope the reviewers will too. > > The basic approach is to add the new memory pools and collectors, the > new jstat counters, and an archive region counter that stands in for > an actual archive region set. HeapRegionSets are disjoint, so One option would be to add a HeapRegionSet tailored for archives that does not check the disjoint-criteria (it is superficially used for verification only anyway) - we already have special classes/flags for different kinds of regions (humongous/free/old) in the HeapRegionSet hierarchy. > initially I tried to create a first-class archive region set (on the > same level as the humongous region set), but that idea foundered on > the fact that there’s too much code I don’t fully understand that > depends on archive regions being in the existing old region set. Probably to simplify the implementation of archive regions :) This is another option, and does not look too bad actually, we only need to check and change all HeapRegion::is_old() or HeapRegion::is_old_or_humongous() checks. Now we only need a good name for is_old_or_archive_or_humongous() because that one is a bit lengthy :) > Externally (i.e., in the MXBeans and the jstat counters), however, > the old region set doesn’t include archive regions (unless running in > legacy mode). > > I used CMS’s TraceCMSMemoryManagerStats class as the model for > TraceConcMemoryManagerStats, which latter collects statistics on > concurrent cycles. There are two STW pauses in each concurrent cycle: > they are recorded separately and count as two sun.gc.collector.2 > events. I would like to move away serviceability code from G1CollectedHeap.h/cpp as much as possible; e.g. it would be very nice to make G1MonitoringSupport the owner of all the serviceability related data. Also the _use_legacy_monitoring member should probably move there too. > The humongous and archive space committed and used values are always > identical, This is because, for some reason, G1 counts the memory filled with filler objects as "used". Other collectors don't. > hence they are always 100% used. You may have noticed that just recently we got a bug (https://bugs.open jdk.java.net/browse/JDK-8207200) filed against the G1 MXBeans because of races in the code particularly code to be not-racy. The reason is the really weird calculation of used/committed for eden space/survivor space/old gen and that the precondition written down in G1MonitoringSupport::recalculate_sizes() does not hold. G1 MemoryMXBeans basically fabricates some numbers as you might have noticed :), so in addition to fixing that issue with the race I am still working on improving the accuracy of the used values. Also, in course of this change I am considering removing some other backwards-bending in returned values for G1 (the mentioned and e.g. funky stuff like assuming that adding together max-capacities of the pools gives you total heap size). I have also a preliminary webrev for that at http://cr.openjdk.java.net /~tschatzl/8207200/webrev/ which unfortunately clashes a lot with your changes. The reason why it is a single webrev is because I am not finished yet - I tend to split it up in parts for much better reviewing at the very end only. Could we work together on first refactoring the code before adding new kinds of spaces to the MXBeans? Looking at this change and mine roughly the following issues would need to be resolved first: - find a solution for archive regions as suggested above :) At the moment, without doing the change, I would tend to make archive regions separate from old regions. - move serviceability stuff as much as possible to g1MonitoringSupport - clean up MemoryPool, remove duplicate information - provide and return sane memory pool used/committed values to the MXBeans - clean up G1MonitoringSupport, e.g. avoid "*used/*committed" variables for every single memory pool. Use MemoryUsage structs for them. Make reading of memory pool information atomic wrt to its readers (note that I think it is currently just impossible to get consistent output for other statistics like jstat) - that's JDK-8207200. - add whatever serviceability stuff for the new pools/jstat/* in steps. > The revised output of jcmd GC.heap_info is in > G1CollectedHeap::print_on(). > I fixed a typo in src/hotspot/share/gc/g1/g1Policy.hpp by changing > the result type of young_list_target_length() from size_t to uint, > which latter is the type of the _young_list_target_length member. > I updated the copyright date in > src/hotspot/share/services/memoryService.hpp to 2018, as I neglected > to do so in a previous push. Comments? Thanks, Thomas
