RFR: 8205607: Use oop_iterate instead of oop_iterate_no_header

[Stefan Karlsson]

On 2018-06-26 00:11, Kim Barrett wrote:
>> On Jun 25, 2018, at 8:25 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
>>
>> Hi all,
>>
>> Please review this patch to use oop_iterate instead of oop_iterate_no_headers.
>>
>> http://cr.openjdk.java.net/~stefank/8205607/webrev.01/
>> https://bugs.openjdk.java.net/browse/JDK-8205607
>>
>> oop_iterate_no_header is a convenience function that allows developers to pass in sub classes of OopClosures, instead of sub classes of OopIterateClosure, to the oop_iterate machinery.
>>
>> I propose that we remove this function, and change the few closures where this is used, to inherit from OopIterateClosure instead of OopClosure.
>>
>> Thanks,
>> StefanK
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/cms/compactibleFreeListSpace.cpp
> 2438 class VerifyAllOopsClosure: public BasicOopIterateClosure {
>
> Seeing this here and elsewhere, I wish BasicOopIterateClosure were
> called something like OopIterateNoMetadataClosure or
> NoMetadataOopIterateClosure. "Basic" doesn't really tell me much.
>
> If you agree, such a name change can be another RFE.

Me and Per went over a few alternatives. We wanted a short name and 
didn't like the negation in NoMetadata. We couldn't find a perfect name, 
and BasicOopIterateClosure was the least disliked name that we came up 
with. It's not an optimal name, but I hope that anyone reading this code 
will end up reading the class comment:

// An OopIterateClosure that can be used when there's no need to visit 
the Metadata.
class BasicOopIterateClosure : public OopIterateClosure {

But, yes, we can change the name if we can agree on a good name.

>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/cms/compactibleFreeListSpace.cpp
> 2527     // Iterate over all oops in the heap. Uses the _no_header version
> 2528     // since we are not interested in following the klass pointers.
> 2529     CMSHeap::heap()->oop_iterate(&cl);
>
> Comment needs updating; there isn't a _no_header version anymore.

Will fix.

>
> ------------------------------------------------------------------------------
>
> Some places that used to do oop_iterate_no_header now do oop_iterate
> using some arbitrary (based on signature type) OopIterateClosure.
>
> MutableSpace
> GenCollectedHeap
> PsOldSpace
>
> It's only by knowing the closure is a BasicOopIterateClosure that we
> know the metadata won't be processed. And that may require tracing
> through several levels of calls.

Yes, this is exactly what I want. I don't think the oop_iterate 
functions of the Space, Gen, or Heap classes should have to know, or 
care, if the passed in closure visits the metadata or not. I want that 
decision to be made by passing in the appropriate closure, which means 
that you need to know what your closure does.

> [There may also be a (small)
> performance cost associated with that in some cases, since we'll be
> calling the virtual do_metadata function for each of the objects being
> processed to discover the metadata should be skipped.  But I'm mostly
> concerned about knowing that we're dealing with a no-header iteration.]

There shouldn't be a difference here. The old code also took a virtual 
call for the do_metadata function.

>
> It might be clearer if the oop_iterate_no_header names were retained
> here, with the argument type being changed to BasicOopIterateClosure*.

I would prefer to not do that. This patch provides generic oop_iterate 
functions that can be used by both metadata visiting closures, and 
closures that don't visit metadata, and we have moved all responsibility 
to determine if metadata should be visited to the closures. I don't 
think we need to be extra restrictive here.

Thanks,
StefanK

>
> ------------------------------------------------------------------------------
>