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

[Doerr, Martin]

Hi Thomas,

> Yeah, I left them in out of a vague notion of "documentation". Can remove them if you care.
I’d prefer shorter code.

Best regards,
Martin


From: Thomas Stüfe <thomas.stuefe at gmail.com>
Sent: Mittwoch, 3. April 2019 11:43
To: Doerr, Martin <martin.doerr at sap.com>
Cc: Coleen Phillmore <coleen.phillimore at oracle.com>; Andrew Dinn <adinn at redhat.com>; Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net>
Subject: Re: RFR(s): 8221539: [metaspace] Improve MetaspaceObj::is_metaspace_obj() and friends



On Wed, Apr 3, 2019 at 11:22 AM Doerr, Martin <martin.doerr at sap.com<mailto:martin.doerr at sap.com>> wrote:
Hi Thomas,

thank you for doing all the work. I appreciate the improvement.

Only question I have is do we want the now redundant NULL checks before calling MetaspaceObj::is_valid?
(I don’t need to see another webrev if you just want to get rid of them.)


Yeah, I left them in out of a vague notion of "documentation". Can remove them if you care.

Thanks, Thomas


Best regards,
Martin



From: Thomas Stüfe <thomas.stuefe at gmail.com<mailto:thomas.stuefe at gmail.com>>
Sent: Mittwoch, 3. April 2019 10:57
To: Coleen Phillmore <coleen.phillimore at oracle.com<mailto:coleen.phillimore at oracle.com>>; Andrew Dinn <adinn at redhat.com<mailto:adinn at redhat.com>>; Doerr, Martin <martin.doerr at sap.com<mailto:martin.doerr at sap.com>>
Cc: Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net<mailto:hotspot-runtime-dev at openjdk.java.net>>
Subject: Re: RFR(s): 8221539: [metaspace] Improve MetaspaceObj::is_metaspace_obj() and friends

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