RFR (M): 8071278: Fix the closure mess in G1RemSet::refine_card()

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

RFR (M): 8071278: Fix the closure mess in G1RemSet::refine_card()

Thomas Schatzl
Hi all,

  one line:

"188 lines changed: 23 ins; 143 del; 22 mod; 3097 unchg"

This is a rip-out and manual collapsing of needlessly complex code in
the update rs algorithm.

There are more improvements obvious now in that area (JDK-8071280, JDK-
8162928, JDK-8175554 at least), so I kept this one to the minimum.

The change makes the code a bit faster too due to avoiding repeated
null-checks, region-crossing checks etc. etc.

CR:
https://bugs.openjdk.java.net/browse/JDK-8071278
Webrev:
http://cr.openjdk.java.net/~tschatzl/8071278/webrev/
Testing:
jprt, tier2+3 testing, some benchmarks

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

Re: RFR (M): 8071278: Fix the closure mess in G1RemSet::refine_card()

Kim Barrett
> On Feb 23, 2017, at 6:23 AM, Thomas Schatzl <[hidden email]> wrote:
>
> Hi all,
>
>   one line:
>
> "188 lines changed: 23 ins; 143 del; 22 mod; 3097 unchg"
>
> This is a rip-out and manual collapsing of needlessly complex code in
> the update rs algorithm.
>
> There are more improvements obvious now in that area (JDK-8071280, JDK-
> 8162928, JDK-8175554 at least), so I kept this one to the minimum.
>
> The change makes the code a bit faster too due to avoiding repeated
> null-checks, region-crossing checks etc. etc.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8071278
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8071278/webrev/
> Testing:
> jprt, tier2+3 testing, some benchmarks
>
> Thanks,
>   Thomas

Looks good.

Reply | Threaded
Open this post in threaded view
|

Re: RFR (M): 8071278: Fix the closure mess in G1RemSet::refine_card()

Thomas Schatzl
Hi Kim,

On Fri, 2017-02-24 at 01:33 -0500, Kim Barrett wrote:

> >
> > On Feb 23, 2017, at 6:23 AM, Thomas Schatzl <thomas.schatzl@oracle.
> > com> wrote:
> > [...]
> >
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8071278
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8071278/webrev/
> > Testing:
> > jprt, tier2+3 testing, some benchmarks
> >
> > Thanks,
> >   Thomas
> Looks good.
>

  thanks for your review.

Thomas
Reply | Threaded
Open this post in threaded view
|

Re: RFR (M): 8071278: Fix the closure mess in G1RemSet::refine_card()

Stefan Johansson
In reply to this post by Thomas Schatzl
Thanks for cleaning this up Thomas,

On 2017-02-23 12:23, Thomas Schatzl wrote:

> Hi all,
>
>    one line:
>
> "188 lines changed: 23 ins; 143 del; 22 mod; 3097 unchg"
>
> This is a rip-out and manual collapsing of needlessly complex code in
> the update rs algorithm.
>
> There are more improvements obvious now in that area (JDK-8071280, JDK-
> 8162928, JDK-8175554 at least), so I kept this one to the minimum.
>
> The change makes the code a bit faster too due to avoiding repeated
> null-checks, region-crossing checks etc. etc.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8071278
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8071278/webrev/

Change looks good in general, just a few small things.
---
src/share/vm/gc/g1/g1_specialized_oop_closures.hpp:
  45       f(G1UpdateRSOrPushRefOopClosure,_nv)              \

Alignment of '\'.
----
src/share/vm/gc/g1/g1OopClosures.hpp:
192   bool apply_to_weak_ref_discovered_field() { return true; }

Did you do any analysis whether or not this is needed? Or did you just
re-add it since FilterOutOfRegionClosure had it? I never figured out a
clear answer to that when I was looking at this a while back. I don't
see any need to change this right now, just curious if you know why it
is needed.
---

Thanks,
Stefan

> Testing:
> jprt, tier2+3 testing, some benchmarks
>
> Thanks,
>    Thomas
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (M): 8071278: Fix the closure mess in G1RemSet::refine_card()

