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

[Thomas Stüfe]

Hi Coleen,

On Fri, Mar 1, 2019 at 1:22 AM <coleen.phillimore at oracle.com> wrote:

>
> Thomas,
>
>
> http://cr.openjdk.java.net/~stuefe/webrevs/8218988--improve-metaspace-verifications/webrev.01/webrev/src/hotspot/share/memory/metaspace/chunkManager.cpp.frames.html
>
> 269 #endif
>
> Nit, can you add // ASSERT to this endif.
>
>
> http://cr.openjdk.java.net/~stuefe/webrevs/8218988--improve-metaspace-verifications/webrev.01/webrev/src/hotspot/share/memory/metaspace/virtualSpaceNode.cpp.udiff.html
>
> + if (error) {
> + print_map(tty, is_class());
> + assert(false, "%s", err);
> + }
>
> I think you could use fatal() for this, instead of assert(false)
>
>
Ok.


> Are all the uses of EVERY_NTH under the MetaspaceExpand lock?
>
>
Almost all of them. They have to be. There is one section in
SpaceManager::allocate() which is only under its local metaspace lock but
it only verifies its metrics.


> This looks like a very nice cleanup of this code.  I don't need to see
> another webrev if you decide to change anything for the small comments
> above.
>
> Thanks for taking care of this code!
> Coleen
>

Thank you for the review! Will push it again thru jdk-submit, just to be
sure, then I will push.

..Thomas


>
> On 2/27/19 3:39 AM, Thomas Stüfe wrote:
> > Hi all,
> >
> > I'd love to get a second pair of eyes on this. Zhengyu already reviewed;
> > submit tests ran thru; patch is active in our nightlies w/o problems; I
> see
> > no regressions in the jtreg tests vulnerable to metaspace performance
> (e.g.
> > SelectionResolution).
> >
> > Latest webrev:
> >
> http://cr.openjdk.java.net/~stuefe/webrevs/8218988--improve-metaspace-verifications/webrev.01/webrev/
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8218988
> >
> > Thanks & Best Regards, Thomas
> >
> >
> > On Fri, Feb 15, 2019 at 11:12 AM Thomas Stüfe <thomas.stuefe at gmail.com>
> > 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
> >>
> >>
> >>
> >>
> >>
>
>