Hi Ralf, Not a complete review yet. But this looks good. The seeking before seemed awkward.
Some remarks: In DumpWriter, _current_entry_left and _entry_ended seem only to be needed for asserting. Please enclose their definition in DEBUG_ONLY, and initialize them in the ctor. -- I like that DumperSupport::dump_object_array() does not write stuff anymore. That felt surprising. Still feels awkward that it warns about large arrays, seems more of a thing the caller should do. And that we have to pass in header size for it to do that. Not a big deal though. -- (not your patch): since DumperSupport::dump_class_and_array_classes(Klass*) should assert that Klass* is an InstanceKlass; or, even better, use InstanceKlass* as parameter. -- This was a bit of a brain teaser. More comments would be helpful. You wrote a good abstract in the JBS issue, could you please copy the proposed implementation as a comment into the class declaration of DumpWriter. -- DumpWriter::start_dump_entry(): It took me a while to understand how the segment size is updated if the entry is huge, since by the time we finish the entry the segment header will already be flushed out. The answer is, I think, that this is not needed since we only write one record so the initial size we wrote into the segment header is still valid. Proposed comment change: -// Will be fixed up later if we can add more entries. +// Seed segment size with size of its first record. Should we add more records later, we will update the segment size (see finish_dump_segment()) -- That's all I have for now. If there are still Reviewers missing after my vacation, I'll take another look. Cheers, Thomas On Mon, Nov 25, 2019 at 3:41 PM Schmelter, Ralf <ralf.schmel...@sap.com> wrote: > Hello, > > this change removes the need to use seek on the hprof file when creating a > heap dump, thus making it possible to stream the dump. This enables us to > dump to a socket or directly gzip the dump. > > Instead of fixing the heap dump segments size on the written file, the > size of the heap dump segments is either fixed up in the buffer instead or, > for entries to big to fit into the buffer fully, the entry get its own > segment with no need to fix up the segment size later. > > To do this, we now need to know how large an heap dump segment entry is > when starting to write the entry. This is either trivial (for the roots) or > already known (for the instance and array dump entries). Just the class > entry needed a little more code to track the size. > > The change results in more heap dump segments in the written heap dump. > But since the overhead per segment is 9 bytes, even for the smallest used > buffer (64K) the overhead is less than 0.02%. Additionally the heap dump > now expects to be able to allocate at least 64k for the buffer. The old > code tried to run even with a buffer of 1 byte or no buffer at all. > > Bugreport: https://bugs.openjdk.java.net/browse/JDK-8234510 > Webrev: http://cr.openjdk.java.net/~rschmelter/webrevs/8234510/webrev.0/ > > Best regards, > Ralf >
