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