RFR: 8253909: Implement detailed map file for CDS

[Ioi Lam]

Hi Thomas,

Thanks for the review!

On 10/1/20 11:26 PM, Thomas Stuefe wrote:
> Not a full review, just some drive-by comments.
>
> src/hotspot/share/memory/archiveBuilder.cpp line 723:
>
>> 721:   // Dump all the data [base...top). Pretend that the base address
>> 722:   // will be mapped to runtime_base at run-time.
>> 723:   static void write_data(address base, address top, address runtime_base) {
> Consider printing the data with os::print_hex_dump() instead. Would slim down this coding.

Fixed. I had to modify os::print_hex_dump() to optionally print a 
"logical" address.

> src/hotspot/share/memory/archiveBuilder.cpp line 769:
>
>> 767:       log_info(cds, map)("Log level = info");
>> 768:       log_info(cds, map)("Run with -Xlog:cds+map=debug for more detailed information");
>> 769:       log_info(cds, map)("Run with -Xlog:cds+map=trace for even more detailed information");
> Matter of taste, but I do not think usage information belongs into a log.

Removed

> src/hotspot/share/memory/archiveBuilder.cpp line 759:
>
>> 757:              GrowableArray<MemRegion> *closed_heap_regions,
>> 758:              GrowableArray<MemRegion> *open_heap_regions,
>> 759:              char* bitmap, size_t bitmap_size_in_bytes) {
> Consider, instead of directly writing to UL, writing to an outputStream*. That would make the code more versatile and
> reusable. You can then use LogStream to print to UL with the same result.

UL allows several logging levels. I don't know how to do that with 
outputStream (without passing 3 different outputStreams).


> src/hotspot/share/memory/archiveBuilder.cpp line 609:
>
>> 607: // picked by the OS. At runtime, we try to map at a fixed location (SharedBaseAddress). For
>> 608: // consistency, we log everything using runtime addresses.
>> 609: class ArchiveBuilder::CDSMapLogger {
> AllStatic?

Fixed

> src/hotspot/share/memory/archiveBuilder.cpp line 775:
>
>> 773:     address header_end = header + mapinfo->header()->header_size();
>> 774:     write_region("header", header, header_end, 0);
>> 775:     write_data(header, header_end, 0);
> Consider writing out the header members human-readable, that would be more useful than a hexdump. I'd also print them
> already at Info level, since often the header info is the only thing interesting.

Done. I attached the updated example logs to the bug report.

> src/hotspot/share/memory/filemap.hpp line 461:
>
>> 459:   void  write_region(int region, char* base, size_t size,
>> 460:                      bool read_only, bool allow_exec);
>> 461:   char* write_bitmap_region(const CHeapBitMap* ptrmap,
> Can you add a comment here and in MetaspaceShared::write_core_archive_regions() that the returned bitmap is c-heap
> allocated and needs to be freed?

Fixed.

Thanks
- Ioi