RFR 8204552: NMT: Separate thread stack tracking from virtual memory tracking

Zhengyu Gu zgu at redhat.com
Mon Mar 11 12:54:23 UTC 2019 

Hi,

Could I get a second review?

Thomas Stüfe has reviewed.

The latest webrev:
http://cr.openjdk.java.net/~zgu/JDK-8204552/webrev.03/

It passed hotspot_nmt and submit tests. Thomas also verified on AIX.

Thanks,

-Zhengyu


On 3/4/19 4:05 PM, zgu at redhat.com wrote:
> On Thu, 2019-02-28 at 20:01 +0100, Thomas Stüfe wrote:
>>
> Hi Thomas,
> 
>> Ah, I get it now. Yes, I know the type, some folks are just way too
>> curious :)
>>
>> On the other hand, cross checking statistical numbers and requiring
>> them to make sense makes, well, sense. If they do, it proves you have
>> understood the issue and that your statistics are reliable.
>>   
>>>>
>>>> That said, lets back up a bit, because I still do not understand
>>> what
>>>> you do:
>>>>
>>>> Not sure if I claimed at some point AIX clib allocates thread
>>> stacks
>>>> with malloc? If yes, this was a misunderstanding. I actually do
>>> not
>>>> know which API they use to allocate stacks (aix libc is closed
>>>> source) but I know the effects: thread stack is located in the
>>> data
>>>> segment and not aligned. So it could be allocated with malloc,
>>> but
>>>> also with something like sbrk(). Not sure if this is even
>>> important.
>>>
>>> No, it is not important, but it has to go somewhere. If it is not
>>> malloc, then virtual memory with unaligned address? and possibly
>>> unaligned size? it invites more skepticism.
>>
>> Okay, you mean NMT tracks now VM and malloc, and we need to account
>> for thread stacks somewhere, so we need to use one of the existing
>> mechanisms?
> 
> The answer is no, but probably not worth to invent another one, what do
> you think? Besides NMT always special cases mtThreadStack since
> beginning.
> 
>>   
>>>
>>>>
>>>> I would rather go with some form of "attribute based"
>>> implementation.
>>>> Rather than have one implementation for vm, one for malloc, we
>>> could
>>>> have one which assumes stack boundaries are page aligned and
>>> pages
>>>> can be uncommitted; one where we only track the size and nothing
>>> has
>>>> to be aligned. Basically the same you do in your patch, but I
>>> would
>>>> not name it "..malloc.."
>>>
>>> Okay, I am fine with not naming it "malloc" during reporting.
>>> What's
>>> your suggestion?
>>
>> It is not so much reporting I am concerned with but the code itself.
>> I can live with the numbers for threads to appear as "malloced" to
>> the customer, but inside the code - for maintainers and possible
>> future porters of your code - I would like to be more exact with the
>> terminology.
>>
>> Note that "not aligned page boundaries and cannot call mincore on it"
>> could apply to other forms of stack management as well. For instance,
>> a hypothetical VM implementation where the stacks are managed by user
>> code itself (you can start a thread and provide your own stack).
>>
>> Proposal: I would just rename "MallocThreadStack" to
>> "SimpleThreadStack" or similar, meaning that no assumptions are made
>> about the alignment of the stack boundaries and that we do not know
>> how to find out the size of the committed space. This would be a
>> minimal implementation for handicapped platforms like AIX. The
>> implementation could remain the same.
> 
> Done. I also remoevd "malloc" from thread stack reporting for
> !track_as_vm() cases.
> 
> e.g.
> 
> -                    Thread (reserved=30937KB, committed=30937KB)
>                              (thread #30)
>                              (Stack: 30784KB)
>                              (malloc=115KB #182)
>                              (arena=38KB #58)
> 
>>   
>>>>
>>>> However, I am not even sure if we need all that machinery just to
>>>> track stacks allocation sites. Is that so exciting?
>>> Again, no, but it has to come from somewhere, right?
>>>
>>>> I would assume to see always the same stacks, since there are not
>>>> many variations of "start java thread". Sure it is cool to see.
>>> But
>>>> to me a summary number "sum of java stack memory" would be
>>>> sufficient, maybe in Threads beside the java thread counter. One
>>>> advantage this would have is that we do not need NMT to be on to
>>>> track that.
>>> Again, my intention is to make numbers adding up as closely as
>>> possible.
>>
>> I understood that now. Thank you for your patience.
>>
>> So now that I know the background, I took a closer look at your
>> patch. Beside my general concern - that I do not like the "malloc"
>> naming in all the code - there are few remarks. Seems all solid:
>>
>> src/hotspot/share/services/memBaseline.cpp
>>
>> Question: You use MallocAllocationSiteWalker to walk the thread
>> stacks. But that will exclude allocations considered too small, <
>> SIZE_THRESHOLD. Would this not confuse your numbers?
>>
> Yep. This is indeed a source of inaccuracy :-( IIRC, it was due to the
> concern of tracking memory overhead, probably unfounded.
> 
>> --
>>
>> src/hotspot/share/services/memReporter.cpp
>>
>> Of course, here we have exactly the problem you described, that we
>> report more than RSS since thread stacks are not completely committed
>> on AIX. But I can live with that.
>>
>> Looks good. I assume you tried out all that reporting code by
>> "faking" malloc-like thread stack allocation on your machine?
> Yes, had to disable a few asserts.
> 
>> --
>>
>> src/hotspot/share/services/threadStackTracker.cpp, hpp: looks ok.
>> Should we maybe assert that when deleting a thread stack
>> (ThreadStackTracker::delete_thread_stack), stack size was the same as
>> with create?
>>
> Added assertion in SimpleThreadStackSite::equals(), okay?
> 
> Updated webrev: http://cr.openjdk.java.net/~zgu/JDK-8204552/webrev.02/
> 
> Thanks,
> 
> -Zhengyu
> 
>>
>> Cheers, Thomas
>>
>>   
>>> Thanks,
>>>
>>> -Zhengyu
>>>
>>>
>>>
>>>
>>>>
>>>> Thanks Thomas
>>>>
>>>>
>>>>
>>>> On Mon, Feb 25, 2019 at 6:50 PM <zgu at redhat.com> wrote:
>>>>> On Mon, 2019-02-25 at 18:21 +0100, Thomas Stüfe wrote:
>>>>>> Short question, just to clarify things:
>>>>>>
>>>>>> when you talk about "virtual memory thread stack" and
>>> "malloced
>>>>>> thread stack" you mean actually
>>>>>> a) a thread stack whose boundaries are page aligned and which
>>> can
>>>>>> have uncommitted portions and we are able to find that out
>>> (e.g.
>>>>>> mincore)
>>>>>> b) a thread stack whose boundaries are not necessarily page
>>>>> aligned
>>>>>> and which either have no uncommitted portions or we are
>>> unable to
>>>>>> find that out?
>>>>>>
>>>>>> And the whole "malloc" terminology just stems from the fact
>>> that
>>>>> you
>>>>>> use the malloc tracking mechanics of NMT to track the latter,
>>>>> just
>>>>>> because it is a good fit?
>>>>> No, I do mean if the thread stack is backed by virtual memory
>>> or
>>>>> malloc'd memory.
>>>>>
>>>>> The thread stack is backed by malloc'd memory, it does not
>>> expect
>>>>> to
>>>>> have guard pages. If it is not the case, then this patch is
>>> still
>>>>> broken.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> -Zhengyu
>>>>>
>>>>>
>>>>>>
>>>>>> ..Thomas
>>>>>>
>>>>>>
>>>>>> On Mon, Feb 25, 2019 at 5:10 PM <zgu at redhat.com> wrote:
>>>>>>> Friendly ping!
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> -Zhengyu
>>>>>>>
>>>>>>> On Mon, 2019-02-18 at 14:57 -0500, zgu at redhat.com wrote:
>>>>>>>> On Mon, 2019-02-18 at 08:50 -0500, zgu at redhat.com wrote:
>>>>>>>>> NMT tracks thread stacks as virtual memory since day
>>> one,
>>>>> as
>>>>>>> most
>>>>>>>>> of
>>>>>>>>> platofrms are using virtual memory to back thread
>>> stacks.
>>>>>>> However,
>>>>>>>>> there are exceptions, e.g. AIX. The matter of fact,
>>> POSIX
>>>>>>> standard
>>>>>>>>> does
>>>>>>>>> not dictate what kind of memory or even alignment
>>>>> requirement
>>>>>>> for
>>>>>>>>> thread stacks.
>>>>>>>>>
>>>>>>>>> This patch separates thread stack tracking from virtual
>>>>> memory
>>>>>>>>> tracking. It delegates to virtual memory tracker if
>>> thread
>>>>>>> stack is
>>>>>>>>> backed by virtual memory.
>>>>>>>>>
>>>>>>>>> For thread stacks that are backed by malloc'd memory,
>>> it
>>>>> can
>>>>>>> not
>>>>>>>>> simply
>>>>>>>>> utilize malloc tracker for detail tracking, since it
>>> can
>>>>> not
>>>>>>>>> install
>>>>>>>>> tracking header in place. Therefore, it has to track
>>> those
>>>>>>> memory
>>>>>>>>> separately, then piggybacks to existing malloc tracking
>>>>>>>>> infrastructure
>>>>>>>>> during snapshot, for baselining and reporting.
>>>>>>>>>
>>>>>>>>> Thanks for Goetz Lindenmaier and Thomas Stüfe's help to
>>> run
>>>>> the
>>>>>>>>> patch
>>>>>>>>> through SAP buidling and testing infrastructure, that
>>> is
>>>>>>> especially
>>>>>>>>> helpful for verifying the fix for AIX.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8204552
>>>>>>>>> Webrev: http://cr.openjdk.java.net/~zgu/JDK-8204552/web
>>> rev.
>>>>> 01/
>>>>>>>>>
>>>>>>>>> Test:
>>>>>>>>>    hotspot_nmt on Linux x64 (fastdebug and release)
>>>>>>>>
>>>>>>>> Also passed JDK-submit tests.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> -Zhengyu
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> -Zhengyu
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>

More information about the hotspot-runtime-dev mailing list