On Wed, 14 Aug 2024 19:13:49 GMT, Gerard Ziemski <gziem...@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
