RFR: 8183281: Remove unnecessary call to increment_gc_time_stamp

[Stefan Johansson]

On 2017-06-30 17:34, Erik Helin wrote:
> On 06/30/2017 01:53 PM, Stefan Johansson wrote:
>> Hi Erik,
>>
>> On 2017-06-30 11:37, Erik Helin wrote:
>>> Hi all,
>>>
>>> the following small patch removes an unnecessary call to
>>> increment_gc_time_stamp from
>>> G1CollectedHeap::do_collection_pause_at_safepoint (and the long,
>>> wrong, comment above the call).
>>>
>>> We already do a call increment_gc_time_stamp much earlier in
>>> do_collection_pause_at_safepoint, which is enough. The reasons
>>> outlined in the comment motivating a second call is no longer true,
>>> the code has changed (but the comment has not).
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8183281
>>> Patch: see below
>>> Testing: make hotspot
>>>
>> Patch looks good, but I would like to see some more testing than just
>> building hotspot. Running the gc jtreg tests for example.
>
> Thanks for reviewing! All pass for both fastdebug and product when 
> running `make test TEST=hotspot_gc` on my Linux workstation.
>
Thanks for running the tests, ship it!
StefanJ
> Thanks,
> Erik
>
>> Thanks for cleaning up the code,
>> Stefan
>>> Thanks,
>>> Erik
>>>
>>> # HG changeset patch
>>> # User ehelin
>>> # Date 1498814642 -7200
>>> #      Fri Jun 30 11:24:02 2017 +0200
>>> # Node ID 62400b3cbec4e0d06e0d6c21c9486070d8c906a4
>>> # Parent  10ccf0a5f63fdca04d9eda2c774ccdd0e12bc1a1
>>> 8183281: Remove unnecessary call to increment_gc_time_stamp
>>>
>>> diff -r 10ccf0a5f63f -r 62400b3cbec4
>>> src/share/vm/gc/g1/g1CollectedHeap.cpp
>>> --- a/src/share/vm/gc/g1/g1CollectedHeap.cpp    Thu Jun 29 19:09:04
>>> 2017 +0000
>>> +++ b/src/share/vm/gc/g1/g1CollectedHeap.cpp    Fri Jun 30 11:24:02
>>> 2017 +0200
>>> @@ -3266,29 +3266,6 @@
>>>
>>>          MemoryService::track_memory_usage();
>>>
>>> -        // In prepare_for_verify() below we'll need to scan the 
>>> deferred
>>> -        // update buffers to bring the RSets up-to-date if
>>> -        // G1HRRSFlushLogBuffersOnVerify has been set. While scanning
>>> -        // the update buffers we'll probably need to scan cards on the
>>> -        // regions we just allocated to (i.e., the GC alloc
>>> -        // regions). However, during the last GC we called
>>> -        // set_saved_mark() on all the GC alloc regions, so card
>>> -        // scanning might skip the [saved_mark_word()...top()] area of
>>> -        // those regions (i.e., the area we allocated objects into
>>> -        // during the last GC). But it shouldn't. Given that
>>> -        // saved_mark_word() is conditional on whether the GC time 
>>> stamp
>>> -        // on the region is current or not, by incrementing the GC 
>>> time
>>> -        // stamp here we invalidate all the GC time stamps on all the
>>> -        // regions and saved_mark_word() will simply return top() for
>>> -        // all the regions. This is a nicer way of ensuring this 
>>> rather
>>> -        // than iterating over the regions and fixing them. In 
>>> fact, the
>>> -        // GC time stamp increment here also ensures that
>>> -        // saved_mark_word() will return top() between pauses, i.e.,
>>> -        // during concurrent refinement. So we don't need the
>>> -        // is_gc_active() check to decided which top to use when
>>> -        // scanning cards (see CR 7039627).
>>> -        increment_gc_time_stamp();
>>> -
>>>          if (VerifyRememberedSets) {
>>>            log_info(gc, verify)("[Verifying RemSets after GC]");
>>>            VerifyRegionRemSetClosure v_cl;
>>