RFR: 8205607: Use oop_iterate instead of oop_iterate_no_header

[Stefan Karlsson]

On 2018-06-26 09:12, Kim Barrett wrote:
>> 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

Here are some of the names on our whiteboard:

PureOopIterateClosure
BasicOopIterateClosure
PlainOopIterateClosure
StandardOopIterateClosure
NoMetadataOopIterateClosure
WithoutMetadataOopIterateClosure
MetadataAgnosticOopIterateClosure
MetadataIgnoringOopIterateClosure

MetadataOopIterateClosure
MetadataAwareOopIterateClosure
MetadataVisitingOopIterateClosure

OopIterateClosureWithMetadata
OopIterateClosureWithoutMetadata

OopIterateWithMetadataClosure
OopIterateWithoutMetadataClosure

OopIterateMetadataClosure
OopIterateNoMetadataClosure

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

No. See:

-int oopDesc::oop_iterate_no_header(OopClosure* blk) {
-  // The NoHeaderExtendedOopClosure wraps the OopClosure and proxies all
-  // the do_oop calls, but turns off all other features in 
OopIterateClosure.
-  NoHeaderExtendedOopClosure cl(blk);
-  return oop_iterate_size(&cl);
-}

  The old code wrapped OopClosure in another closure 
(NoHeaderExtendedOopClosure) that turned off the metadata visiting 
parts. The oop_iterate framework still called the virtual do_metadata 
that NoHeaderExtendedOopClosure inherited from ExtendedOopClosure.

I added them during the permgen removal so that we wouldn't have to 
change all OopClosures into ExtendedOopClosure, and thereby didn't have 
to deal with all the baggage that ExtendedOopClosure brought. Now that 
we are creating a leaner OopIterateClosure, I'd like to get rid of this.

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

It was added as a convenience wrapper, so that closures that didn't need 
to care about metadata could remain inheriting from OopClosures instead 
of ExtendedOopClosure.

Regarding the name, do_metadata used to be named do_header when it only 
guarded the klass pointer in objects:
http://hg.openjdk.java.net/jdk6/jdk6/hotspot/file/abf71d5a0e42/src/share/vm/oops/instanceKlass.cpp

// closure's do_header() method dicates whether the given closure should be
// applied to the klass ptr in the object header.

#define InstanceKlass_OOP_OOP_ITERATE_DEFN(OopClosureType, nv_suffix)  \
                                                                        \
int instanceKlass::oop_oop_iterate##nv_suffix(oop obj, OopClosureType* 
closure) { \

SpecializationStats::record_iterate_call##nv_suffix(SpecializationStats::ik);\
   /* header */                                                         \
   if (closure->do_header()) {                                          \
     obj->oop_iterate_header(closure);                                  \
   }                                                                    \
   InstanceKlass_OOP_MAP_ITERATE(                                       \
     obj,                                                               \
     SpecializationStats::                                              \
       record_do_oop_call##nv_suffix(SpecializationStats::ik);          \
     (closure)->do_oop##nv_suffix(p),                                   \
     assert_is_in_closed_subset)                                        \
   return size_helper();                                                \
}

> 
> Looks good.

Thanks!

StefanK

> 
> 
>