[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    RE: RFR (M) 8234510: Remove file seeking requirement for writing a heap dump
From:       "Schmelter, Ralf" <ralf.schmelter () sap ! com>
Date:       2019-12-20 15:23:37
Message-ID: AM0PR02MB45005A0CA4DFEC3ADF9329159F2D0 () AM0PR02MB4500 ! eurprd02 ! prod ! outlook ! com
[Download RAW message or body]

Hi Christoph,

thanks for the review.

> I think it would ease understanding of the code if you would also
> create a method DumpWriter::start_dump_segment().

That's a feature, so you only have to check in one place.

> 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?

I would hope the compiler is smart enough to eliminate the call.

> Then, I've spotted a little spelling error: line 623 - segement should be segment.

Will fix it

> 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.

I think you're right. I will rename it accordingly.

> 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.

I always did the size calculations in the same order the entries are written in, so \
it should be clear which term corresponds to which write statement. I have to try out \
how long the comments would get and if they facilitate a better understanding.

Best regards,
Ralf


[prev in list] [next in list] [prev in thread] [next in thread]