RFR: 8264027: Refactor "CLEANUP" region printing

[Thomas Schatzl-4]

Hi all,

  can I have reviews for this small cleanup that refactors "CLEANUP" region status printing?

- where managing a `FreeRegionList` is not necessary, inline the call to the method (which is guarded by a check that if logging is not enabled, we do not print anything anyway)
- for the single remaining case, create a helper method in `G1HRPrinter`

The first item has been done as per the suggestion from @kstefanj [here](https://github.com/openjdk/jdk/pull/3154#pullrequestreview-619061961) ; the alternative would be to keep the free region lists and call `G1HRPrinter::cleanup` on it later, which I actually initially implemented. I could not find a difference performance-wise.

Testing: checked if there is some perf difference on a heap with 50k regions; tier1-3

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

Commit messages:
 - Initial implementation, move cleanup printing to closures

Changes: https://git.openjdk.java.net/jdk/pull/3243/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3243&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264027
  Stats: 71 lines in 4 files changed: 44 ins; 21 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3243.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3243/head:pull/3243

PR: https://git.openjdk.java.net/jdk/pull/3243

[Kim Barrett-2]

On Mon, 29 Mar 2021 12:43:12 GMT, Thomas Schatzl <[hidden email]> wrote:

> Hi all,
>
>   can I have reviews for this small cleanup that refactors "CLEANUP" region status printing?
>
> - where managing a `FreeRegionList` is not necessary, inline the call to the method (which is guarded by a check that if logging is not enabled, we do not print anything anyway)
> - for the single remaining case, create a helper method in `G1HRPrinter`
>
> The first item has been done as per the suggestion from @kstefanj [here](https://github.com/openjdk/jdk/pull/3154#pullrequestreview-619061961) ; the alternative would be to keep the free region lists and call `G1HRPrinter::cleanup` on it later, which I actually initially implemented. I could not find a difference performance-wise.
>
> Testing: checked if there is some perf difference on a heap with 50k regions; tier1-3

Looks good.

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

Marked as reviewed by kbarrett (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3243

[Albert Mingkun Yang]

On Mon, 29 Mar 2021 12:43:12 GMT, Thomas Schatzl <[hidden email]> wrote:

> Hi all,
>
>   can I have reviews for this small cleanup that refactors "CLEANUP" region status printing?
>
> - where managing a `FreeRegionList` is not necessary, inline the call to the method (which is guarded by a check that if logging is not enabled, we do not print anything anyway)
> - for the single remaining case, create a helper method in `G1HRPrinter`
>
> The first item has been done as per the suggestion from @kstefanj [here](https://github.com/openjdk/jdk/pull/3154#pullrequestreview-619061961) ; the alternative would be to keep the free region lists and call `G1HRPrinter::cleanup` on it later, which I actually initially implemented. I could not find a difference performance-wise.
>
> Testing: checked if there is some perf difference on a heap with 50k regions; tier1-3

Marked as reviewed by ayang (Committer).

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

PR: https://git.openjdk.java.net/jdk/pull/3243

[Thomas Schatzl-4]

On Tue, 30 Mar 2021 03:14:41 GMT, Kim Barrett <[hidden email]> wrote:

>> Hi all,
>>
>>   can I have reviews for this small cleanup that refactors "CLEANUP" region status printing?
>>
>> - where managing a `FreeRegionList` is not necessary, inline the call to the method (which is guarded by a check that if logging is not enabled, we do not print anything anyway)
>> - for the single remaining case, create a helper method in `G1HRPrinter`
>>
>> The first item has been done as per the suggestion from @kstefanj [here](https://github.com/openjdk/jdk/pull/3154#pullrequestreview-619061961) ; the alternative would be to keep the free region lists and call `G1HRPrinter::cleanup` on it later, which I actually initially implemented. I could not find a difference performance-wise.
>>
>> Testing: checked if there is some perf difference on a heap with 50k regions; tier1-3
>
> Looks good.

Thanks @kimbarrett @albertnetymk for your reviews.

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

PR: https://git.openjdk.java.net/jdk/pull/3243

[Thomas Schatzl-4]

On Mon, 29 Mar 2021 12:43:12 GMT, Thomas Schatzl <[hidden email]> wrote:

> Hi all,
>
>   can I have reviews for this small cleanup that refactors "CLEANUP" region status printing?
>
> - where managing a `FreeRegionList` is not necessary, inline the call to the method (which is guarded by a check that if logging is not enabled, we do not print anything anyway)
> - for the single remaining case, create a helper method in `G1HRPrinter`
>
> The first item has been done as per the suggestion from @kstefanj [here](https://github.com/openjdk/jdk/pull/3154#pullrequestreview-619061961) ; the alternative would be to keep the free region lists and call `G1HRPrinter::cleanup` on it later, which I actually initially implemented. I could not find a difference performance-wise.
>
> Testing: checked if there is some perf difference on a heap with 50k regions; tier1-3

This pull request has now been integrated.

Changeset: bf26a255
Author:    Thomas Schatzl <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/bf26a255
Stats:     71 lines in 4 files changed: 44 ins; 21 del; 6 mod

8264027: Refactor "CLEANUP" region printing

Reviewed-by: kbarrett, ayang

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

PR: https://git.openjdk.java.net/jdk/pull/3243