RFR: 8138737: Remove oop_ms_adjust_pointers and use oop_iterate instead

[Kim Barrett]

> On Apr 11, 2017, at 9:15 AM, Stefan Johansson <stefan.johansson at oracle.com> wrote:
> 
> Hi,
> 
> Please review this change for:
> https://bugs.openjdk.java.net/browse/JDK-8138737
> 
> Webrev:
> http://cr.openjdk.java.net/~sjohanss/8138737/hotspot.00/
> 
> Summary:
> For the mark-sweep full GCs we need to handle the adjust-closure special for InstanceRefKlass. Before this change this was done by having a special method for all *Klass, oop_ms_adjust_pointers and instead of calling oop_iterate(AdjustClosure) we called this method. Since it is only InstanceRefKlass that needs the special handling it would be nice to be able to use the normal oop_iterate code path and only special case InstanceRefKlass.
> 
> This change removes all use of oop_ms_adjust_pointers and makes it possible to special handle InstanceRefKlass by defining a special ReferenceIterationMode for the closure in need of the special treatment. This technique can also be used to remove ExtendedOopClosure::apply_to_weak_ref_discovered_field (descirbed in JDK-8138888) and I plan to send out a review for this once this review is done. The fix for that issue is basically adding one more ReferenceIterationMode and define what it should do.
> 
> Testing:
> - Locally running a lot of Full GCs
> - RBT hs tier 1-3
> 
> Thanks,
> Stefan

Generally like where this is going.  A few minor comments.

------------------------------------------------------------------------------  
The usual copyright reminder.

------------------------------------------------------------------------------  
src/share/vm/memory/iterator.hpp
  75   // The current default iteration mode is to do discovery.
  76   virtual ReferenceIterationMode reference_iteration_mode() { return DO_DISCOVERY; }

I think I would prefer a pure virtual declaration here, with no
default. Defaults are good when there is an obvious (and usually safe)
value to use, but I'm not sure that's the case here. Though looking
around, it does seem there is presently only one closure that
overrides it. But I wonder how much of that is because many closures
don't have an associated ReferenceProcessor, and so don't really do
discovery. Though what is done then is kind of complicated right now.
But I have a change waiting for some stuff to circulate from jdk9/dev
to jdk10/hs that I think removes that complexity.

I guess that's a long-winded way of saying okay for now, but we should
keep an eye on this.

------------------------------------------------------------------------------ 
src/share/vm/gc/serial/markSweep.inline.hpp
Removed:
  44 inline int MarkSweep::adjust_pointers(oop obj) {
  45   return obj->ms_adjust_pointers();
  46 }

Is it really worth eliminating MarkSweep::adjust_pointers?
Seems like it's still a convenient shorthand to retain, just with a
different definition.

Keeping it would eliminate the changes to several of the files in this
changeset. It might also let the closure variable be private?

------------------------------------------------------------------------------ 
src/share/vm/gc/shared/specialized_oop_closures.hpp
  95   f(AdjustPointerClosure, _nv)

I prefer the space before _nv here, but every other similar form in
this file is lacking that space.

------------------------------------------------------------------------------ 
src/share/vm/oops/instanceRefKlass.hpp
 123   // Default way of handling instance ref klasses, does reference
 124   // processing if a ReferenceProcessor has been supplied.
 125   template <bool nv, typename T, class OopClosureType, class Contains>
 126   static void oop_oop_iterate_discovery(oop obj, ReferenceType type, OopClosureType* closure, Contains& contains);
 127 
 128   // Just handle the fields, don't care about reference processing.
 129   template <bool nv, typename T, class OopClosureType, class Contains>
 130   static void oop_oop_iterate_fields(oop obj, OopClosureType* closure, Contains& contains);

I wouldn't describe the discovery case as a "default", nor the fields
case as "just". And what does the discovery handler do if no reference
processor is provided? It's probably equivalent to the fields handler
in that case. And the discovery case pays attention to
apply_to_weak_ref_discovred_field.

------------------------------------------------------------------------------ 
src/share/vm/oops/instanceRefKlass.inline.hpp
  81   log_develop_trace(gc, ref)("Process reference default " PTR_FORMAT, p2i(obj));

Again with the "default".

------------------------------------------------------------------------------ 
src/share/vm/oops/instanceRefKlass.hpp
 132 #ifdef ASSERT
 133   template <typename T>
 134   static void trace_reference_gc(const char *s, oop obj, T* referent_addr, T* next_addr, T* discovered_addr);
 135 #endif

Perhaps make this DEBUG_RETURN, and unconditionalize calls?

------------------------------------------------------------------------------ 
src/share/vm/oops/instanceRefKlass.hpp
  53   template <bool nv, typename T, class OopClosureType, class Contains>
  54   friend class InstanceRefKlassSpecialized;

I don't see this class defined or used anywhere.

------------------------------------------------------------------------------