RFR (L) 8028541: Native Memory Tracking enhancement

[Zhengyu Gu]

Coleen,

Thanks for the review.

On 6/11/2014 7:21 PM, Coleen Phillimore wrote:
>
> Hi Zhengyu,
>
> I have early feedback but I haven't looked at the new files yet. 
> Nothing major.  I like the new code and I'm glad you could add NMT 
> tracking to vmError reporting.
>
> http://cr.openjdk.java.net/~zgu/8028541/webrev.00/src/os/solaris/vm/os_solaris.cpp.udiff.html 
>
>
> + bool os::unsetenv(const char* name) {
> +  assert(name != NULL, "Null pointer");
> +  return (::unsetenv(name) == 0);
> + }
> +
>
> Can you add this to os_posix.cpp instead?  Someday this duplicate code 
> will be consolidated so it can start in posix even though it's not posix.
>
OK.  I assume that aix can also use os_posix functions, right?

> http://cr.openjdk.java.net/~zgu/8028541/webrev.00/src/os/windows/vm/os_windows.cpp.udiff.html 
>
>
> I thought the original comment was more descriptive.  Is it still a 
> workaround?
>
I think we still need this workaround. Auto-merge overwrote Christian's 
comment, I will restore.

> http://cr.openjdk.java.net/~zgu/8028541/webrev.00/src/share/vm/runtime/arguments.cpp.html 
>
>
> Can you factor lines 3596-3623 into a function like verify_nmt_flag?  
> Is it that you already know what nmt level is used from the launcher 
> and are just checking consistency and initializing here?  These 
> cascading if() code checking options gets too long if you're not careful.

Will do.
>
> In memReporter.hpp
>
> A short comment before each class what they are for and how you would 
> get to this code would be nice.  I think they correspond to NMT 
> settings (summary vs. detail).  Mem*DiffReporter reports the 
> difference between the current memory tracked and a previous snapshot 
> it appears.
Will do.
>
>
> In memReporter.cpp
>
> Lines 302-311, you could have two functions that does this same 
> calculation for these values so that it doesn't get broken.  The same 
> calculation appears 61 and 64.
>
> If you use size_t can you avoid eagerly scaling the amounts? 
> amount_in_current_scale appears 66 times which is a lot.  Can you only 
> call the scaling when you print the amounts?
I think that is what I did.  All calculations are done in byte scale, 
only converts to current scale when checks amount > 0 in current scale, 
and print the number.

> In MemTracker.hpp
>
> Line 109, I think you can assert that the JVM is single threaded.
>
What's the reliable method to check JVM single thread mode? I tried in 
early NMT implementation, but it was not reliable.

Thanks,

-Zhengyu

> That's as far as I got today.  This looks good!
>
> Coleen
>
> On 5/22/14, 3:19 PM, Zhengyu Gu wrote:
>> This is significant rework of native memory tracking introduced in 
>> earlier releases.
>>
>> The goal of this enhancement is to improve scalability, from both 
>> tracking memory and CPU usage perspectives, so it can scale well with 
>> increased memory allocation in large applications.
>>
>> The enhancement is mainly focused on malloc memory tracking, whose 
>> activities are several magnitude higher than virtual memory, and was 
>> the main bottleneck in early implementation.
>>
>> Instead of using book keeping records for tracking malloc activities, 
>> new implementation co-locates tracking data along side with user data 
>> by using a prefixed header. The header size is 8 bytes on 32-bit 
>> systems and 16 bytes on 64-bit systems, which ensure that user data 
>> also align properly.
>>
>> Virtual memory tracking still uses book keeping records, and 
>> ThreadCritical lock is always acquired to alter the records and 
>> related data structures.
>>
>> Summary tracking data is maintained in static data structures, via 
>> atomic operations. Malloc detail tracking call stacks are maintained 
>> in a lock free hashtable.
>>
>> The key improvements:
>>   1. Up-to-date tracking report.
>>   2. Detail tracking now shows multiple call frames. Number of frames 
>> is compilation time decision, currently default to 4.
>>   3. Malloc tracking is lock free.
>>   4. Tracking summary is reported in hs_err file when native memory 
>> tracking is enabled.
>>   5. Query is faster, uses little memory and need a very little process.
>>
>> The drawback is that, malloc tracking header is always needed if 
>> native memory tracking has ever been enabled, even after tracking is 
>> shutdown.
>>
>> Impacts:
>>   The most noticeable impact for JVM developers, is that Arena now 
>> also take memory type as constructor parameter, besides the new 
>> operators.
>>      Arena* a = new (mtCode) Arena()  => Arena* a = new (mtCode) 
>> Arena(mtCode)
>>
>> The webrev shows modification of about 60 files, but most of them are 
>> due to tracking API changes, mainly due to tracking stack, now, is an 
>> object, vs. a single pc.
>>
>> The most important files for this implementations are:
>>
>> memTracker.hpp/cpp
>> mallocTracker.hpp/cpp and mallocTracker.inline.hpp
>> virtualMemoryTracker.hpp/cpp
>> mallocSiteTable.hpp/cpp
>>
>> allocationSite.hpp
>> nativeCallStack.hpp/cpp
>> linkedlist.hpp
>>
>>
>> Tests:
>>   - JPRT
>>   - NMT test suite
>>   - vm.quick.testlist
>>   - Kitchensink stability test for 16+ days
>>   - FMW
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8028541
>> Webrev: http://cr.openjdk.java.net/~zgu/8028541/webrev.00/
>>
>> Thanks,
>>
>> -Zhengyu
>>
>