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

[coleen.phillimore at oracle.com]

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)

Are all the uses of EVERY_NTH under the MetaspaceExpand lock?

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

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