RFR: JDK-8261297: NMT: Final report should use scale 1

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: JDK-8261297: NMT: Final report should use scale 1

Thomas Stuefe
If PrintNMTStatistics is specified, we write a NMT report to stdout before exiting the VM. This is useful for leak analysis.

However, the report uses the standard "K" scale, which is shouldn't, since this omits small leaks < 1K. Hence it should use scale=1. This would provide us with a precise exit report. Note that for this to work JDK-8261238 has to be fixed too, which is a separate issue.

This patch:
- changes the scale of the final report to 1
- does some minor cleanup work in the MemReporter class hierarchy (const-ifying scale and the output stream, and establishing 0 as "default scale" as to avoid having the default scale hard coded in four places.

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

Commit messages:
 - Start

Changes: https://git.openjdk.java.net/jdk/pull/2450/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2450&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261297
  Stats: 36 lines in 3 files changed: 13 ins; 5 del; 18 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2450.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2450/head:pull/2450

PR: https://git.openjdk.java.net/jdk/pull/2450
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8261297: NMT: Final report should use scale 1

Thomas Stuefe
On Mon, 8 Feb 2021 08:15:21 GMT, Thomas Stuefe <[hidden email]> wrote:

> If PrintNMTStatistics is specified, we write a NMT report to stdout before exiting the VM. This is useful for leak analysis.
>
> However, the report uses the standard "K" scale, which is shouldn't, since this omits small leaks < 1K. Hence it should use scale=1. This would provide us with a precise exit report. Note that for this to work JDK-8261238 has to be fixed too, which is a separate issue.
>
> This patch:
> - changes the scale of the final report to 1
> - does some minor cleanup work in the MemReporter class hierarchy (const-ifying scale and the output stream, and establishing 0 as "default scale" as to avoid having the default scale hard coded in four places.

@zhengyu123 , are you okay with this?

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

PR: https://git.openjdk.java.net/jdk/pull/2450
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8261297: NMT: Final report should use scale 1

Zhengyu Gu-3
In reply to this post by Thomas Stuefe
On Mon, 8 Feb 2021 08:15:21 GMT, Thomas Stuefe <[hidden email]> wrote:

> If PrintNMTStatistics is specified, we write a NMT report to stdout before exiting the VM. This is useful for leak analysis.
>
> However, the report uses the standard "K" scale, which is shouldn't, since this omits small leaks < 1K. Hence it should use scale=1. This would provide us with a precise exit report. Note that for this to work JDK-8261238 has to be fixed too, which is a separate issue.
>
> This patch:
> - changes the scale of the final report to 1
> - does some minor cleanup work in the MemReporter class hierarchy (const-ifying scale and the output stream, and establishing 0 as "default scale" as to avoid having the default scale hard coded in four places.

Sorry for the delay.

src/hotspot/share/services/memReporter.hpp line 48:

> 46:  public:
> 47:   // scale = 0 -> default scale
> 48:   MemReporterBase(outputStream* out = NULL, size_t scale = 0) :

Why you want to default scale parameter to 0, instead of default_scale?

src/hotspot/share/services/memTracker.hpp line 119:

> 117:
> 118:  public:
> 119:

Unrelated?

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

Changes requested by zgu (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2450
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8261297: NMT: Final report should use scale 1 [v2]

Thomas Stuefe
In reply to this post by Thomas Stuefe
> If PrintNMTStatistics is specified, we write a NMT report to stdout before exiting the VM. This is useful for leak analysis.
>
> However, the report uses the standard "K" scale, which is shouldn't, since this omits small leaks < 1K. Hence it should use scale=1. This would provide us with a precise exit report. Note that for this to work JDK-8261238 has to be fixed too, which is a separate issue.
>
> This patch:
> - changes the scale of the final report to 1
> - does some minor cleanup work in the MemReporter class hierarchy (const-ifying scale and the output stream, and establishing 0 as "default scale" as to avoid having the default scale hard coded in four places.

Thomas Stuefe has updated the pull request incrementally with two additional commits since the last revision:

 - Missed one
 - Feedback Zhengyu

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2450/files
  - new: https://git.openjdk.java.net/jdk/pull/2450/files/7bbcb0fa..d2479461

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

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

PR: https://git.openjdk.java.net/jdk/pull/2450
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8261297: NMT: Final report should use scale 1 [v2]

Thomas Stuefe
In reply to this post by Zhengyu Gu-3
On Wed, 10 Feb 2021 13:18:40 GMT, Zhengyu Gu <[hidden email]> wrote:

> Sorry for the delay.

No problem, I just pinged you in case you had not seen this PR. Since there were so many.

As requested I wired "default_scale" directly and removed the "0" as "default". I also removed the default NULL parameter for outputStream since I found no uses of MemReporter classes without an outputStream specified.

I also removed the superfluous newline.

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

PR: https://git.openjdk.java.net/jdk/pull/2450
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8261297: NMT: Final report should use scale 1 [v3]

Thomas Stuefe
In reply to this post by Thomas Stuefe
> If PrintNMTStatistics is specified, we write a NMT report to stdout before exiting the VM. This is useful for leak analysis.
>
> However, the report uses the standard "K" scale, which is shouldn't, since this omits small leaks < 1K. Hence it should use scale=1. This would provide us with a precise exit report. Note that for this to work JDK-8261238 has to be fixed too, which is a separate issue.
>
> This patch:
> - changes the scale of the final report to 1
> - does some minor cleanup work in the MemReporter class hierarchy (const-ifying scale and the output stream, and establishing 0 as "default scale" as to avoid having the default scale hard coded in four places.

Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:

  Missed another one

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2450/files
  - new: https://git.openjdk.java.net/jdk/pull/2450/files/d2479461..c343316c

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

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

PR: https://git.openjdk.java.net/jdk/pull/2450
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8261297: NMT: Final report should use scale 1 [v2]

Zhengyu Gu-3
In reply to this post by Thomas Stuefe
On Wed, 10 Feb 2021 15:36:52 GMT, Thomas Stuefe <[hidden email]> wrote:

>> If PrintNMTStatistics is specified, we write a NMT report to stdout before exiting the VM. This is useful for leak analysis.
>>
>> However, the report uses the standard "K" scale, which is shouldn't, since this omits small leaks < 1K. Hence it should use scale=1. This would provide us with a precise exit report. Note that for this to work JDK-8261238 has to be fixed too, which is a separate issue.
>>
>> This patch:
>> - changes the scale of the final report to 1
>> - does some minor cleanup work in the MemReporter class hierarchy (const-ifying scale and the output stream, and establishing 0 as "default scale" as to avoid having the default scale hard coded in four places.
>
> Thomas Stuefe has updated the pull request incrementally with two additional commits since the last revision:
>
>  - Missed one
>  - Feedback Zhengyu

Looks good and trivial

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

Marked as reviewed by zgu (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2450
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8261297: NMT: Final report should use scale 1 [v2]

Thomas Stuefe
On Wed, 10 Feb 2021 15:41:12 GMT, Zhengyu Gu <[hidden email]> wrote:

> Looks good and trivial

Thanks!

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

PR: https://git.openjdk.java.net/jdk/pull/2450
Reply | Threaded
Open this post in threaded view
|

Integrated: JDK-8261297: NMT: Final report should use scale 1

Thomas Stuefe
In reply to this post by Thomas Stuefe
On Mon, 8 Feb 2021 08:15:21 GMT, Thomas Stuefe <[hidden email]> wrote:

> If PrintNMTStatistics is specified, we write a NMT report to stdout before exiting the VM. This is useful for leak analysis.
>
> However, the report uses the standard "K" scale, which is shouldn't, since this omits small leaks < 1K. Hence it should use scale=1. This would provide us with a precise exit report. Note that for this to work JDK-8261238 has to be fixed too, which is a separate issue.
>
> This patch:
> - changes the scale of the final report to 1
> - does some minor cleanup work in the MemReporter class hierarchy (const-ifying scale and the output stream, and establishing 0 as "default scale" as to avoid having the default scale hard coded in four places.

This pull request has now been integrated.

Changeset: 1740de2a
Author:    Thomas Stuefe <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/1740de2a
Stats:     34 lines in 3 files changed: 11 ins; 5 del; 18 mod

8261297: NMT: Final report should use scale 1

Reviewed-by: zgu

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

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