RFR: 8262099: jcmd VM.metaspace should report unlimited size if MaxMetaspaceSize isn't specified

[Yang Yi]

Hi,

Can I have a review of this minor fix about `jcmd VM.metaspace`?

When MaxMetaspaceSize is not specified, it would set to max_uintx, jcmd VM.metaspace reports metaspace summary information as follows:

$ ./jcmd <pid> VM.metaspace | grep MaxMetaspaceSize
MaxMetaspaceSize: 17179869184.00 GB

Actually, this is a bug, the expected output should be:

MaxMetaspaceSize: unlimited

The root cause of the problem is that the MaxMetaspaceSize rounding performed during arguments parsing is inconsistent with the round checking in print_settings(metaspaceRerporter.cpp).

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

Commit messages:
 - jcmd VM.metaspace should report unlimited size

Changes: https://git.openjdk.java.net/jdk/pull/2671/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2671&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8262099
  Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2671.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2671/head:pull/2671

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

[David Holmes-2]

On Mon, 22 Feb 2021 03:21:45 GMT, Yang Yi <[hidden email]> wrote:

> Hi,
>
> Can I have a review of this minor fix about `jcmd VM.metaspace`?
>
> When MaxMetaspaceSize is not specified, it would set to max_uintx, jcmd VM.metaspace reports metaspace summary information as follows:
>
> $ ./jcmd <pid> VM.metaspace | grep MaxMetaspaceSize
> MaxMetaspaceSize: 17179869184.00 GB
>
> Actually, this is a bug, the expected output should be:
>
> MaxMetaspaceSize: unlimited
>
> The root cause of the problem is that the MaxMetaspaceSize rounding performed during arguments parsing is inconsistent with the round checking in print_settings(metaspaceRerporter.cpp).

src/hotspot/share/memory/metaspace/metaspaceReporter.cpp line 3:

