RFR: 8229278: Improve hs_err location printing to assume less about GC internals

Kim Barrett kim.barrett at oracle.com
Tue Aug 27 23:07:35 UTC 2019


> On Aug 27, 2019, at 10:49 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
> 
> Hi Stefan,
> 
> Thank you for the review. I like your cleanups. I folded them in to the next webrev: http://cr.openjdk.java.net/~eosterlund/8229278/webrev.01/

------------------------------------------------------------------------------

It's kind of annoying that we have both locationPrinter.hpp and
locationPrinter.inline.hpp.  I think locationPrinter.hpp probably
isn't useful without the .inline file also being included.

I was going to suggest merging into the .hpp and eliminating the
.inline.hpp, but noticed the use of compressedOops.inline.hpp.
Maybe go the other way, no .hpp file?  That's a little odd, but I
think there are some other places where that's been done.

Your call on doing anything with this comment.

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/collectedHeap.hpp

Removed pure virtual block_start and block_is_obj.  However, some of
the derived classes still (now unnecessarily) declare their
implementations virtual.  (Too bad they aren't declared using the
C++11 "override" virtual specifier.)

------------------------------------------------------------------------------
src/hotspot/share/gc/z/zCollectedHeap.cpp
352 bool ZCollectedHeap::print_location(outputStream* st, void* addr) const {
353   if (LocationPrinter::is_valid_obj(addr)) {
...
358   }
359   return false;
360 }

ZGC won't attempt to determine whether addr is a pointer into the
middle of an object and print something useful in that case, correct?

I'm not sure whether it was previously attempting to do so either; I
don't really want to study the code being deleted. :)

But that seems like a loss of useful functionality compared to the
other collectors, assuming it's actually possible to do.  Which I'm
guessing it isn't?

------------------------------------------------------------------------------

This looks good, except for the now extraneous virtual specifiers.  I
don't need a new webrev for that.




More information about the hotspot-gc-dev mailing list