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-8269092: Add OldObjectSampleEvent.allocationSize field #4549

Closed
wants to merge 3 commits into from

Conversation

@luhenry
Copy link
Member

@luhenry luhenry commented Jun 22, 2021

The data is already available in ObjectSample.allocated and getting this data
in another way is complicated (via Instrumentation for example). Having the
size of the objects also allows to augment the memory leak analysis based on
the object size impact on top of based on the object count impact.


Progress

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

Issue

  • JDK-8269092: Add OldObjectSampleEvent.allocationSize field

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4549/head:pull/4549
$ git checkout pull/4549

Update a local copy of the PR:
$ git checkout pull/4549
$ git pull https://git.openjdk.java.net/jdk pull/4549/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4549

View PR using the GUI difftool:
$ git pr show -t 4549

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4549.diff

The data is already available in ObjectSample.allocated and getting this data
in another way is complicated (via Instrumentation for example). Having the
size of the objects also allows to augment the memory leak analysis based on
the object size impact on top of based on the object count impact.
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 22, 2021

👋 Welcome back luhenry! 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.

Loading

@openjdk openjdk bot added the rfr label Jun 22, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 22, 2021

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

  • hotspot-jfr

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.

Loading

@openjdk openjdk bot added the hotspot-jfr label Jun 22, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 22, 2021

Webrevs

Loading

@egahlin
Copy link
Member

@egahlin egahlin commented Jun 22, 2021

How will the "allocation size" help in the analysis?

I think this likely to confuse people more than actually help people find memory leaks.

Loading

@luhenry
Copy link
Member Author

@luhenry luhenry commented Jun 23, 2021

Right now the only information about the OldObject in this sample we have are the age, time of the allocation, the number of elements in the array (if it's one), the total amount of heap used (overall, for the whole process, not just this object), and the class, address, and referrer of the object itself.

In the case of memory leak analysis, we then know which objects appear to be leaking. We also know that, based on the heuristics of the priority queue in the leak profiler, the sampled objects have the most impact on memory usage (since sampling is biased towards bigger shallow objects). However, we still don't know exactly how much memory is used. We can approximate it if it's an array, based on the size of the elements and the number of elements. However, it's always going to be approximative since the size of the object header might change in the future (project Lilliput), and because of various other constraints like alignement of elements, size of each element, etc.

I'm sure these constraints would not necessarily be impossible to overcome, but because we already have this exact information, I figured it to be much easier to put it straight in the event. This exact field is also present in AllocationRequiringGC, ObjectAllocationInNewTLAB, and ObjectAllocationOutsideTLAB, so there is precedent in terms of naming, typing, and meaning for related events.

Loading

@egahlin
Copy link
Member

@egahlin egahlin commented Jun 23, 2021

In the case of memory leak analysis, we then know which objects appear to be leaking. We also know that, based on the heuristics of the priority queue in the leak profiler, the sampled objects have the most impact on memory usage (since sampling is biased towards bigger shallow objects).

Not sure I understand, but it may not matter

However, we still don't know exactly how much memory is used. We can approximate it if it's an array, based on the size of the elements and the number of elements. However, it's always going to be approximative since the size of the object header might change in the future (project Lilliput), and because of various other constraints like alignement of elements, size of each element, etc.

But why do you need it?

Is a large (or small?) "allocation size" more likely a leak, or what is the thinking?

Loading

@luhenry
Copy link
Member Author

@luhenry luhenry commented Jun 23, 2021

Is a large (or small?) "allocation size" more likely a leak, or what is the thinking?

It's not so much that it's more likely to leak, but it's to allow the user to prioritise accordingly to the size of the leak. The bigger the leak, the more urgent it seems because the more impactful it is to fix. Overall, it doesn't hurt to have this information, it's trivial to gather (it's a one-liner), and it can help users of JFR to prioritise which leak to fix.

Loading

@luhenry
Copy link
Member Author

@luhenry luhenry commented Jun 24, 2021

Let me just add a quick example of how we plan to use it.

At Datadog, we have for other languages Heap Live Objects and Heap Live Size profilers. These work for Python and Go in a very similar manner to the OldObjectSample in JFR: it takes a sample of objects, and track their lifetime.

Here is an example of what it looks like in practice on a sample profile:

