Quantcast

RFR: 8171238: Unify cleanup code used in G1 Remark and Full GC marking

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

RFR: 8171238: Unify cleanup code used in G1 Remark and Full GC marking

Stefan Johansson
Hi,

Please review this re-factoring for:
https://bugs.openjdk.java.net/browse/JDK-8171238

Webrev:
http://cr.openjdk.java.net/~sjohanss/8171238/hotspot.00/

Summary:
Some small parts of the code in the G1 Mark Sweep Full GC is parallel,
one of those is the string and symbol table cleaning as well as the
string deduplication cleanup. They are currently run as two separate
tasks which means the worker threads have to be started and stopped two
times. The two tasks can be joined together and with the cleanup in
place the Remark phase of the concurrent cycle can also be unified to
use the same code paths.

Testing:
* JPRT
* RBT tier2 + tier3

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

Re: RFR: 8171238: Unify cleanup code used in G1 Remark and Full GC marking

Thomas Schatzl
Hi,

On Tue, 2017-02-21 at 11:17 +0100, Stefan Johansson wrote:

> Hi,
>
> Please review this re-factoring for:
> https://bugs.openjdk.java.net/browse/JDK-8171238
>
> Webrev:
> http://cr.openjdk.java.net/~sjohanss/8171238/hotspot.00/
>
> Summary:
> Some small parts of the code in the G1 Mark Sweep Full GC is
> parallel, 
> one of those is the string and symbol table cleaning as well as the 
> string deduplication cleanup. They are currently run as two separate 
> tasks which means the worker threads have to be started and stopped
> two 
> times. The two tasks can be joined together and with the cleanup in 
> place the Remark phase of the concurrent cycle can also be unified
> to 
> use the same code paths.

The following comments are mostly about naming, since these are very
much opinions, feel free to ignore what you do not like. The change
itself looks good.

- not really happy with "G1StringSymbolTableUnlinkTask" not showing
that it also does dedup now.

- I would kind of think "complete" as the matching word for "partial",
not "full".

- I would prefer if the comments for full/partial cleaning would
explain what the method does, not (only) when/what it is used for?

- maybe improve the "cleaning" to indicate what is cleaned (or what
cleaning means), i.e. something like
"full/partial_clean_dead_weak_references" or so - that is however a bit
long for me too. Probably you can find a better name.
Some better comments might remove the need for this.

Thanks,
  Thomas

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

Re: RFR: 8171238: Unify cleanup code used in G1 Remark and Full GC marking

Stefan Johansson
Thanks for looking at this Thomas,

On 2017-02-21 12:11, Thomas Schatzl wrote:

> Hi,
>
> On Tue, 2017-02-21 at 11:17 +0100, Stefan Johansson wrote:
>> Hi,
>>
>> Please review this re-factoring for:
>> https://bugs.openjdk.java.net/browse/JDK-8171238
>>
>> Webrev:
>> http://cr.openjdk.java.net/~sjohanss/8171238/hotspot.00/
>>
>> Summary:
>> Some small parts of the code in the G1 Mark Sweep Full GC is
>> parallel,
>> one of those is the string and symbol table cleaning as well as the
>> string deduplication cleanup. They are currently run as two separate
>> tasks which means the worker threads have to be started and stopped
>> two
>> times. The two tasks can be joined together and with the cleanup in
>> place the Remark phase of the concurrent cycle can also be unified
>> to
>> use the same code paths.
> The following comments are mostly about naming, since these are very
> much opinions, feel free to ignore what you do not like. The change
> itself looks good.
>
> - not really happy with "G1StringSymbolTableUnlinkTask" not showing
> that it also does dedup now.
I agree, as we talked about off-line G1StringAndSymbolCleaningTask is
not optimal either but better at least.
> - I would kind of think "complete" as the matching word for "partial",
> not "full".
Agree, changed.
>
> - I would prefer if the comments for full/partial cleaning would
> explain what the method does, not (only) when/what it is used for?
>
> - maybe improve the "cleaning" to indicate what is cleaned (or what
> cleaning means), i.e. something like
> "full/partial_clean_dead_weak_references" or so - that is however a bit
> long for me too. Probably you can find a better name.
> Some better comments might remove the need for this.
Added comments, but left partial_cleaning/complete_cleaning as the names.

New webrev:
Full: http://cr.openjdk.java.net/~sjohanss/8171238/hotspot.01/
Inc: http://cr.openjdk.java.net/~sjohanss/8171238/hotspot.00-01/

Thanks,
Stefan

> Thanks,
>    Thomas
>

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

Re: RFR: 8171238: Unify cleanup code used in G1 Remark and Full GC marking

Thomas Schatzl
Hi,

On Thu, 2017-02-23 at 11:40 +0100, Stefan Johansson wrote:
> >
[....]
> New webrev:
> Full: http://cr.openjdk.java.net/~sjohanss/8171238/hotspot.01/
> Inc: http://cr.openjdk.java.net/~sjohanss/8171238/hotspot.00-01/
>
> Thanks,
> Stefan

  looks good. Please fixup copyright dates before pushing ;)

Thomas

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

Re: RFR: 8171238: Unify cleanup code used in G1 Remark and Full GC marking

Stefan Johansson


On 2017-02-23 13:28, Thomas Schatzl wrote:

> Hi,
>
> On Thu, 2017-02-23 at 11:40 +0100, Stefan Johansson wrote:
> [....]
>> New webrev:
>> Full: http://cr.openjdk.java.net/~sjohanss/8171238/hotspot.01/
>> Inc: http://cr.openjdk.java.net/~sjohanss/8171238/hotspot.00-01/
>>
>> Thanks,
>> Stefan
>    looks good. Please fixup copyright dates before pushing ;)
Will do!
Thanks,
Stefan
> Thomas
>

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

Re: RFR: 8171238: Unify cleanup code used in G1 Remark and Full GC marking

Per Liden
Hi Stefan,

On 2017-02-23 14:47, Stefan Johansson wrote:

>
>
> On 2017-02-23 13:28, Thomas Schatzl wrote:
>> Hi,
>>
>> On Thu, 2017-02-23 at 11:40 +0100, Stefan Johansson wrote:
>> [....]
>>> New webrev:
>>> Full: http://cr.openjdk.java.net/~sjohanss/8171238/hotspot.01/
>>> Inc: http://cr.openjdk.java.net/~sjohanss/8171238/hotspot.00-01/

Patch looks good. Just one thing, please remove the now unused
G1StringDedup::unlink() function. No need for a new webrev on my part.

In some future cleanup patch it would also seem natural if we moved the
unlink_or_oops_do() function to a style similar to parallel_unlink() and
always have the caller decide what Task to call this form. But let's not
do that in this patch.

cheers,
Per

>>>
>>> Thanks,
>>> Stefan
>>    looks good. Please fixup copyright dates before pushing ;)
> Will do!
> Thanks,
> Stefan
>> Thomas
>>
>
Loading...