> 1: /*
> 2:  * Copyright (c) 2018, 2021, Oracle and/or its affiliates. All rights reserved.
> 3:  * Copyright (c) 2018, 2021 SAP SE. All rights reserved.

Please note that while Oracle requests/requires that it's copyright line gets updated each year a file is modified, it is not clear whether that extends to any other copyright lines that might appear in the file. In general for companies other than Oracle, only a committer from that company should update a copyright line for that company.

Thanks,
David

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

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

[Yang Yi]

> Hi,
>
> Can I have a review of this minor fix about `jcmd VM.metaspace`?
>
> When MaxMetaspaceSize is not specified, it would set to max_uintx, jcmd VM.metaspace reports metaspace summary information as follows:
>
> $ ./jcmd <pid> VM.metaspace | grep MaxMetaspaceSize
> MaxMetaspaceSize: 17179869184.00 GB
>
> Actually, this is a bug, the expected output should be:
>
> MaxMetaspaceSize: unlimited
>
> The root cause of the problem is that the MaxMetaspaceSize rounding performed during arguments parsing is inconsistent with the round checking in print_settings(metaspaceRerporter.cpp).

Yang Yi has updated the pull request incrementally with one additional commit since the last revision:

  remain unchanged SAP copyright year, only update Oracle copyright year

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2671/files
  - new: https://git.openjdk.java.net/jdk/pull/2671/files/566dce02..ca16d9cc

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2671&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2671&range=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2671.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2671/head:pull/2671

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

[Yang Yi]

On Mon, 22 Feb 2021 05:29:40 GMT, David Holmes <[hidden email]> wrote:

>> Yang Yi has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   remain unchanged SAP copyright year, only update Oracle copyright year
>
> src/hotspot/share/memory/metaspace/metaspaceReporter.cpp line 3:
>
>> 1: /*
>> 2:  * Copyright (c) 2018, 2021, Oracle and/or its affiliates. All rights reserved.
>> 3:  * Copyright (c) 2018, 2021 SAP SE. All rights reserved.
>
> Please note that while Oracle requests/requires that it's copyright line gets updated each year a file is modified, it is not clear whether that extends to any other copyright lines that might appear in the file. In general for companies other than Oracle, only a committer from that company should update a copyright line for that company.
>
> Thanks,
> David

Changed. Thanks for pointing out this!

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

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

[Thomas Stuefe]

On Mon, 22 Feb 2021 05:43:06 GMT, Yang Yi <[hidden email]> wrote:

>> src/hotspot/share/memory/metaspace/metaspaceReporter.cpp line 3:
>>
>>> 1: /*
>>> 2:  * Copyright (c) 2018, 2021, Oracle and/or its affiliates. All rights reserved.
>>> 3:  * Copyright (c) 2018, 2021 SAP SE. All rights reserved.
>>
>> Please note that while Oracle requests/requires that it's copyright line gets updated each year a file is modified, it is not clear whether that extends to any other copyright lines that might appear in the file. In general for companies other than Oracle, only a committer from that company should update a copyright line for that company.
>>
>> Thanks,
>> David
>
> Changed. Thanks for pointing out this!

I would have been fine with the copyright change :)

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

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

[Thomas Stuefe]

On Mon, 22 Feb 2021 05:47:58 GMT, Yang Yi <[hidden email]> wrote:

>> Hi,
>>
>> Can I have a review of this minor fix about `jcmd VM.metaspace`?
>>
>> When MaxMetaspaceSize is not specified, it would set to max_uintx, jcmd VM.metaspace reports metaspace summary information as follows:
>>
>> $ ./jcmd <pid> VM.metaspace | grep MaxMetaspaceSize
>> MaxMetaspaceSize: 17179869184.00 GB
>>
>> Actually, this is a bug, the expected output should be:
>>
>> MaxMetaspaceSize: unlimited
>>
>> The root cause of the problem is that the MaxMetaspaceSize rounding performed during arguments parsing is inconsistent with the round checking in print_settings(metaspaceRerporter.cpp).
>
> Yang Yi has updated the pull request incrementally with one additional commit since the last revision:
>
>   remain unchanged SAP copyright year, only update Oracle copyright year

Change looks fine, thanks for fixing. Can you please add a comment like "see Metaspace::ergo_initialize()" ?

I think the only reason we align MaxMetaspaceSize down is to forestall any comparison overflows on metaspace expansion. But I am not sure this is even an issue. I would like a solution where we leave it at max_uint and check the comparisons for overflow instead. But this patch looks fine.

Make sure you run the associated JTreg test.

Cheers, Thomas

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

Marked as reviewed by stuefe (Reviewer).

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

[Yang Yi]

On Mon, 22 Feb 2021 09:19:26 GMT, Thomas Stuefe <[hidden email]> wrote:

>> Yang Yi has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   remain unchanged SAP copyright year, only update Oracle copyright year
>
> Change looks fine, thanks for fixing. Can you please add a comment like "see Metaspace::ergo_initialize()" ?
>
> I think the only reason we align MaxMetaspaceSize down is to forestall any comparison overflows on metaspace expansion. But I am not sure this is even an issue. I would like a solution where we leave it at max_uint and check the comparisons for overflow instead. But this patch looks fine.
>
> Make sure you run the associated JTreg test.
>
> Cheers, Thomas

Hi Thomas, I've updated the patch according to your review comment. All related tests have passed. In addition, I added a new test case to ensure that it works well in the future. Also I have updated the SAP copyright year.(Please let me know if I misunderstand your meaning:-))

Thanks,
Yang Yi

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

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

[Yang Yi]

> Hi,
>
> Can I have a review of this minor fix about `jcmd VM.metaspace`?
>
> When MaxMetaspaceSize is not specified, it would set to max_uintx, jcmd VM.metaspace reports metaspace summary information as follows:
>
> $ ./jcmd <pid> VM.metaspace | grep MaxMetaspaceSize
> MaxMetaspaceSize: 17179869184.00 GB
>
> Actually, this is a bug, the expected output should be:
>
> MaxMetaspaceSize: unlimited
>
> The root cause of the problem is that the MaxMetaspaceSize rounding performed during arguments parsing is inconsistent with the round checking in print_settings(metaspaceRerporter.cpp).

Yang Yi has updated the pull request incrementally with one additional commit since the last revision:

  add more testcase, and comment out change

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2671/files
  - new: https://git.openjdk.java.net/jdk/pull/2671/files/ca16d9cc..0be70d1d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2671&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2671&range=01-02

  Stats: 41 lines in 2 files changed: 24 ins; 1 del; 16 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2671.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2671/head:pull/2671

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

[Yang Yi]

> Hi,
>
> Can I have a review of this minor fix about `jcmd VM.metaspace`?
>
> When MaxMetaspaceSize is not specified, it would set to max_uintx, jcmd VM.metaspace reports metaspace summary information as follows:
>
> $ ./jcmd <pid> VM.metaspace | grep MaxMetaspaceSize
> MaxMetaspaceSize: 17179869184.00 GB
>
> Actually, this is a bug, the expected output should be:
>
> MaxMetaspaceSize: unlimited
>
> The root cause of the problem is that the MaxMetaspaceSize rounding performed during arguments parsing is inconsistent with the round checking in print_settings(metaspaceRerporter.cpp).

Yang Yi has updated the pull request incrementally with one additional commit since the last revision:

  IANAL, keep SAP copyright year unchanage

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2671/files
  - new: https://git.openjdk.java.net/jdk/pull/2671/files/0be70d1d..b42940b5

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2671&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2671&range=02-03

  Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2671.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2671/head:pull/2671

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

[Lutz Schmidt]

On Tue, 23 Feb 2021 02:15:02 GMT, Yi Yang <[hidden email]> wrote:

>> Hi,
>>
>> Can I have a review of this minor fix about `jcmd VM.metaspace`?
>>
>> When MaxMetaspaceSize is not specified, it would set to max_uintx, jcmd VM.metaspace reports metaspace summary information as follows:
>>
>> $ ./jcmd <pid> VM.metaspace | grep MaxMetaspaceSize
>> MaxMetaspaceSize: 17179869184.00 GB
>>
>> Actually, this is a bug, the expected output should be:
>>
>> MaxMetaspaceSize: unlimited
>>
>> The root cause of the problem is that the MaxMetaspaceSize rounding performed during arguments parsing is inconsistent with the round checking in print_settings(metaspaceRerporter.cpp).
>
> Yi Yang has updated the pull request incrementally with one additional commit since the last revision:
>
>   IANAL, keep SAP copyright year unchanage

Changes look good to me.
Thanks for fixing. I was complaining about the, well, awful printout at another place.

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

Marked as reviewed by lucy (Reviewer).

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

[Yang Yi]

On Wed, 24 Feb 2021 17:27:54 GMT, Lutz Schmidt <[hidden email]> wrote:

>> Yi Yang has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   IANAL, keep SAP copyright year unchanage
>
> Changes look good to me.
> Thanks for fixing. I was complaining about the, well, awful printout at another place.

Thanks @dholmes-ora @tstuefe @RealLucy for reviews. Since I'm not a committer I can not commit directly, can you help to sponsor this patch :) ?

Thanks,
Yang

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

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

[Yang Yi]

On Mon, 22 Feb 2021 03:21:45 GMT, Yi Yang <[hidden email]> wrote:

> Hi,
>
> Can I have a review of this minor fix about `jcmd VM.metaspace`?
>
> When MaxMetaspaceSize is not specified, it would set to max_uintx, jcmd VM.metaspace reports metaspace summary information as follows:
>
> $ ./jcmd <pid> VM.metaspace | grep MaxMetaspaceSize
> MaxMetaspaceSize: 17179869184.00 GB
>
> Actually, this is a bug, the expected output should be:
>
> MaxMetaspaceSize: unlimited
>
> The root cause of the problem is that the MaxMetaspaceSize rounding performed during arguments parsing is inconsistent with the round checking in print_settings(metaspaceRerporter.cpp).

This pull request has now been integrated.

Changeset: 3a0d6a64
Author:    Yang Yi <[hidden email]>
Committer: David Holmes <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/3a0d6a64
Stats:     41 lines in 2 files changed: 24 ins; 1 del; 16 mod

8262099: jcmd VM.metaspace should report unlimited size if MaxMetaspaceSize isn't specified

Reviewed-by: stuefe, lucy

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

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