Quantcast

RFR: 8138737: Remove oop_ms_adjust_pointers and use oop_iterate instead

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR: 8138737: Remove oop_ms_adjust_pointers and use oop_iterate instead

Stefan Johansson
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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8138737: Remove oop_ms_adjust_pointers and use oop_iterate instead

Kim Barrett
> On Apr 11, 2017, at 9:15 AM, Stefan Johansson <[hidden email]> 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.

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8138737: Remove oop_ms_adjust_pointers and use oop_iterate instead

Stefan Johansson
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 <[hidden email]> 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


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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8138737: Remove oop_ms_adjust_pointers and use oop_iterate instead

Kim Barrett
> On Apr 24, 2017, at 9:53 AM, Stefan Johansson <[hidden email]> wrote:
> 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/

This looks good.

Loading...