RFR: 8205607: Use oop_iterate instead of oop_iterate_no_header

[Kim Barrett]

> On Jun 25, 2018, at 7:25 PM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
> 
> 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.

I don't know what you might have tried.  Here are a few ideas, after
grovelling around with a thesaraus:

MetadataObliviousOopIterateClosure
MetadataIgnoringOopIterateClosure

OopIterateVisitingMetadataClosure (replacing MetadataVisitingOopIterateClosure)
OopIterateIgnoringMetadataClosure

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

The old code completely ignored do_metadata and the like, didn’t it?  Else what was the point of _no_header?

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

It's not obvious whether these oop_iterate_no_header were intended to
be a semantic distinction or simply an optimization.  But since all
the uses seem to be verification-like, it becomes harder to use an
optimization justification for the unusual API.  So I'm willing to buy
your argument that the semantic distinction isn't needed.

Looks good.