RFR: 8314651: G1: Fix -Wconversion warnings in static fields of HeapRegion [v2]

Albert Mingkun Yang ayang at openjdk.org
Tue Aug 29 13:21:36 UTC 2023


On Tue, 29 Aug 2023 08:16:33 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Albert Mingkun Yang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
>> 
>>  - review
>>  - Merge branch 'master' into g1-heap-region-log-uint
>>  - g1-heap-region-log-uint
>
> src/hotspot/share/gc/g1/g1Arguments.cpp line 137:
> 
>> 135:   uint region_size_log_mb = HeapRegion::LogOfHRGrainBytes > LOG_M
>> 136:                           ? HeapRegion::LogOfHRGrainBytes - LOG_M
>> 137:                           : 0;
> 
> I would just assert that `HeapRegion::LogOfHRGrainBytes >= LOG_M` here. People messing with with minimum region size would need to check this code anyway.
> 
> Further, `LOG_M` should probably be calculated as `log2_exact(HeapRegion::min_size())` instead of using the constant directly (and renamed appropriately, something like `Log[Of]MinHeapRegionSize/GrainBytes`).
> 
> Then there would be no need to do the maximum calculation (my original suggestion would have been `MAX2(HeapRegion::LogOfHRGrainBytes - LOG_M, 0u)` btw)

Checking how it's used around `G1RemSetArrayOfCardsEntriesBase`, `log_m` is more about 1MB. The very fact that min-heap-region is 1M seems an accident. Kept the original name and added assertion.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/15360#discussion_r1308804146


More information about the hotspot-gc-dev mailing list