Code review request: CR 6995781 Native Memory Tracking Phase 1

[Zhengyu Gu]

Hi Thomas,

Thanks for review and suggestions. Please see comments in text.

On 6/20/2012 10:29 AM, Thomas Stüfe wrote:
> Hi Zhengyu,
>
> I work for SAP Germany on the SAP JVM, which is derived from the
> Oracle VM. We got interested in your work on NMT, so I took a look at
> your change.
>
> Thank you for your work, NMT may prove very useful!
>
> I have some suggestions for improvements (and hope I understood
> everything correctly):
>
> 1) For NEW_C_HEAP_ARRAY and friends, you introduced a new parameter,
> the memory type. Which now has to be specified on every callsite (see
> e.g. arguments.cpp).
> This is a matter of taste, but if one wanted to remove the need for
> this extra parameter, one could introduce a default memory type (which
> could be e.g. mtInternal). That default type could be overridden via
> macro at file scope, so that all NEW_C_HEAP_ARRAY invocations in a
> given file would use that memory type, without having to specify it
> each time.
We had discussion on this during design phase. We feel that, if we 
provide default value, developers might
forget properly tag the memory, since this parameter is not instinctive, 
we feel we should let compiler to catch this,
rather than let inaccurate information slip through.

> 2) allocation.cpp: If I understand it correctly, you track Arena size
> changes (Arena::set_size_in_bytes()), and, separately from that, Chunk
> allocation and destruction. That way you loose the information about
> which chunk is associated with which Arena - you know chunk locations,
> but not to which Arena they belong, or if they are pooled.
>
Correct.
> A different approach could be not to track arena size changes, but
> chunk ownership change: when a chunk is handed out to an arena by the
> ChunkPool, and when the chunk is returned to the ChunkPool. you could
> track Chunk location and size together with Arena address - that way,
> you'd know which chunk belongs to which arena and you could
> reconstruct a memory layout with arena information.
>
Yes, it can be done this way, I will file a RFE to track this idea.
> I think it would come down to exact the same number of calls to the
> memory tracker, only the tracked information would be richer. I don't
> know though how much performance that would cost.
>
> 3) If you don't agree with (2): you moved the implementation of
> Arena::set_size_in_bytes() to allocation.cpp. Performance-wise it may
> make sense to inline the part of the function which does the
> comparison ("if (_size_in_bytes != size) {") and separate the call to
> "MemTracker::record_arena_size((address)this, size);" into an own
> method, which you could leave in allocation.cpp.
>
Yes.

> 4) Under Windows, you use RtlCaptureStackBackTrace(), which in theory
> could give you the whole backtrace in one go; however, you call it
> repeatedly for each frame (memTracker.cpp, void
> MemTracker::walk_stack()), which could be avoided.
Agree. walk_stack() is leftover debug function, so I did not optimize it.

Thanks,

-Zhengyu

> Kind regards,
>
> Thomas Stuefe
> SAP Germany
>
>
>> This is the webrev for native memory tracking phase 1, which is tracked
>> by CR6995781 (http://monaco.us.oracle.com/detail.jsf?cr=6995781) and
>> related DCmd CR7151532 (http://monaco.us.oracle.com/detail.jsf?cr=7151532).
>>
>> Native memory tracking (NMT) phase 1, is designed to track native memory
>> (malloc'd and mmap'd) usages by VM code only, it does not track the
>> memory usages by libraries and JNI code.
>>
>> On the implementation side, NMT intercepts memory related calls in os
>> implementation, such as os::malloc, os::realloc, os::free,
>> os::reserve_memory, os::commit_memory and etc. the caller is required to
>> provide the 'memory type' information, which represents the VM subsystem
>> that the memory is allocated for. The 'memory type' is defined in
>> /src/share/memory/allocation.hpp. Also, a caller's pc is captured if NMT
>> tracking level is set to detail.
>>
>> There are a few ways to tag a memory block to a memory type:
>>
>> 1. os::malloc takes an extra parameter for the memory type.
>> 2. CHeapObj now is a template, it takes a memory type as template
>> parameter to tag the object to a specified memory type.
>> 3. Utility classes, such as Arena, GrowableArray and etc. the memory
>> type is a parameter of 'new' operator.
>> 4. Virtual memory block is tagged on its base address.
>>
>> When NMT intercepts the call, a memory record is created and serialized
>> by a sequence number, generated by global sequence generator. The memory
>> record is written to a per-thread recorder without a  lock, when calling
>> thread is a 'safepoint visible' JavaThread - a JavaThread that will
>> block at safepoint. Otherwise, ThreadCritical lock is acquired to write
>> to a global recorder. The recorders (or raw data) are synchronized at
>> some safepoints, and global sequence generator is also reset at
>> synchronization time, to avoid overflow the sequence number.
>>
>> A dedicated NMT worker thread is created to process the raw memory
>> records. It first stages a generation of raw data (a generation is
>> defined as span between resets of global sequence number), then promotes
>> the staged data to a global snapshot, which maintains information of all
>> 'live' memory block.
>>
>> NMT Tracking option has to be specified through VM command line option
>> -XX:NativeMemoryTracking (off by default), tracking level can not be
>> altered during runtime, but it can be shutdown through jcmd tool.
>>
>> NMT DCmd implements a set of sub-command to control NMT runtime and
>> request tracking data.
>>
>> Webrev: http://cr.openjdk.java.net/~zgu/6995781/webrev.00
>>
>> The tests and results:
>>
>>    - Passed internal smoke tests
>>    - JTreg test: http://cr.openjdk.java.net/~zgu/6995781/JTreport/
>>
>>
>> Additional notes:
>>     - Some classes' new operators are marked _NOINLINE_ (for the same
>> purpose as above), the performance runs, I performed, indicate that they
>> do not impact performance numbers.
>>
>>     - SA agent changes are in progress
>>
>> Thanks,
>>
>> -Zhengyu
>>
>>
>>