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

[zgu at redhat.com]

On Fri, 2019-02-15 at 13:54 -0500, zgu at redhat.com wrote:
> 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;
4) Also is_valid_chunksize(bool is_class, size_t size) can be a static
or const method.

Thanks,

-Zhengyu

> 
> 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-metaspa
> > ce
> > -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