RFR [L][4/7]: 8180415: Rebuild remembered sets during the concurrent cycle
sangheon.kim
sangheon.kim at oracle.com
Thu Mar 22 04:47:15 UTC 2018
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.2_to_3 (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::update_at_allocate() 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::update_at_free() 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.0_to_1 (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 <class T> inline void
>>> G1AdjustClosure::adjust_pointer(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* start_region = hr-
>>>> humongous_start_region();
>>> start_region is only used to get bottom and crate an oop, so maybe
>>> instead create the oop directly, like:
>>> oop humongous = oop(hr->humongous_start_region()->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->next_mark_bitmap()->is_marked(start_region-
>>>> bottom()) ||
>>> 765 (top_at_rebuild_start > hr-
>>>> next_top_at_mark_start()))
>>> {
>>>
>>> Break this check out to get something like: is_live(humongous,
>>> tars,
>>> ntams).
>> Fixed.
>>
>>> There is also the marked_bytes 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::phase3_adjust_pointers() 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.1_to_2/ (diff)
>> http://cr.openjdk.java.net/~tschatzl/8180415/webrev.2/ (full)
>>
>> Thomas
>>
More information about the hotspot-gc-dev
mailing list