RFR: 8203028: Simplify reference processing in light of JDK-8175797
Kim Barrett
kim.barrett at oracle.com
Tue May 15 13:48:03 UTC 2018
> On May 15, 2018, at 4:17 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
>
> Hi,
>
> On Mon, 2018-05-14 at 18:09 -0400, Kim Barrett wrote:
>>> On May 14, 2018, at 8:45 AM, Thomas Schatzl <thomas.schatzl at oracle.
>>> com> wrote:
>>>
>>> Hi Kim,
>>>
>>> some initial comments…
>>
>> Thanks for looking at this.
>>
>>> On Sun, 2018-05-13 at 21:32 -0400, Kim Barrett wrote:
>>>> […]
>>>> CR:
> [...]
>
>
>>> - please remove the unused complete_gc parameter from
>>> ReferenceProcessor::process_phase2.
>>
>> I intentionally left it alone. Removing it completely starts getting
>> messy when one starts walking up the callers and hits the parallel
>> task layer (either drop the argument there instead, or have two
>> different work functions with consequent fannout). And I expect this
>> interface is going to be completely changed by JDK-8202845, so it
>> didn't seem worth doing much here.
>
> Oh, I would have simply kept the parameter in the ProcessTask layer.
> You are right, we want to discuss this later though.
>
> [...]
>
> Thanks for all the other clarifications in the documentation.
>
>>
>> New webrev, rebased to include JDK-8201491:
>> http://cr.openjdk.java.net/~kbarrett/8203028/open.01/
>>
>> New webrevs with above changes:
>> full: http://cr.openjdk.java.net/~kbarrett/8203028/open.02/
>> incr: http://cr.openjdk.java.net/~kbarrett/8203028/open.02.inc/
>>
>
> maybe some really stupid question, but: do we really not need to
> update the next field if we discovered an (inactive) Reference? (in
> instanceRefKlass.inline.hpp, in oop_oop_iterate_discovery).
The next field does get updated, just not via any bespoke mechanism.
It's now treated as an ordinary oop-containing field, just like queue.
See the InstanceRefKlass::update_nonstatic_oop_maps change.
oop_oop_iterate_discovery and friends are helpers for
oop_oop_iterate_ref_processing_specialized. That function is a helper
for InstanceRefKlass::oop_oop_iterate(|_reversed|bounded). And those
functions call InstanceKlass::oop_oop_iterate (processing the fields
in the oop map, now including next) as well as the _ref_processing
helper.
> One situation that comes to my mind is that while a (Final-)Reference
> is on the pending list it might still be moved (concurrent
> discovery/Remark puts it on the pending list, some mixed gc might move
> it later). While nobody seems to be touching that "next" reference any
> more for FinalReferences after being marked that way, it seems bad
> style to have a dangling oop (and probably messes up some heap
> verification).
There’s no dangling oop; next is treated like all normal oop fields, e.g.
it is no longer treated specially like referent and discovered.
> Another is situation is that the next field is used as a link in the
> ReferenceQueue (for non-FinalReference), so the next field is always be
> a valid oop (and fixed up), shouldn't it? (Since these References
> referent is NULL, they won't be discovered, but the next field needs to
> be iterated over?)
And it is, in the normal fashion used for other oop fields.
> I do not think the next field can be the only source of a new
> undiscovered object graph (it is either NULL or self-looped or part of
> a ReferenceQueue) though.
I'm not sure what you are getting at here. In process_phase2, the
only invocation of keep_alive now is on the is_alive-returned-true
referent. But maybe the above answer about next field handling helps
here too?
More information about the hotspot-gc-dev
mailing list