RFR [L][4/7]: 8180415: Rebuild remembered sets during the concurrent cycle (original) (raw)
sangheon.kim sangheon.kim at oracle.com
Thu Mar 22 04:47:15 UTC 2018
- Previous message (by thread): RFR [L][4/7]: 8180415: Rebuild remembered sets during the concurrent cycle
- Next message (by thread): RFR [L][4/7]: 8180415: Rebuild remembered sets during the concurrent cycle
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Hi Thomas,
On 03/13/2018 07:54 AM, Thomas Schatzl wrote:
Hi all,
while working on some follow-up changes and discussing with StefanJ, there some issues came up: - the amount of marked bytes for humongous objects has been calculated incorrectly, regardless of TAMS value it always considered humongous objects as below tams. There is no actual impact except in gc+liveness log messages in that even if the humongous object were allocated during marking, it looked like it had been allocated before marking. This has been found by the sanity check at the end of the rebuilding in later changes where it has been made to work with humongous regions too due to other changes. - StefanJ mentioned that gc+remset+tracking log messages should have a gc id. Added. New webrevs: http://cr.openjdk.java.net/~tschatzl/8180415/webrev.2to3 (diff) http://cr.openjdk.java.net/~tschatzl/8180415/webrev.3 (full) webrev.3 looks good to me. I have some minor comments.
=========================================================== src/hotspot/share/gc/g1/g1AllocRegion.cpp
- Need to revert copyright update as there's no change.
=========================================================== src/hotspot/share/gc/g1/g1ConcurrentMark.cpp 1045 class G1UpdateRemSetTrackingAfterRebuild : public HeapRegionClosure {
- Couldn't have a constructor to pass G1CollectedHeap same as G1UpdateRemSetTrackingBeforeRebuild and use it from do_heap_region()?
=========================================================== src/hotspot/share/gc/g1/g1ConcurrentMark.hpp 143 static const size_t RegionMarkStatsCacheSize = 1024; 643 static const uint RegionMarkStatsCacheSize = 1024;
- There are 2 RegionMarkStatsCacheSize and only the latter one seems to be used. Or am I missing something?
623 // Rebuilds the remembered sets for chosen regions concurrently and in parallel to the application.
- Please ignore if I'm wrong. :) I guess you meant, processes with MT and concurrently with the application. Something like '... chosen regions in parallel and concurrently with the application.'?
=========================================================== src/hotspot/share/gc/g1/g1FullCollector.cpp
- Copyright year update
=========================================================== /src/hotspot/share/gc/g1/g1OopClosures.inline.hpp 145 HeapRegion* to = _g1->heap_region_containing(obj);
- Getting HeapRegionRemSet and using it seems better for 147,148,149 lines.
=========================================================== src/hotspot/share/gc/g1/g1RemSet.cpp 764 log_debug(gc,remset)("Humongous obj region %u marked %d start " PTR_FORMAT " region start " PTR_FORMAT " tams " PTR_FORMAT " tars " PTR_FORMAT, 842 "bot " PTR_FORMAT " ntams " PTR_FORMAT " TARS " PTR_FORMAT,
- Uppercase for tams and tars used in log message?
888 uint num_workers = workers->active_workers();
- Need one more space before 'uint'.
=========================================================== src/hotspot/share/gc/g1/heapRegionRemSet.hpp 135 bool add_reference(OopOrNarrowOopStar from, uint tid);
- It would be helpful to mention when it returns true/false. I guess returns true when we add a reference.
- I couldn't see returning true in add_reference(). Am I missing something? I know we are not using the return value so no problem so far. Or heapRegionRemSet.cpp:line 432 should return true? 432 return false; (heapRegionRemSet.cpp)
=========================================================== src/hotspot/share/logging/logPrefix.hpp
- Copyright year update
=========================================================== test/hotspot/jtreg/gc/concurrent_phase_control/TestConcurrentPhaseControlG1Basics.java 2 * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
- Should be '2017, 2018'
=========================================================== src/hotspot/share/gc/g1/g1RemSetTrackingPolicy.cpp 36 bool G1RemSetTrackingPolicy::needs_scan_for_rebuild(HeapRegion* r) const{
- Need a space between const and {
85 if ((total_live_bytes > 0) && 86 (is_interesting_humongous_region(r) || CollectionSetChooser::region_occupancy_low_enough_for_evac(total_live_bytes)) && 87 !r->rem_set()->is_tracked()) {
- Maybe I'm wrong but Line 86 and 87 seem to need more spaces to align with line 85.
=========================================================== src/hotspot/share/gc/g1/g1RemSetTrackingPolicy.hpp 52 // pause. Called at safepoint in the remark pause.
- cleanup pause
Thanks, Sangheon
Thanks, Thomas On Thu, 2018-03-08 at 12:49 +0100, Thomas Schatzl wrote: Hi Stefan,
On Thu, 2018-03-08 at 10:15 +0100, Stefan Johansson wrote: Thanks for addressing my initial comments Thomas,
On 2018-03-07 09:59, Thomas Schatzl wrote: Hi all,
Stefan had some comments about the HeapRegionType parameter in the G1RemSetTrackingPolicy::updateatallocate() method - that one is not really required when placing the call to this method correctly and just using the HeapRegion's type directly. Changing this removes another 40 LOC of changes. There has been another bug introduced by me during cleaning up for final review: G1RemSetTrackingPolicy::updateatfree() is empty in this change, which is wrong for this change - in this change we still do not free the remembered sets during the cleanup pause, only in the Concurrent Cleanup phase. When doing this concurrently, an assert triggers when setting the remembered set state to empty outside a safepoint. The fix is to make the remembered set untracked during the Cleanup phase still. New webrevs: http://cr.openjdk.java.net/~tschatzl/8180415/webrev.0to1 (diff) http://cr.openjdk.java.net/~tschatzl/8180415/webrev.1 (full) The change looks good. Just some additional comments: src/hotspot/share/gc/g1/g1ConcurrentMark.cpp 1255 GCTraceTime(Debug, gc)("Complete Remembered Set Tracking"); ... 1291 GCTraceTime(Debug, gc)("Finalize Concurrent Mark Cleanup"); Please add the "phases" tag to the above logging to be more consistent with the remark and other pauses. --- src/hotspot/share/gc/g1/g1FullGCOopClosures.inline.hpp 52 template inline void G1AdjustClosure::adjustpointer(T* p) In this method the comments should be updated since we no longer return anything. --- src/hotspot/share/gc/g1/g1RemSet.cpp 757 HeapRegion* startregion = hr- humongousstartregion(); startregion is only used to get bottom and crate an oop, so maybe instead create the oop directly, like: oop humongous = oop(hr->humongousstartregion()->bottom()); 761 // A humongous object is live (with respect to the scanning) either 762 // a) it is marked on the bitmap as such 763 // b) its TARS is larger than nTAMS, i.e. has been allocated during marking. 764 if (cm->nextmarkbitmap()->ismarked(startregion- bottom()) || 765 (topatrebuildstart > hr- nexttopatmarkstart())) { Break this check out to get something like: islive(humongous, tars, ntams). Fixed. There is also the markedbytes counting in this method that makes the code a bit harder to follow. I've discussed this with Thomas offline and he has an idea of how to avoid this. As discussed I would like to postpone this for now as it is much harder than it looks and warrants its own CR (actually two or three). Also in G1FullCollector::phase3adjustpointers() there is a GCTraceTime that needs updating, since we no longer rebuild remember sets. All done. New webrevs: http://cr.openjdk.java.net/~tschatzl/8180415/webrev.1to2/ (diff) http://cr.openjdk.java.net/~tschatzl/8180415/webrev.2/ (full) Thomas
- Previous message (by thread): RFR [L][4/7]: 8180415: Rebuild remembered sets during the concurrent cycle
- Next message (by thread): RFR [L][4/7]: 8180415: Rebuild remembered sets during the concurrent cycle
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]