RFR: 8258431: Provide a JFR event with live set size estimate [v12]

Jaroslav Bachorík jaroslav.bachorik at datadoghq.com
Thu Mar 18 09:26:53 UTC 2021


First of all, it is a pity my [attempt to open a discussion regarding
this topic](https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2021-February/033705.html)
didn't get more attention.

On Wed, Mar 17, 2021 at 6:22 PM Thomas Schatzl
<tschatzl at openjdk.java.net> wrote:
>
> On Tue, 16 Mar 2021 12:23:52 GMT, Jaroslav Bachorik <jbachorik at openjdk.org> wrote:
>
> > > Also, for this purpose, why would used() not be a good substitute for liveness? If e.g. used() average grows over time you can deduce the same I would assume (particularly used() after mixed gc phase in g1).
> >
> > The major problem is that eg. for g1 given large enough heap the used value can keep on growing for quite long time, possibly generating wrong signal about potential memory leak.
>
> So can this liveness estimate - just filter used() a bit more.
>
> Actually I strongly believe that due to containing more details, used() could be more appropriate for this: first, it updates more often, second, information like distance between valleys and peaks of the sawtooth pattern are indicative of memory running away.
> If you really want to limit yourselves to something that is similar in update frequency to this liveness estimate, one could use just one value in a "tooth". However I think just strong enough low-pass filtering enough is as fine.
>
> >
> > If the live estimate is set to `used()` after mixed gc phase in g1 I think it still will be a good estimate.
> > The only thing I am opposing is having `live()` call return the current `used()` value which, IMO, might become rather confusing.
> >
> > > Do you have any numbers on what the impact of using used() vs. this live() would be in such a use case?
> >
> > Nope. Do you mean perf impact?
>
> Impact on false positives/negatives.
>
> >
> > > What I'm afraid of is that mixing values taken at different times - used and capacity are taken at the time of the event, and the liveness estimated updated at other, irregular intervals may cause significiant amount of confusion in interpreting this value. It might be obvious to you, but there will be other users.
> >
> > IDK. If the event field would explicitly mention that this is the **last known live size estimate** it should set the expectations right.
> >
> > > One option could be detaching the liveness estimate from used()/capacity() (I see a value in having some heap usage summary at regular intervals) and send the liveness estimate event just when they are generated? Then the various collectors could send this liveness value at times when they think they are fairly accurate, not when the collectors must and particularly not in conjunction with samples taken at completely different times.
> >
> > The problem is the irregularity - when the live size is reported only when it is calculated there might be long periods in the recording missing the live size data at all. In order for this information to be useful it should be reported at least at the beginning and end of a JFR chunk.
> >
> > > Independent of whether used/capacity and liveness are sent, the receiver needs to do statistics (trend lines) on those anyway.
> >
> > Yes. It's just that with the live size estimate one wouldn't be getting the false positives one would get with used heap trend.
>
> We've now discussed this issue within the gc team a bit and came to the following conclusions.
>
> Before going into that, our guidelines for adding new tracing code: generally we avoid providing functionality in the VM/GC that can be procured easily otherwise or there is a good substitute, particularly if their actual content is unclear. The VM is also generally not the place to store or accumulate data that is only used in external applications for their convenience, particularly where the general public usefulness is questionable or do not drive forward GC algorithms in some way.
>
> In the past in the cases we have done this a few times, and this has caused lots of maintenance burden (adding it, keeping it up to date, and finally removing it because nobody used it after all).
>
> We are open to providing raw data for events that fits this in the most painless way for the VM if they are well specified.
>
> The whole periodic HeapSummary event and its contents are questionable in this light:
>
> - used() and capacity() are provided "regularly", and it could either be retrieved at any time by other means (e.g. MXBeans), or even forced to be provided (cause a gc if you are really desperate). Particularly in cases of continuous monitoring, there should be no problem getting them even with existing JFR events.

These values might be quite imprecise - it is not uncommon for G1 to
see heap usage increasing for long time which could falsely trigger a
leak alarm.
Also, triggering GC does not seem to be that great idea considering
that it will cause a rather lengthy safepoint.

> - it can be argued that it is *very* painless for GC to provide the periodic used/capacity, their values are well defined for all collectors. Still I believe particularly if you do continuous monitoring, this is kind of unnecessary and should be polled instead of pushed if required at higher frequency as provided now.

