RFR (S): 8184667: Clean up G1ConcurrentMark files

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR (S): 8184667: Clean up G1ConcurrentMark files

Thomas Schatzl
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/
Testing:
jprt

Thanks,
  Thomas
Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8184667: Clean up G1ConcurrentMark files

Stefan Johansson
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.
---

Thanks,
Stefan

> Testing:
> jprt
>
> Thanks,
>    Thomas

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8184667: Clean up G1ConcurrentMark files

Thomas Schatzl
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

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8184667: Clean up G1ConcurrentMark files

Stefan Johansson


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)
Looks great,
StefanJ
> Thanks,
>    Thomas
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8184667: Clean up G1ConcurrentMark files

Per Liden
In reply to this post by Thomas Schatzl
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
>