Hi Ralf, I've spent some time with your change now as well.
Overall, looks good. I only have some minor findings: There are 3 new methods: void DumpWriter::finish_dump_segment() void DumpWriter::start_sub_record() void DumpWriter::end_sub_record() I think it would ease understanding of the code if you would also create a method DumpWriter::start_dump_segment(). The work that should be done in there is currently done implicitly in DumpWriter::start_sub_record(). Also, all that DumpWriter::end_sub_record() does is asserting and debug only. So, maybe the whole method and its calls could be enclosed in debug_only? Then, I've spotted a little spelling error: line 623 - segement should be segment. Then, _sub_record_ended could be changed to _in_sub_record and the according semantics be adapted. I found that to be more understandable - but maybe it's just a personal taste thing. And a last point is that there are many places where sizes are calculated, e.g. lines 1002, 1032-1036, 1081, 1116, 1174, 1175. Here, I think code comments could be added/improved to facilitate quicker understanding for the folks that are ingenuous to this code. Cheers Christoph > -----Original Message----- > From: serviceability-dev <[email protected]> On > Behalf Of Schmelter, Ralf > Sent: Montag, 16. Dezember 2019 13:28 > To: Schmelter, Ralf <[email protected]>; Thomas Stüfe > <[email protected]> > Cc: OpenJDK Serviceability <[email protected]>; hotspot- > [email protected] > Subject: [CAUTION] RE: RFR (M) 8234510: Remove file seeking requirement > for writing a heap dump > > I forgot to post the updated webrev: > http://cr.openjdk.java.net/~rschmelter/webrevs/8234510/webrev.2/ > > In addition to the changes requested by Thomas, I also renamed the entries > in the heap dump segment from entries to sub-records, since that is what > they are called in the comment describing the format. > > Best regards, > Ralf