Thomas Schatzl
Hi Stefan,

On Fri, 2017-02-24 at 14:07 +0100, Stefan Johansson wrote:

> Thanks for cleaning this up Thomas,
>
> On 2017-02-23 12:23, Thomas Schatzl wrote:
> >
> > Hi all,
> >
> >    one line:
> >
> > "188 lines changed: 23 ins; 143 del; 22 mod; 3097 unchg"
> >
> > This is a rip-out and manual collapsing of needlessly complex code
> > in the update rs algorithm.
> >
> > There are more improvements obvious now in that area (JDK-8071280,
> > JDK-8162928, JDK-8175554 at least), so I kept this one to the
> > minimum.
> >
> > The change makes the code a bit faster too due to avoiding repeated
> > null-checks, region-crossing checks etc. etc.
> >
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8071278
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8071278/webrev/
> Change looks good in general, just a few small things.
> ---
> src/share/vm/gc/g1/g1_specialized_oop_closures.hpp:
>   45       f(G1UpdateRSOrPushRefOopClosure,_nv)              \
>
> Alignment of '\'.

Fixed.

> ----
> src/share/vm/gc/g1/g1OopClosures.hpp:
> 192   bool apply_to_weak_ref_discovered_field() { return true; }
>
> Did you do any analysis whether or not this is needed? Or did you
> just re-add it since FilterOutOfRegionClosure had it? I never figured
> out a clear answer to that when I was looking at this a while back. I
> don't see any need to change this right now, just curious if you know
> why it is needed.

I did only do cursory analysis about that, but it seems that some code
(G1CollectedHeap::preserve_cm_referents(), pending list management)
relies on it, so it needs to be preserved, and so simply added it when
removing the FilterOutOfRegionClosure you described.

I will think about it again in more detail soonish.

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

Re: RFR (M): 8071278: Fix the closure mess in G1RemSet::refine_card()

Stefan Johansson


On 2017-02-24 15:36, Thomas Schatzl wrote:

> Hi Stefan,
>
> On Fri, 2017-02-24 at 14:07 +0100, Stefan Johansson wrote:
>> Thanks for cleaning this up Thomas,
>>
>> On 2017-02-23 12:23, Thomas Schatzl wrote:
>>> Hi all,
>>>
>>>     one line:
>>>
>>> "188 lines changed: 23 ins; 143 del; 22 mod; 3097 unchg"
>>>
>>> This is a rip-out and manual collapsing of needlessly complex code
>>> in the update rs algorithm.
>>>
>>> There are more improvements obvious now in that area (JDK-8071280,
>>> JDK-8162928, JDK-8175554 at least), so I kept this one to the
>>> minimum.
>>>
>>> The change makes the code a bit faster too due to avoiding repeated
>>> null-checks, region-crossing checks etc. etc.
>>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8071278
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8071278/webrev/
>> Change looks good in general, just a few small things.
>> ---
>> src/share/vm/gc/g1/g1_specialized_oop_closures.hpp:
>>    45       f(G1UpdateRSOrPushRefOopClosure,_nv)              \
>>
>> Alignment of '\'.
> Fixed.
>
>> ----
>> src/share/vm/gc/g1/g1OopClosures.hpp:
>> 192   bool apply_to_weak_ref_discovered_field() { return true; }
>>
>> Did you do any analysis whether or not this is needed? Or did you
>> just re-add it since FilterOutOfRegionClosure had it? I never figured
>> out a clear answer to that when I was looking at this a while back. I
>> don't see any need to change this right now, just curious if you know
>> why it is needed.
> I did only do cursory analysis about that, but it seems that some code
> (G1CollectedHeap::preserve_cm_referents(), pending list management)
> relies on it, so it needs to be preserved, and so simply added it when
> removing the FilterOutOfRegionClosure you described.
>
> I will think about it again in more detail soonish.
Sounds fair.

Shit it,
StefanJ
> Thanks,
>    Thomas
>