Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8261238: NMT should not limit baselining by size threshold #2428

Open

Conversation

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Feb 5, 2021

NMT is a very useful tool to detect memory leaks. Its easy to use, relatively cheap and requires almost zero setup.

But it artificially limits output to call sites with >1K total memory consumption. That is not helpful, as it confuses the developer and obscures possible memory leaks. It also introduces subtle display errors in that callsites suddenly seem to appear, when comparing to the baseline, with a load of >1K when in fact they had been there already when the baseline was taken.

I propose to remove the 1K limit completely. This will marginally increase the costs of taking "detail" NMT reports by about 210K for a single report, 420K for a report done using a baseline.

If this is really a problem preventing us from getting more precise NMT output (really?) there are possibilities to decrease the cost again by sharing call stacks by reference instead of copying them around. I have a patch in work which does just that (https://bugs.openjdk.java.net/browse/JDK-8261491), but I honestly think adding 200-400K temporarily during NMT detail reporting would not be such a big deal.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8261238: NMT should not limit baselining by size threshold

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2428/head:pull/2428
$ git checkout pull/2428

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 5, 2021

👋 Welcome back stuefe! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Feb 5, 2021

@tstuefe The following label will be automatically applied to this pull request:

  • hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@tstuefe tstuefe marked this pull request as ready for review Feb 5, 2021
@openjdk openjdk bot added the rfr label Feb 5, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 5, 2021

@tstuefe tstuefe force-pushed the tstuefe:JDK-8261238-NMT-should-not-limit-baseline-threshold branch from 6f98dbe to 4d35243 Feb 8, 2021
Copy link
Contributor

@zhengyu123 zhengyu123 left a comment

The threshold was put in place purely due to memory consumption concerns.
There was a memory overhead requirement in original document (prior to JEP, Oracle private). If this is no longer a concern, I am good with this change.

@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Feb 8, 2021

The threshold was put in place purely due to memory consumption concerns.
There was a memory overhead requirement in original document (prior to JEP, Oracle private). If this is no longer a concern, I am good with this change.

Great, Zhengyu, thanks!

@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 9, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 9/02/2021 2:29 am, Thomas Stuefe wrote:

On Mon, 8 Feb 2021 16:03:59 GMT, Zhengyu Gu <zgu at openjdk.org> wrote:

The threshold was put in place purely due to memory consumption concerns.
There was a memory overhead requirement in original document (prior to JEP, Oracle private). If this is no longer a concern, I am good with this change.

Great, Zhengyu, thanks!

To know if this is still a concern we know to know what the original
concern was.

David

@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Feb 9, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 9/02/2021 2:29 am, Thomas Stuefe wrote:

On Mon, 8 Feb 2021 16:03:59 GMT, Zhengyu Gu wrote:

The threshold was put in place purely due to memory consumption concerns.
There was a memory overhead requirement in original document (prior to JEP, Oracle private). If this is no longer a concern, I am good with this change.

Great, Zhengyu, thanks!

To know if this is still a concern we know to know what the original
concern was.

David

Yes, it would be helpful to know what the initial memory requirements were as well as the considerations behind it. NMT tries to be very slim; which is good, but at various places we trade performance for memory footprint (eg. live with a load factor of 5+ in the malloc site table rather than spending an additional 4-12K to broaden the table). Without knowing these requirements and without deciding if they are still valid improving NMT is difficult.

@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Feb 11, 2021

I am currently working on a patch which reduces cost of baselining: https://bugs.openjdk.java.net/browse/JDK-8261491 which should be more than enough to mitigate the modest footprint increase of this patch.

So, can we get this patch approved please, unless there are other technical considerations I am not seeing?

@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Feb 23, 2021

I split out non-controversial display changes into JDK-8262165.

@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Mar 6, 2021

Gentle Ping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants