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

[Thomas Stüfe]

On Wed, Apr 3, 2019 at 11:22 AM Doerr, Martin <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>
> *Sent:* Mittwoch, 3. April 2019 10:57
> *To:* Coleen Phillmore <coleen.phillimore at oracle.com>; Andrew Dinn <
> adinn at redhat.com>; Doerr, Martin <martin.doerr at sap.com>
> *Cc:* Hotspot dev runtime <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> 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>
> 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
>
>
>
>
>
>
>
>