RFR(s-m): 8218988: Improve metaspace verifications

[zgu at redhat.com]

Hi Thomas,

1) It seems that it needs include "utilities/align.hpp" in
metaspaceCommon.hpp to make NON-PCH build happy.

2) There are several instances this->xxxx() calls in
virtualSpaceNode.cpp, you want to clean them up? seems that you are
doing some cleanup in this patch anyway.

3) chunkManager.hpp
I don't like this method: 
const ChunkList* free_chunks(ChunkIndex index) const;

I see it is only used in debug code, make it debug only? or just cast
away const of *this* pointer at call site?

Thanks,

-Zhengyu



On Fri, 2019-02-15 at 11:12 +0100, Thomas Stüfe wrote:
> Hi all,
> 
> may I please have reviews for the following change:
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8218988
> cr:
> http://cr.openjdk.java.net/~stuefe/webrevs/8218988--improve-metaspace
> -verifications/webrev.00/webrev/
> 
> In short, this reworks metaspace verifications.
> 
> Thorough verifications were added for JDK-8198423, but turned out too
> slow
> in general, so were made switchable later (-XX:+VerifyMetaspace) with
> a
> switch which is usually off. However, it would be good to have these
> verifications at least sometimes, so this patch changes
> VerifyMetaspace
> from a boolean to an interval which causes verifications every n
> passes
> thru a verification callsite. The default is 500, which does not slow
> down
> metaspace-heavy tests like SelectionResolution noticably.
> 
> This patch also contains a number of cleanups and simplifications. In
> particular:
> 
> class ChunkManager:
> 
> - removed sum_free_chunks(), sum_free_chunks_count(),
> locked_verify_free_chunks_total() and
> locked_verify_free_chunks_count().
> These functions were used to validate the counters in ChunkManager.
> But
> this way was inefficient, since we walk the lists twice. The counter
> validation is now done in ChunkManager::verify() and only requires
> one list
> walk. Also, less code.
> - Now we only have one verification function left,
> ChunkManager::verify().
> To distinguish between quick counter verify and deep costly verify, I
> gave
> it a "bool slow" argument.
> - all verifications are now #ifdef ASSERT. In fact, they were before
> too
> since we used assert() inside the verification functions.
> - expensive verifications are now done depending on
> VerifyMetaspaceInterval.
> - I removed expensive verifications from some hot paths (e.g.
> ChunkManager::chunk_freelist_allocate). Verifications are now done
> only
> when interesting things happen: chunk fuses or splits, or a
> Classloader
> died and releases its SpaceManager.
> 
> minor stuff:
> 
> - made getters for counters (free_chunks_total_words(),
> free_chunks_total_bytes(), free_chunks_count()) inline
> - removed locked_print_sum_free_chunks(), which was nowhere used.
> - constified versions of free_chunks() and humongous_dictionary()
> 
> class VirtualSpaceNode:
> 
> - removed container_count_slow() and verify_container_count() since
> it
> duplicates the verifications we do in VirtualSpaceNode::verify().
> - similar to ChunkManager, gave VirtualSpaceNode::verify a "bool
> slow"
> parameter to distinguish between quick and thorough verification.
> - modified VirtualSpaceNode::verify_free_chunks_are_ideally_merged()
> to
> print out a map of the node if it fails, before asserting.
> 
> Miscellanous:
> 
> - added some new internal counters to the statistic (see
> metaspaceCommon.hpp)
> 
> 
> Gtests: test_virtualSpaceNode.cpp
> 
> - renamed ChunkManagerTest to ChunkManagerTestAccessor to make its
> (narrow)
> intent clearer
> - removed parts which had been commented out since chunk coalescation
> (8198423) and which would never really work, plus code which relied
> on it.
> 
> Note: IMHO the value of these tests is a bit diminished and probably
> not
> worth the trouble of maintaining? To compensate for that I did add
> test_metaspace_allocation.cpp some time ago which is more thorough
> and less
> vulnerable against changes in the metaspace implementation.
> 
> ----
> 
> Tests: I stressed my patch locally (linux x64) with my own metaspace
> tests
> as well as hotspot/jtreg/runtime/*, including the SelectionResolution
> tests. Before pushing, I will put the patch through our nightlies.
> 
> Thank you, Thomas