RFR(s): 8221539: [metaspace] Improve MetaspaceObj::is_metaspace_obj() and friends

[coleen.phillimore at oracle.com]

This looks good, with the unnecessary null checks removed.  I don't need 
to see another version but do a sanity build before pushing please!
Thanks!
Coleen


On 4/3/19 4:57 AM, Thomas Stüfe wrote:
> Hi all,
>
> new version:
>
> Delta:
> http://cr.openjdk.java.net/~stuefe/webrevs/8221539--%5bmetaspace%5d-improve-metaspaceobj--is_metaspace_obj()-and-friends/delta_to_4/webrev/index.html
>
> Full:
> http://cr.openjdk.java.net/~stuefe/webrevs/8221539--%5bmetaspace%5d-improve-metaspaceobj--is_metaspace_obj()-and-friends/webrev.04/webrev/
>
> Changes:
>
> - As Coleen wished, I completely removed the non-static variant of 
> MetaspaceObj::is_metaspace_obj() and fixed the callers.
> - I also renamed the static variant of 
> MetaspaceObj::is_metaspace_obj() to MetaspaceObj::is_valid() to be in 
> line with similar calls, e.g. Symbol::is_valid().
>
> @Coleen: This envelope should only weed out obvious non-null bogus 
> values and hopefully stack and C-heap addresses; my hope is that nodes 
> come and go but that the total envelope size will be always minuscule 
> compare to the 64bit address range and outside C-heap and stacks. 
> Usually mmap regions are clustered, as are C-Heap allocations and stacks.
>
> But if that turns out to be inefficient after a while, we may 
> recalculate the envelope; just have to make sure no concurrent 
> lock-less walks happen.
>
> Thanks, Thomas
>
>
>
> On Tue, Apr 2, 2019 at 2:21 PM <coleen.phillimore at oracle.com 
> <mailto:coleen.phillimore at oracle.com>> wrote:
>
>
>
>     On 4/2/19 1:47 AM, Thomas Stüfe wrote:
>>     Hi Coleen, Andrew,
>>
>>     thank you for reviewing my little change. Unfortunately, I had an
>>     error in the space list verification method which needed fixing,
>>     so here is a second version:
>>
>>     http://cr.openjdk.java.net/~stuefe/webrevs/8221539--%5bmetaspace%5d-improve-metaspaceobj--is_metaspace_obj()-and-friends/webrev.01/webrev/
>
>>
>>     Differences:
>>     - As Coleen requested: in allocation.cpp I replaced the
>>     comparison this==NULL with a static helper method
>
>     I think you have to change the callers to not pass this as null. 
>     So you can't do metaspaceobj->is_metaspace_object() because you're
>     calling with "this" potentially NULL.
>
>     So remove this function:
>
>       bool MetaspaceObj::is_metaspace_object() const {
>     - return Metaspace::contains((void*)this);
>     + return MetaspaceObj::is_metaspace_object(this);
>       }
>
>
>>     - I had mistype "envelope" as "envolope" in
>>     "expand_envelope_to_include_node()". Since that sounded funny I
>>     changed it.
>>     - The real bug was in VirtualSpaceList::verify() where I checked
>>     that the extension of the envelope is as large as the current
>>     nodes. But that is wrong, since the envelope never is shrunk (by
>>     design) and nodes at the border of the envelope may have been
>>     unmapped. So the real test should be to test if no node is
>>     outside the envelope.
>
>     So this envelope is an interesting concept and name.  It seems
>     okay.  I guess over time, it won't give you a very good answer. 
>     Maybe you'll have to fix the boundaries someday.
>
>     Looks good though.  Thank you for making this improvement for
>     performance.
>
>     Coleen
>>
>>     Thanks, Thomas
>>
>>
>>     On Wed, Mar 27, 2019 at 10:01 PM Thomas Stüfe
>>     <thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com>> wrote:
>>
>>         Hi all,
>>
>>         May I please have reviews for this small optimization:
>>
>>         cr:
>>         http://cr.openjdk.java.net/~stuefe/webrevs/8221539--%5bmetaspace%5d-improve-metaspaceobj--is_metaspace_obj()-and-friends/webrev.00/webrev/index.html
>>         Issue: https://bugs.openjdk.java.net/browse/JDK-8221539
>>
>>         There are several functions which, given an unknown pointer
>>         assumed to be a metaspace object, check if the pointer is
>>         indeed a metaspace object by walking the VirtualSpaceList and
>>         checking ranges.
>>
>>         This patch adds checks which weed out the obvious cases to
>>         avoid needlessly walking the vs list.
>>
>>         Patch also adds verifications for the VirtualSpaceList in
>>         debug cases. Those run only when a new node has been added to
>>         the list, or when a node has been purged, so very sparingly.
>>
>>         When purging nodes, I removed a small unnecessary and
>>         inefficient check which checked whether (one of the) purged
>>         nodes was still in the list. Since we now as part of the new
>>         VirtualSpaceNode::verify() walk this list, the check is
>>         unnecessary.
>>
>>         Thanks, Thomas
>>
>>
>