Quantcast

RFR(s): 8178363: Incorrect check for nmethod re-registration in C1

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

RFR(s): 8178363: Incorrect check for nmethod re-registration in C1

Per Liden
Hi,

When an nmethod is compiled, it's registered with the heap if
ScavengeRootsInCode is non-zero (which it always is). However, during
code patching in C1 we re-register the nmethod with the heap only if it
also has scavengable oops. This additional requirement is only valid for
calls to CodeCache::add_scavenge_root_nmethod(), and not for
CollectedHeap::register_nmethod(). This happens to work today since G1's
is_scavengable() basically always returns true.

Bug: https://bugs.openjdk.java.net/browse/JDK-8178363
Webrev: http://cr.openjdk.java.net/~pliden/8178363/webrev.0/
Testing: jprt

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

Re: RFR(s): 8178363: Incorrect check for nmethod re-registration in C1

Stefan Karlsson
On 2017-04-10 09:05, Per Liden wrote:

> Hi,
>
> When an nmethod is compiled, it's registered with the heap if
> ScavengeRootsInCode is non-zero (which it always is). However, during
> code patching in C1 we re-register the nmethod with the heap only if it
> also has scavengable oops. This additional requirement is only valid for
> calls to CodeCache::add_scavenge_root_nmethod(), and not for
> CollectedHeap::register_nmethod(). This happens to work today since G1's
> is_scavengable() basically always returns true.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8178363
> Webrev: http://cr.openjdk.java.net/~pliden/8178363/webrev.0/
> Testing: jprt

Looks good, from a GC perspective.

For non-G1 GCs, we now, unnecessarily, take the CodCache lock and call
find_nmethod even when mirror and appendix are non-scavengable. Maybe
this should be reviewed on hotspot-comp-dev as well?

Thanks,
StefanK

>
> cheers,
> Per
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(s): 8178363: Incorrect check for nmethod re-registration in C1

Per Liden
Hi Stefan,

On 2017-04-10 09:31, Stefan Karlsson wrote:

> On 2017-04-10 09:05, Per Liden wrote:
>> Hi,
>>
>> When an nmethod is compiled, it's registered with the heap if
>> ScavengeRootsInCode is non-zero (which it always is). However, during
>> code patching in C1 we re-register the nmethod with the heap only if it
>> also has scavengable oops. This additional requirement is only valid for
>> calls to CodeCache::add_scavenge_root_nmethod(), and not for
>> CollectedHeap::register_nmethod(). This happens to work today since G1's
>> is_scavengable() basically always returns true.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8178363
>> Webrev: http://cr.openjdk.java.net/~pliden/8178363/webrev.0/
>> Testing: jprt
>
> Looks good, from a GC perspective.

Thanks for reviewing!

>
> For non-G1 GCs, we now, unnecessarily, take the CodCache lock and call
> find_nmethod even when mirror and appendix are non-scavengable. Maybe
> this should be reviewed on hotspot-comp-dev as well?

Sure, adding comp-dev.

thanks,
Per

>
> Thanks,
> StefanK
>
>>
>> cheers,
>> Per
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(s): 8178363: Incorrect check for nmethod re-registration in C1

Rickard Bäckman
Looks good.

On 04/10, Per Liden wrote:

> Hi Stefan,
>
> On 2017-04-10 09:31, Stefan Karlsson wrote:
> >On 2017-04-10 09:05, Per Liden wrote:
> >>Hi,
> >>
> >>When an nmethod is compiled, it's registered with the heap if
> >>ScavengeRootsInCode is non-zero (which it always is). However, during
> >>code patching in C1 we re-register the nmethod with the heap only if it
> >>also has scavengable oops. This additional requirement is only valid for
> >>calls to CodeCache::add_scavenge_root_nmethod(), and not for
> >>CollectedHeap::register_nmethod(). This happens to work today since G1's
> >>is_scavengable() basically always returns true.
> >>
> >>Bug: https://bugs.openjdk.java.net/browse/JDK-8178363
> >>Webrev: http://cr.openjdk.java.net/~pliden/8178363/webrev.0/
> >>Testing: jprt
> >
> >Looks good, from a GC perspective.
>
> Thanks for reviewing!
>
> >
> >For non-G1 GCs, we now, unnecessarily, take the CodCache lock and call
> >find_nmethod even when mirror and appendix are non-scavengable. Maybe
> >this should be reviewed on hotspot-comp-dev as well?
>
> Sure, adding comp-dev.
>
> thanks,
> Per
>
> >
> >Thanks,
> >StefanK
> >
> >>
> >>cheers,
> >>Per

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

Re: RFR(s): 8178363: Incorrect check for nmethod re-registration in C1

Per Liden
On 2017-04-12 08:40, Rickard Bäckman wrote:
> Looks good.

Thanks Rickard!

cheers,
Per

>
> On 04/10, Per Liden wrote:
>> Hi Stefan,
>>
>> On 2017-04-10 09:31, Stefan Karlsson wrote:
>>> On 2017-04-10 09:05, Per Liden wrote:
>>>> Hi,
>>>>
>>>> When an nmethod is compiled, it's registered with the heap if
>>>> ScavengeRootsInCode is non-zero (which it always is). However, during
>>>> code patching in C1 we re-register the nmethod with the heap only if it
>>>> also has scavengable oops. This additional requirement is only valid for
>>>> calls to CodeCache::add_scavenge_root_nmethod(), and not for
>>>> CollectedHeap::register_nmethod(). This happens to work today since G1's
>>>> is_scavengable() basically always returns true.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8178363
>>>> Webrev: http://cr.openjdk.java.net/~pliden/8178363/webrev.0/
>>>> Testing: jprt
>>>
>>> Looks good, from a GC perspective.
>>
>> Thanks for reviewing!
>>
>>>
>>> For non-G1 GCs, we now, unnecessarily, take the CodCache lock and call
>>> find_nmethod even when mirror and appendix are non-scavengable. Maybe
>>> this should be reviewed on hotspot-comp-dev as well?
>>
>> Sure, adding comp-dev.
>>
>> thanks,
>> Per
>>
>>>
>>> Thanks,
>>> StefanK
>>>
>>>>
>>>> cheers,
>>>> Per
>
> /R
>
Loading...