Unsigned overflow in g1Policy.cpp

Filed on behalf of William Kemper, <kemperw@amazon.com>.

At the end of a cycle, G1 updates a statistic based on the number of bytes copied during the cycle. The statistic is used to select the number of regions that can be collected without exceeding the pause time goal. G1 computes the bytes copied as the difference between _collection_set->bytes_used_before() - freed_bytes and assigns the result to an unsigned value. In some cases, freed_bytes is larger than _collection_set->bytes_used_before() which ends up as a very large unsigned value. When this happens, G1 believes the cost to copy bytes (measured in ms per byte) is much lower than it actually is. In this case, G1 may select more regions than it should for collection. See the code in JDK 11's g1Policy.cpp, line 672 in G1Policy::record_collection_pause_end. And the code in JDK 8's g1CollectorPolicy.cpp, line 1136.

Steps to reproduce: It happens very rarely, but the bug is clear (checking that unsigned value > 0).

This code has changed significantly on tip, so I don't think a backport makes sense.

Here is a suggested patch:

--- a/src/src/hotspot/share/gc/g1/g1Policy.cpp
+++ b/src/src/hotspot/share/gc/g1/g1Policy.cpp
@@ -666,11 +666,10 @@ void G1Policy::record_collection_pause_end(double pause_time_ms, size_t cards_sc
     _analytics->report_rs_length_diff((double) rs_length_diff);
 
     size_t freed_bytes = heap_used_bytes_before_gc - cur_used_bytes;
- size_t copied_bytes = _collection_set->bytes_used_before() - freed_bytes;
- double cost_per_byte_ms = 0.0;
-
- if (copied_bytes > 0) {
- cost_per_byte_ms = average_time_ms(G1GCPhaseTimes::ObjCopy) / (double) copied_bytes;
+ if (_collection_set->bytes_used_before() > freed_bytes) {
+ size_t copied_bytes = _collection_set->bytes_used_before() - freed_bytes;
+ double average_copy_time = average_time_ms(G1GCPhaseTimes::ObjCopy);
+ double cost_per_byte_ms = average_copy_time / (double) copied_bytes;
       _analytics->report_cost_per_byte_ms(cost_per_byte_ms, collector_state()->mark_or_rebuild_in_progress());
     }