RFR (S): 8184667: Clean up G1ConcurrentMark files

[Per Liden]

Hi Thomas,

Looks good. Nice cleanup! Just one minor nit:

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp:

  860           task->do_marking_step(mark_step_duration_ms,
  861                                     true  /* do_termination */,
  862                                     false /* is_serial*/);

Argument indentation.


I don't need to see a new webrev.

/Per

On 2017-10-20 14:12, Thomas Schatzl wrote:
> Hi Stefan,
>
>   thanks for your review.
>
> On Fri, 2017-10-20 at 14:04 +0200, Stefan Johansson wrote:
>> Thanks for cleaning up this code Thomas,
>>
>> On 2017-10-19 15:44, Thomas Schatzl wrote:
>>> Hi all,
>>>
>>>    can I have reviews for this code cleanup of G1ConcurrentMark*
>>> files?
>>>
>>> The idea is to fix formatting, naming, assert strings, remove
>>> unused
>>> variables and methods, some missing braces around statements, and
>>> look
>>> into whether member visibility could be improved.
>>>
>>> No actual behavior changes.
>>>
>>> The CR contains references to a few other cleanup RFEs that I
>>> thought
>>> would not fit perfectly.
>>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8184667
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8184667/webrev/
>>
>> Looks good in general. Just a few small things:
>>
>> src/hotspot/share/gc/g1/g1ConcurrentMark.cpp:
>> 1028   G1CMConcurrentMarkingTask markingTask(this, cm_thread());
>>
>> Rename markingTask to either just task or marking_task.
>> ---
>> src/hotspot/share/gc/g1/g1ConcurrentMark.hpp:
>>   638 #ifdef ASSERT
>>   639   // The task queue set needed for stealing
>>   640   G1CMTaskQueueSet*           _task_queues;
>>   641   // Indicates whether the task has been claimed
>>
>>   642   bool                        _claimed;
>>   643 #endif // ASSERT
>>
>> I wouldn't mind if you just removed those two. Not sure keeping them
>> around is worth it.
>> ---
>>
>
>   I agree. All fixed in
> http://cr.openjdk.java.net/~tschatzl/8184667/webrev.1 (full)
> http://cr.openjdk.java.net/~tschatzl/8184667/webrev.0_to_1 (diff)
>
> Thanks,
>   Thomas
>