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.
> 
> Good catch.
>  
> > > 
> > 
> > > 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.
> 
> I cleaned them up. Sometimes I leave them in for documentation
> purposes, but yes they clutter the code.
>  

Thanks for fixing.
> > > 
> > 
> > > 3) chunkManager.hpp
> > 
> > > I don't like this method: 
> > 
> > > const ChunkList* free_chunks(ChunkIndex index) const;
> 
> Ok, I removed it and changed the callsite.
> 
> Not really happy with it. Unfortunately there is no good alternative
> to const overloading, even though it is ugly... But I can live with
> this solution.
>  
Me too.
> > 4) Also is_valid_chunksize(bool is_class, size_t size) can be a
> > static
> > 
> > or const method.
> 
> 
> 
> It is a global function, not a class method. I can make it static but
> that would maybe mean something different that you intended?
> 
I must have misread this as a member function.
Looks good to me.
Thanks,
-Zhengyu
> Thanks again, 
> Thomas
>  
> > 
> > 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-met
> > aspa
> > 
> > > > 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
> >