Well, we are doing continuous monitoring and it is not unnecessary ..

>
> - the suggested "liveness estimate" however goes against all of these guidelines:
>   - the value is ill-defined if at all. The quality of the implementations is all over the place:
>     - Epsilon the used() value
There is literally no other information for epsilon if I am not
totally mistaken.

>     - Serial the used() value and sometimes some attempt to actually return the live data using the dead-wood heuristic
>     - Parallel returns used() always
>     - G1 returns the amount of bytes marked plus the bytes allocated while marking, used() in other cases (although that may change)
>     - Z returns the amount of bytes marked without the bytes allocated while marking (this is unclear to me actually)
>     - Shenandoah seems to be fairly close to G1, I have no idea what the results are on the various additional modes it uses.
Yes. And this was my initial question in the email thread I started
before even attempting this PR. Whatever limited response I got seemed
to confirm this way of getting the liveness estimate.

>   - for those collectors that can not give you a good value, the application could as well easily generate it - just use used(). (That deadwood optimization does not change the situation a lot as the difference would be at most 5% or so difference, well within "estimate" range).
>   - it forces gcs that do not use or need that value at all first calculate it and then keep it around for just this event
>   - this liveness estimate, which is outdated a few instructions after the application runs, is coupled with current used()/capacity() values
>   - there is no indication if that estimate in that event can actually used for the suggested purpose: It could have been calculated at any point in time, so its use for trend lines is limited (e.g.. regression etc). For such a regression you typically need multiple values anyway, and even more for some output with a significant amount of considence, so that single value without timestamp does not seem to help. If you track continuously, you would get all values anyway.
>
> So overall I believe the current suggestion to have the VM provide all these values and the event is just introducing complexity in the VM for convenience of the application.
>
> Still I think there might be need for the raw data if available, and if it's easily obtainable, then fine, do send some duplicate data. So my suggestion and what we in the gc team can support is to
>
> a) provide that HeapSummary event with capacity() and used() (but as mentioned, on a change they are sent out already so I do not see the exact situation in particular with continuous tracking...).
>
> b) provide some Liveness (or "Marked bytes" or similar) event when the value is generated (if they are generated) as a one-off event in places it can be derived without significant costs. I.e. some "send_marked_bytes()/live_bytes()" method to be called in Serial, Parallel, G1, Z and Shenandoah when generated and available. No additional storage of that value in the and repeated emission of that event by the VM.
> I'm intentionally using "marked bytes" here because this is a value that can actually be defined and verified by reviewers that it's actually returned. Some best effort estimate is just misleading, and is a pain to maintain and argue whether the goal has been met (and it's even worse to (dis-)prove that a change introduced a bug). Maybe it could be extended to "marked bytes plus allocated during marking, sent when marking finished" for all collectors that do marking - I do not know (particularly for Z), maybe it can.
> This approach also allows anyone to easily incrementally add new occurrences of that event as more code to support it has been written (e.g. with PR #2966 for g1 full gc), and allows leaving out collectors that do not support it, or places where this has not been gathered.
>
> We think this may be a useful value, that can be explained to others, will cause minimal misunderstanding, can be verified at least in the reviews (and tests where sending of that event is verified for the various situations it should be sent), and maintained.
>
> Returning "liveness" in a good way (both in accuracy and overhead) is a completely different issue, and probably worth a few PhDs. Just dodging the issue with appending "estimate" to the name is not the fix given alternatives.
>
> I would further ask you to at least create two different CRs for adding the two events (more for later additions of the event) for easier and faster review. You can provide a link to a "all-in" diff to let the reviewers see where you want to go with this. However having non-trivial changes across (at this point) 34 files for different collectors for different reasons is nontrivial and very exhausting to review and re-review (10 times so far at least for me).

I am sorry for wasting yours and other reviewers time. I really tried
getting some consensus before coming with a half-baked PR but as you
can see the responses start flowing only when the PR is 'almost done'.

I am sensing quite negative attitude toward this change and the whole
idea of capturing the liveness estimate. At the same time the
solutions you propose give me nothing over just doing custom JFR event
generation using data obtained via JMX - so I really don't see a point
in doing this change in hotspot.

I am going to close this PR and I apologize again for wasting time of
all involved engineers.

Regards,

-JB-

>
> Thanks,
>   Thomas
>
> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/2579



More information about the hotspot-gc-dev mailing list