RFR: 8138737: Remove oop_ms_adjust_pointers and use oop_iterate instead

Mon Apr 24 13:53:25 UTC 2017

Thanks Kim for looking at this,

On 2017-04-21 22:02, Kim Barrett wrote:
>> 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.
I agree with this, or at least that we should have a different default.

>
> ------------------------------------------------------------------------------
> 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.
It probably isn't anymore. I had later changes that would have benefited 
from not going through MarkSweep, and then it would have made more sense 
to do the separation here. But this code has changed recently and I 
actually don't need this anymore. I will revert this. Good catch!

> Keeping it would eliminate the changes to several of the files in this
> changeset. It might also let the closure variable be private?
No, it is still used from some other classes, G1MarkSweep for example.

>
> ------------------------------------------------------------------------------
> 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.
I agree, fixed.

>
> ------------------------------------------------------------------------------
> 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.
I've updated the wording. I also removed just from iterator.hpp.

Without a reference processor the discovery handler still differs a bit 
because it looks at the next field to determine whether or not to handle 
the discovered 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".
Changed.

> ------------------------------------------------------------------------------
> 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?
It's always hard to get those right, but I was just told there is a 
NOT_DEBUG_RETURN which can be used here. So I changed to use that and 
removed debug_only from the call.

>
> ------------------------------------------------------------------------------
> 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.
Nice catch, that was from a different idea I had :)

Thanks again for the review here are the new webrevs:
Inc: http://cr.openjdk.java.net/~sjohanss/8138737/hotspot.00-01/
Full: http://cr.openjdk.java.net/~sjohanss/8138737/hotspot.01/

Thanks,
Stefan

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