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