RFR (S): 8202021: Improve variable naming in ReferenceProcessor

Stefan Johansson stefan.johansson at oracle.com
Mon Apr 23 14:02:15 UTC 2018


Hi Thomas,

On 2018-04-19 22:08, Thomas Schatzl wrote:
> Hi Sangheon,
> 
>    thanks for your review. I updated the change in-place.
> 
> Thomas
> 
> On Thu, 2018-04-19 at 12:38 -0700, sangheon.kim wrote:
>> Hi Thomas,
>>
>> On 04/19/2018 12:01 PM, Thomas Schatzl wrote:
>>> Hi all,
>>>
>>>     can I have reviews for this cleanup change that improves
>>> variable
>>> naming in ReferenceProcessor code.
>>>
>>> The main change is about renaming some member variables from
>>> something
>>> really confusing to me to something useful (to me again :)) in the
>>> DiscoveredListIterator class.
>>>
>>> The other change is related to changing ReferenceProcessor::num_q
>>> to
>>> num_queues, i.e. spelling out what the "q" is supposed to mean.
>>>
>>> That imho improves readability quite a bit.
>>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8202021
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8202021/webrev/

Thanks for doing this, having better names really help when working with 
this code. The change looks ok, but I still struggle a bit with what all 
members are. I think my problem is seeing that _ref and _discovered_addr 
go together. What do you think about renaming:
_ref -> _current
_discovered_addr -> _current_discovered_addr
Potentially also:
_referent -> _current_referent
_referent_addr -> _current_referent_addr

Cheers,
Stefan

>>> Testing:
>>> local compilation, hs-tier1-4 with 8202017
>>>
>>> based on JDK-8202018, but I do not think there is overlap.
>>>
>>> Thanks,
>>>     Thomas
>>>
>>
>> Looks good.
>> Just minor nits, so don't need extra webrev for these.
>>
>> /src/hotspot/share/gc/shared/referenceProcessor.cpp
>> 112   _discovery_is_mt     = mt_discovery;
>> 113   _num_queues               = MAX2(1U, mt_processing_degree);
>> 114   _max_num_queues           = MAX2(_num_queues,
>> mt_discovery_degree);
>> - Align with line 112?
>>
>> 312     log_develop_trace(gc, ref)("        obj " INTPTR_FORMAT
>> "/next_d
>> " INTPTR_FORMAT, p2i(obj), p2i(next_discovered));
>> - next_d -> next_discovered
>>
>> src/hotspot/share/gc/shared/referenceProcessor.hpp
>> - Copyright year update
>>
>> src/hotspot/share/gc/shared/referenceProcessor.inline.hpp
>> - Copyright year update
>>
>> Thanks,
>> Sangheon
>>
>>
>>
>>
> 


More information about the hotspot-gc-dev mailing list