OpenJDK › OpenJDK Hotspot › OpenJDK Hotspot Runtime System

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

Classic

List

Threaded

9 messages Options

Thomas Stuefe

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

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

Thomas Stuefe

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

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

Zhengyu Gu-3

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

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

Thomas Stuefe

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

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

Thomas Stuefe

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

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

Thomas Stuefe

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

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

Zhengyu Gu-3

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

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

Thomas Stuefe

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

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

Thomas Stuefe

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

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