On Fri, 5 May 2023 16:42:29 GMT, Aleksey Shipilev <sh...@openjdk.org> wrote:
>> The API specification clearly states that this method returns "*an >> approximation of the total amount of memory allocated in heap*" so in my >> opinion it is OK to keep it simple here and don't start messing with looks >> and safepoints. >> >> But can't we make this a little more accurate by only adding a threads >> allocated bytes if it is not `thread->is_terminated()`? Wouldn't that >> prevent double counting most of the time? > > I agree we should strive to get the value as accurate as possible. I think > for operational use at scale, we need to avoid doing safepoints. Holding a > `ThreadLock` might also penalize other code that (ab)uses threading (we > frequently see thousands of threads coming and going, don't ask). > > But I have a fundamental question here: since SMR/TLH gives us a snapshot of > currently live threads, and it also protects us from seeing an exiting thread > in bad state (ultimately, a `delete`-d one), why can't we just trust its > `cooked_allocated_bytes`, and avoid adding allocated bytes on exit path? If > we cannot trust that, can we make it trustable while thread is protected by > SMR/TLH? I thought about doing those, but both would slow down the app/JVM and very likely introduce p99.9/p100 latency outliers that we'd rather not see just because we're sampling. Also, 1. The existing thread allocated bytes implementation isn't particularly accurate either, in the sense that reality quickly gets away from the method return values. That was a conscious decision to go with speed and efficiency over stability/accuracy, so I went the same way for this implementation. 2. Cloud services sample at fairly long intervals in order to avoid overhead: 1 minute is common and 5 minutes not unheard of. Stability/accuracy within short time frames is unneeded with such long sampling intervals. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186341500