image
image

JFR generally tracks more useful data, like the reference chains. Unfortunately, we are still blocked for the Heap Live Size profiler in Java because we need to match an OldObjectSample to the size it occupies on the heap, and we have no way today to map precisely the object type to the object allocated size. This is what this PR is attempting to fix.

Loading

@egahlin
Copy link
Member

@egahlin egahlin commented Jun 24, 2021

Let me just add a quick example of how we plan to use it.

Ok, thanks for the explanation.

Loading

src/hotspot/share/jfr/metadata/metadata.xml Outdated Show resolved Hide resolved
Loading
test/jdk/jdk/jfr/event/oldobject/TestAllocationSize.java Outdated Show resolved Hide resolved
Loading
test/jdk/jdk/jfr/event/oldobject/TestAllocationSize.java Outdated Show resolved Hide resolved
Loading
@luhenry
Copy link
Member Author

@luhenry luhenry commented Jun 24, 2021

I've updated according to your review and slightly enhanced the test case.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jun 24, 2021

@luhenry This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8269092: Add OldObjectSampleEvent.allocationSize field

Reviewed-by: egahlin, jbachorik

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 96 new commits pushed to the master branch:

  • fd43d9c: 8269225: JFR.stop misses the written info when the filename is only specified by JFR.start
  • 3a8f3d6: 8269280: (bf) Replace StringBuffer in *Buffer.toString()
  • c37988d: 8268276: Base64 Decoding optimization for x86 using AVX-512
  • 08ee7ae: 8268855: Cleanup name handling in the Thread class and subclasses
  • c79034e: 8269303: Remove unnecessary forward declaration of PSPromotionManager in cpCache.hpp
  • 42968db: 8269293: ObjectMonitor thread id fields should be 64 bits.
  • 2fd7943: 8256425: Obsolete Biased Locking in JDK 18
  • 595446b: 8269087: CheckSegmentedCodeCache test fails in an emulated-client VM
  • 7c31903: 8267075: jcmd VM.cds should print directory of the output files
  • e515873: 8269216: Useless initialization in com/sun/crypto/provider/PBES2Parameters.java
  • ... and 86 more: https://git.openjdk.java.net/jdk/compare/2d088fa91d18252a801db3b84ff87e261d63ebd4...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@egahlin, @jbachorik) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

Loading

@openjdk openjdk bot added the ready label Jun 24, 2021
@luhenry
Copy link
Member Author

@luhenry luhenry commented Jun 25, 2021

/integrate

Loading

@openjdk openjdk bot added the sponsor label Jun 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 25, 2021

@luhenry
Your change (at version 42914bd) is now ready to be sponsored by a Committer.

Loading

@jbachorik
Copy link

@jbachorik jbachorik commented Jun 25, 2021

/sponsor

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jun 25, 2021

Going to push as commit fdcae66.
Since your change was applied there have been 96 commits pushed to the master branch:

  • fd43d9c: 8269225: JFR.stop misses the written info when the filename is only specified by JFR.start
  • 3a8f3d6: 8269280: (bf) Replace StringBuffer in *Buffer.toString()
  • c37988d: 8268276: Base64 Decoding optimization for x86 using AVX-512
  • 08ee7ae: 8268855: Cleanup name handling in the Thread class and subclasses
  • c79034e: 8269303: Remove unnecessary forward declaration of PSPromotionManager in cpCache.hpp
  • 42968db: 8269293: ObjectMonitor thread id fields should be 64 bits.
  • 2fd7943: 8256425: Obsolete Biased Locking in JDK 18
  • 595446b: 8269087: CheckSegmentedCodeCache test fails in an emulated-client VM
  • 7c31903: 8267075: jcmd VM.cds should print directory of the output files
  • e515873: 8269216: Useless initialization in com/sun/crypto/provider/PBES2Parameters.java
  • ... and 86 more: https://git.openjdk.java.net/jdk/compare/2d088fa91d18252a801db3b84ff87e261d63ebd4...master

Your commit was automatically rebased without conflicts.

Loading

@openjdk openjdk bot closed this Jun 25, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jun 25, 2021
@openjdk openjdk bot removed the sponsor label Jun 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 25, 2021

@jbachorik @luhenry Pushed as commit fdcae66.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Loading

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