RFR: 8337217: Port VirtualMemoryTracker to use VMATree

[Gerard Ziemski]

On Wed, 14 Aug 2024 19:13:49 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:

>> src/hotspot/share/nmt/memReporter.cpp line 467:
>> 
>>> 465: 
>>> 466:       if (reserved_and_committed)
>>> 467:         return;
>> 
>> This looks better, but now that I got more comfortable with anonymous local functions using lambda, I think we can do better. We can write a single `print_rgn` lambda function that we can use in both "reserved" and "committed" cases:
>> 
>> 
>>   outputStream* out = output();
>>   const char* scale = current_scale();
>>   auto print_rgn = [&](const VirtualMemoryRegion* rgn, const NativeCallStack* stack,
>>                        MEMFLAGS mem_type = MEMFLAGS::mtNone,
>>                        const char* region_type = "committed",
>>                        bool committed = true) {
>>     // Don't report if size is too small
>>     if (amount_in_current_scale(rgn->size()) == 0) return;
>>     out->cr();
>>     INDENT_BY(committed?8:0,
>>       print_virtual_memory_region(region_type, rgn->base(), rgn->size());
>>       if (stack->is_empty()) {
>>         out->cr();
>>       } else {
>>         if (!committed) {
>>           out->print(" for %s", NMTUtil::flag_to_name(mem_type));
>>         }
>>         out->print_cr(" from ");
>>         INDENT_BY(4, _stackprinter.print_stack(stack);)
>>       }
>>     )
>>   };
>
> The whole method here then becomes:
> 
> 
> void MemDetailReporter::report_virtual_memory_region(const ReservedMemoryRegion* reserved_rgn) {
>   assert(reserved_rgn != nullptr, "null pointer");
> 
>   // We don't bother about reporting peaks here.
>   // That is because peaks - in the context of virtual memory, peak of committed areas - make little sense
>   // when we report *by region*, which are identified by their location in memory. There is a philosophical
>   // question about identity here: e.g. a committed region that has been split into three regions by
>   // uncommitting a middle section of it, should that still count as "having peaked" before the split? If
>   // yes, which of the three new regions would be the spiritual successor? Rather than introducing more
>   // complexity, we avoid printing peaks altogether. Note that peaks should still be printed when reporting
>   // usage *by callsite*.
> 
>   if (reserved_rgn->flag() == mtTest) {
>     log_debug(nmt)("report vmem, rgn base: " INTPTR_FORMAT " size: " SIZE_FORMAT "cur-scale: " SIZE_FORMAT,
>       p2i(reserved_rgn->base()), reserved_rgn->size(),amount_in_current_scale(reserved_rgn->size()));
>   }
> 
>   outputStream* out = output();
>   const char* scale = current_scale();
>   auto print_rgn = [&](const VirtualMemoryRegion* rgn, const NativeCallStack* stack,
>                        MEMFLAGS mem_type = MEMFLAGS::mtNone,
>                        const char* region_type = "committed",
>                        bool committed = true) {
>     // Don't report if size is too small
>     if (amount_in_current_scale(rgn->size()) == 0) return;
>     out->cr();
>     INDENT_BY(committed?8:0,
>       print_virtual_memory_region(region_type, rgn->base(), rgn->size());
>       if (stack->is_empty()) {
>         out->cr();
>       } else {
>         if (!committed) {
>           out->print(" for %s", NMTUtil::flag_to_name(mem_type));
>         }
>         out->print_cr(" from ");
>         INDENT_BY(4, _stackprinter.print_stack(stack);)
>       }
>     )
>   };
> 
>   bool all_committed = reserved_rgn->size() == reserved_rgn->committed_size();
>   const char* region_type = (all_committed ? "reserved and committed" : "reserved");
>   print_rgn(reserved_rgn, reserved_rgn->call_stack(), reserved_rgn->flag(), region_type, false);
> 
>   if (all_committed) {
>     if (MemTracker::is_using_sorted_link_list()) {
>       CommittedRegionIterator itr = reserved_rgn->iterate_committed_regions();
>       const CommittedMemoryRegion* committed_rgn = itr.next();
>       if (committed_rgn->size() == reserved_rgn->...

Notice that by changing:

`INDENT_BY(4, stack->print_on(out);)`

to

`INDENT_BY(4, _stackprinter.print_stack(stack);)`

we went from:


        [0x0000000600000000 - 0x0000000620000000] committed 536870912 from
            [0x000000010ce40dfe]ReservedSpace::reserve(unsigned long, unsigned long, unsigned long, char*, bool)+0x2da[0x000000010ce41214]ReservedHeapSpace::try_reserve_heap(unsigned long, unsigned long, unsigned long, char*)+0x60[0x000000010ce413ac]ReservedHeapSpace::try_reserve_range(char*, char*, unsigned long, char*, char*, unsigned long, unsigned long, unsigned long)+0xae[0x000000010ce41571]ReservedHeapSpace::initialize_compressed_heap(unsigned long, unsigned long, unsigned long)+0x1b5


to


        [0x0000000600000000 - 0x0000000620000000] committed 536870912 from 
            [0x000000010be40d4e]ReservedSpace::reserve(unsigned long, unsigned long, unsigned long, char*, bool)+0x2da
            [0x000000010be41164]ReservedHeapSpace::try_reserve_heap(unsigned long, unsigned long, unsigned long, char*)+0x60
            [0x000000010be412fc]ReservedHeapSpace::try_reserve_range(char*, char*, unsigned long, char*, char*, unsigned long, unsigned long, unsigned long)+0xae
            [0x000000010be414c1]ReservedHeapSpace::initialize_compressed_heap(unsigned long, unsigned long, unsigned long)+0x1b5


which I think is now correct, and used to be wrong.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1717438580