RFR: 8183281: Remove unnecessary call to increment_gc_time_stamp

[Stefan Johansson]

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 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;