RFR(S) 8176887: SIGSEGV in AOTCodeHeap::next when using specific configuration

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

RFR(S) 8176887: SIGSEGV in AOTCodeHeap::next when using specific configuration

Igor Veresov
The problem here is that AOTCodeHeap::got_metadata_do() iterates over the _metaspace_got array assuming that the array contains Metadata*, indeed the array is declared as such. However it also happens to contain MethodCounters*, and MethodCounters is not Metadata, but MetaspaceObj. The simplest and minimal risk solution is to make MethodCounters a subclass of Metadata (we’d like to have this fix in jdk9).


Webrev: http://cr.openjdk.java.net/~iveresov/8176887/webrev.00/ 
JBS: https://bugs.openjdk.java.net/browse/JDK-8176887

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

Re: RFR(S) 8176887: SIGSEGV in AOTCodeHeap::next when using specific configuration

Vladimir Kozlov
This could be good fix for JDK 9.

Are you sure that functions called for Metadata scan will not barf on
MethodCounters?

You need also ask Runtime group to look on this change.

When fixing 8173794 I missed this case (MethodCounters*). We should not
put them into metaspace_got array since GC don't need to process them.
And they increase metaspace_got array significantly.

For 10 we should fix JAPTC code - metaspace_got array should contain
only Klass* pointers. May be to add a separate array since
extLinkageGOTContainer may not right for MethodCounters*.

Thanks,
Vladimir

On 4/7/17 11:55 AM, Igor Veresov wrote:
> The problem here is that AOTCodeHeap::got_metadata_do() iterates over the _metaspace_got array assuming that the array contains Metadata*, indeed the array is declared as such. However it also happens to contain MethodCounters*, and MethodCounters is not Metadata, but MetaspaceObj. The simplest and minimal risk solution is to make MethodCounters a subclass of Metadata (we’d like to have this fix in jdk9).
>
>
> Webrev: http://cr.openjdk.java.net/~iveresov/8176887/webrev.00/
> JBS: https://bugs.openjdk.java.net/browse/JDK-8176887
>
> Thanks!
> igor
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(S) 8176887: SIGSEGV in AOTCodeHeap::next when using specific configuration

Igor Veresov

> On Apr 7, 2017, at 1:29 PM, Vladimir Kozlov <[hidden email]> wrote:
>
> This could be good fix for JDK 9.
>
> Are you sure that functions called for Metadata scan will not barf on MethodCounters?

Yes, there shouldn’t be any problems. In the context of class redefinition it calls mark_on_stack(), which is empty by default except for Method and ConstantPool.

>
> You need also ask Runtime group to look on this change.

CCing the runtime mail list. Guys, any objections to making MethodCounters a Metadata ?

>
> When fixing 8173794 I missed this case (MethodCounters*). We should not put them into metaspace_got array since GC don't need to process them. And they increase metaspace_got array significantly.
>
> For 10 we should fix JAPTC code - metaspace_got array should contain only Klass* pointers. May be to add a separate array since extLinkageGOTContainer may not right for MethodCounters*.


In the context of class redefinition the scan is actually interested only in methods, rather than classes. Going forward, yes, it would probably be better if we had separate GOT arrays for each MetaspaceObj subtype, so that we can separate Methods out and scan only them.

igor

>
> Thanks,
> Vladimir
>
> On 4/7/17 11:55 AM, Igor Veresov wrote:
>> The problem here is that AOTCodeHeap::got_metadata_do() iterates over the _metaspace_got array assuming that the array contains Metadata*, indeed the array is declared as such. However it also happens to contain MethodCounters*, and MethodCounters is not Metadata, but MetaspaceObj. The simplest and minimal risk solution is to make MethodCounters a subclass of Metadata (we’d like to have this fix in jdk9).
>>
>>
>> Webrev: http://cr.openjdk.java.net/~iveresov/8176887/webrev.00/
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8176887
>>
>> Thanks!
>> igor
>>

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

Re: RFR(S) 8176887: SIGSEGV in AOTCodeHeap::next when using specific configuration

Igor Veresov
So… I’m ok to assume that you’re ok with this fix for the time being?

igor

On Apr 7, 2017, at 3:43 PM, Igor Veresov <[hidden email]> wrote:


On Apr 7, 2017, at 2:36 PM, [hidden email] wrote:



On 4/7/17 5:33 PM, Igor Veresov wrote:
On Apr 7, 2017, at 1:29 PM, Vladimir Kozlov <[hidden email]> wrote:

This could be good fix for JDK 9.

Are you sure that functions called for Metadata scan will not barf on MethodCounters?
Yes, there shouldn’t be any problems. In the context of class redefinition it calls mark_on_stack(), which is empty by default except for Method and ConstantPool.

You need also ask Runtime group to look on this change.
CCing the runtime mail list. Guys, any objections to making MethodCounters a Metadata ?

If you make MethodCounters inherited by Metadata it will have a vptr.  It doesn't otherwise.

Yes, but I’d have to have some type id to filter them out anyway (if they are in an array with other metaspace objects), having a vtbl seems like an equivalent solution.


When fixing 8173794 I missed this case (MethodCounters*). We should not put them into metaspace_got array since GC don't need to process them. And they increase metaspace_got array significantly.

For 10 we should fix JAPTC code - metaspace_got array should contain only Klass* pointers. May be to add a separate array since extLinkageGOTContainer may not right for MethodCounters*.

In the context of class redefinition the scan is actually interested only in methods, rather than classes. Going forward, yes, it would probably be better if we had separate GOT arrays for each MetaspaceObj subtype, so that we can separate Methods out and scan only them.

Can you have a special array for Method*s?

Yes, that’s what would be best to do in the future. But it is a substantially bigger change, I’m not sure it would be appropriate for 9 at this point. 

igor

Coleen

igor

Thanks,
Vladimir

On 4/7/17 11:55 AM, Igor Veresov wrote:
The problem here is that AOTCodeHeap::got_metadata_do() iterates over the _metaspace_got array assuming that the array contains Metadata*, indeed the array is declared as such. However it also happens to contain MethodCounters*, and MethodCounters is not Metadata, but MetaspaceObj. The simplest and minimal risk solution is to make MethodCounters a subclass of Metadata (we’d like to have this fix in jdk9).


Webrev: http://cr.openjdk.java.net/~iveresov/8176887/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8176887

Thanks!
igor

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

Re: RFR(S) 8176887: SIGSEGV in AOTCodeHeap::next when using specific configuration

coleen.phillimore


On 4/10/17 3:48 PM, Igor Veresov wrote:
So… I’m ok to assume that you’re ok with this fix for the time being?

If these MethodCounters aren't added to the CDS archive, then the vptr only adds space and I think it's ok for now.
Coleen


igor

On Apr 7, 2017, at 3:43 PM, Igor Veresov <[hidden email]> wrote:


On Apr 7, 2017, at 2:36 PM, [hidden email] wrote:



On 4/7/17 5:33 PM, Igor Veresov wrote:
On Apr 7, 2017, at 1:29 PM, Vladimir Kozlov <[hidden email]> wrote:

This could be good fix for JDK 9.

Are you sure that functions called for Metadata scan will not barf on MethodCounters?
Yes, there shouldn’t be any problems. In the context of class redefinition it calls mark_on_stack(), which is empty by default except for Method and ConstantPool.

You need also ask Runtime group to look on this change.
CCing the runtime mail list. Guys, any objections to making MethodCounters a Metadata ?

If you make MethodCounters inherited by Metadata it will have a vptr.  It doesn't otherwise.

Yes, but I’d have to have some type id to filter them out anyway (if they are in an array with other metaspace objects), having a vtbl seems like an equivalent solution.


When fixing 8173794 I missed this case (MethodCounters*). We should not put them into metaspace_got array since GC don't need to process them. And they increase metaspace_got array significantly.

For 10 we should fix JAPTC code - metaspace_got array should contain only Klass* pointers. May be to add a separate array since extLinkageGOTContainer may not right for MethodCounters*.

In the context of class redefinition the scan is actually interested only in methods, rather than classes. Going forward, yes, it would probably be better if we had separate GOT arrays for each MetaspaceObj subtype, so that we can separate Methods out and scan only them.

Can you have a special array for Method*s?

Yes, that’s what would be best to do in the future. But it is a substantially bigger change, I’m not sure it would be appropriate for 9 at this point. 

igor

Coleen

igor

Thanks,
Vladimir

On 4/7/17 11:55 AM, Igor Veresov wrote:
The problem here is that AOTCodeHeap::got_metadata_do() iterates over the _metaspace_got array assuming that the array contains Metadata*, indeed the array is declared as such. However it also happens to contain MethodCounters*, and MethodCounters is not Metadata, but MetaspaceObj. The simplest and minimal risk solution is to make MethodCounters a subclass of Metadata (we’d like to have this fix in jdk9).


Webrev: http://cr.openjdk.java.net/~iveresov/8176887/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8176887

Thanks!
igor


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

Re: RFR(S) 8176887: SIGSEGV in AOTCodeHeap::next when using specific configuration

Igor Veresov
Thanks, Coleen!

igor

On Apr 10, 2017, at 1:22 PM, [hidden email] wrote:



On 4/10/17 3:48 PM, Igor Veresov wrote:
So… I’m ok to assume that you’re ok with this fix for the time being?

If these MethodCounters aren't added to the CDS archive, then the vptr only adds space and I think it's ok for now.
Coleen


igor

On Apr 7, 2017, at 3:43 PM, Igor Veresov <[hidden email]> wrote:


On Apr 7, 2017, at 2:36 PM, [hidden email] wrote:



On 4/7/17 5:33 PM, Igor Veresov wrote:
On Apr 7, 2017, at 1:29 PM, Vladimir Kozlov <[hidden email]> wrote:

This could be good fix for JDK 9.

Are you sure that functions called for Metadata scan will not barf on MethodCounters?
Yes, there shouldn’t be any problems. In the context of class redefinition it calls mark_on_stack(), which is empty by default except for Method and ConstantPool.

You need also ask Runtime group to look on this change.
CCing the runtime mail list. Guys, any objections to making MethodCounters a Metadata ?

If you make MethodCounters inherited by Metadata it will have a vptr.  It doesn't otherwise.

Yes, but I’d have to have some type id to filter them out anyway (if they are in an array with other metaspace objects), having a vtbl seems like an equivalent solution.


When fixing 8173794 I missed this case (MethodCounters*). We should not put them into metaspace_got array since GC don't need to process them. And they increase metaspace_got array significantly.

For 10 we should fix JAPTC code - metaspace_got array should contain only Klass* pointers. May be to add a separate array since extLinkageGOTContainer may not right for MethodCounters*.

In the context of class redefinition the scan is actually interested only in methods, rather than classes. Going forward, yes, it would probably be better if we had separate GOT arrays for each MetaspaceObj subtype, so that we can separate Methods out and scan only them.

Can you have a special array for Method*s?

Yes, that’s what would be best to do in the future. But it is a substantially bigger change, I’m not sure it would be appropriate for 9 at this point. 

igor

Coleen

igor

Thanks,
Vladimir

On 4/7/17 11:55 AM, Igor Veresov wrote:
The problem here is that AOTCodeHeap::got_metadata_do() iterates over the _metaspace_got array assuming that the array contains Metadata*, indeed the array is declared as such. However it also happens to contain MethodCounters*, and MethodCounters is not Metadata, but MetaspaceObj. The simplest and minimal risk solution is to make MethodCounters a subclass of Metadata (we’d like to have this fix in jdk9).


Webrev: http://cr.openjdk.java.net/~iveresov/8176887/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8176887

Thanks!
igor



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

Re: RFR(S) 8176887: SIGSEGV in AOTCodeHeap::next when using specific configuration

Igor Veresov
Vladimir, so are you ok with us pushing this?

igor

> On Apr 10, 2017, at 2:13 PM, Igor Veresov <[hidden email]> wrote:
>
> Thanks, Coleen!
>
> igor
>
>> On Apr 10, 2017, at 1:22 PM, [hidden email] wrote:
>>
>>
>>
>> On 4/10/17 3:48 PM, Igor Veresov wrote:
>>> So… I’m ok to assume that you’re ok with this fix for the time being?
>>
>> If these MethodCounters aren't added to the CDS archive, then the vptr only adds space and I think it's ok for now.
>> Coleen
>>
>>>
>>> igor
>>>
>>>> On Apr 7, 2017, at 3:43 PM, Igor Veresov <[hidden email] <mailto:[hidden email]>> wrote:
>>>>
>>>>>
>>>>> On Apr 7, 2017, at 2:36 PM, [hidden email] <mailto:[hidden email]> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 4/7/17 5:33 PM, Igor Veresov wrote:
>>>>>>> On Apr 7, 2017, at 1:29 PM, Vladimir Kozlov <[hidden email] <mailto:[hidden email]>> wrote:
>>>>>>>
>>>>>>> This could be good fix for JDK 9.
>>>>>>>
>>>>>>> Are you sure that functions called for Metadata scan will not barf on MethodCounters?
>>>>>> Yes, there shouldn’t be any problems. In the context of class redefinition it calls mark_on_stack(), which is empty by default except for Method and ConstantPool.
>>>>>>
>>>>>>> You need also ask Runtime group to look on this change.
>>>>>> CCing the runtime mail list. Guys, any objections to making MethodCounters a Metadata ?
>>>>>
>>>>> If you make MethodCounters inherited by Metadata it will have a vptr.  It doesn't otherwise.
>>>>
>>>> Yes, but I’d have to have some type id to filter them out anyway (if they are in an array with other metaspace objects), having a vtbl seems like an equivalent solution.
>>>>
>>>>>>
>>>>>>> When fixing 8173794 I missed this case (MethodCounters*). We should not put them into metaspace_got array since GC don't need to process them. And they increase metaspace_got array significantly.
>>>>>>>
>>>>>>> For 10 we should fix JAPTC code - metaspace_got array should contain only Klass* pointers. May be to add a separate array since extLinkageGOTContainer may not right for MethodCounters*.
>>>>>>
>>>>>> In the context of class redefinition the scan is actually interested only in methods, rather than classes. Going forward, yes, it would probably be better if we had separate GOT arrays for each MetaspaceObj subtype, so that we can separate Methods out and scan only them.
>>>>>
>>>>> Can you have a special array for Method*s?
>>>>
>>>> Yes, that’s what would be best to do in the future. But it is a substantially bigger change, I’m not sure it would be appropriate for 9 at this point.
>>>>
>>>> igor
>>>>
>>>>> Coleen
>>>>>
>>>>>> igor
>>>>>>
>>>>>>> Thanks,
>>>>>>> Vladimir
>>>>>>>
>>>>>>> On 4/7/17 11:55 AM, Igor Veresov wrote:
>>>>>>>> The problem here is that AOTCodeHeap::got_metadata_do() iterates over the _metaspace_got array assuming that the array contains Metadata*, indeed the array is declared as such. However it also happens to contain MethodCounters*, and MethodCounters is not Metadata, but MetaspaceObj. The simplest and minimal risk solution is to make MethodCounters a subclass of Metadata (we’d like to have this fix in jdk9).
>>>>>>>>
>>>>>>>>
>>>>>>>> Webrev: http://cr.openjdk.java.net/~iveresov/8176887/webrev.00/ <http://cr.openjdk.java.net/%7Eiveresov/8176887/webrev.00/>
>>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8176887 <https://bugs.openjdk.java.net/browse/JDK-8176887>
>>>>>>>>
>>>>>>>> Thanks!
>>>>>>>> igor
>>>
>>
>

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

Re: RFR(S) 8176887: SIGSEGV in AOTCodeHeap::next when using specific configuration

Vladimir Kozlov
Yes, I am fine with it. We will reverse this vtbl in MethodCounters when we do proper fix in JDK 10.

I would like to do splitting and renaming of these arrays in JDK 10. And it is a lot of changes which we don't want to
see now in JDK 9.

Vladimir

On 4/10/17 5:59 PM, Igor Veresov wrote:

> Vladimir, so are you ok with us pushing this?
>
> igor
>
>> On Apr 10, 2017, at 2:13 PM, Igor Veresov <[hidden email]> wrote:
>>
>> Thanks, Coleen!
>>
>> igor
>>
>>> On Apr 10, 2017, at 1:22 PM, [hidden email] wrote:
>>>
>>>
>>>
>>> On 4/10/17 3:48 PM, Igor Veresov wrote:
>>>> So… I’m ok to assume that you’re ok with this fix for the time being?
>>>
>>> If these MethodCounters aren't added to the CDS archive, then the vptr only adds space and I think it's ok for now.
>>> Coleen
>>>
>>>>
>>>> igor
>>>>
>>>>> On Apr 7, 2017, at 3:43 PM, Igor Veresov <[hidden email] <mailto:[hidden email]>> wrote:
>>>>>
>>>>>>
>>>>>> On Apr 7, 2017, at 2:36 PM, [hidden email] <mailto:[hidden email]> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 4/7/17 5:33 PM, Igor Veresov wrote:
>>>>>>>> On Apr 7, 2017, at 1:29 PM, Vladimir Kozlov <[hidden email] <mailto:[hidden email]>> wrote:
>>>>>>>>
>>>>>>>> This could be good fix for JDK 9.
>>>>>>>>
>>>>>>>> Are you sure that functions called for Metadata scan will not barf on MethodCounters?
>>>>>>> Yes, there shouldn’t be any problems. In the context of class redefinition it calls mark_on_stack(), which is empty by default except for Method and ConstantPool.
>>>>>>>
>>>>>>>> You need also ask Runtime group to look on this change.
>>>>>>> CCing the runtime mail list. Guys, any objections to making MethodCounters a Metadata ?
>>>>>>
>>>>>> If you make MethodCounters inherited by Metadata it will have a vptr.  It doesn't otherwise.
>>>>>
>>>>> Yes, but I’d have to have some type id to filter them out anyway (if they are in an array with other metaspace objects), having a vtbl seems like an equivalent solution.
>>>>>
>>>>>>>
>>>>>>>> When fixing 8173794 I missed this case (MethodCounters*). We should not put them into metaspace_got array since GC don't need to process them. And they increase metaspace_got array significantly.
>>>>>>>>
>>>>>>>> For 10 we should fix JAPTC code - metaspace_got array should contain only Klass* pointers. May be to add a separate array since extLinkageGOTContainer may not right for MethodCounters*.
>>>>>>>
>>>>>>> In the context of class redefinition the scan is actually interested only in methods, rather than classes. Going forward, yes, it would probably be better if we had separate GOT arrays for each MetaspaceObj subtype, so that we can separate Methods out and scan only them.
>>>>>>
>>>>>> Can you have a special array for Method*s?
>>>>>
>>>>> Yes, that’s what would be best to do in the future. But it is a substantially bigger change, I’m not sure it would be appropriate for 9 at this point.
>>>>>
>>>>> igor
>>>>>
>>>>>> Coleen
>>>>>>
>>>>>>> igor
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Vladimir
>>>>>>>>
>>>>>>>> On 4/7/17 11:55 AM, Igor Veresov wrote:
>>>>>>>>> The problem here is that AOTCodeHeap::got_metadata_do() iterates over the _metaspace_got array assuming that the array contains Metadata*, indeed the array is declared as such. However it also happens to contain MethodCounters*, and MethodCounters is not Metadata, but MetaspaceObj. The simplest and minimal risk solution is to make MethodCounters a subclass of Metadata (we’d like to have this fix in jdk9).
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Webrev: http://cr.openjdk.java.net/~iveresov/8176887/webrev.00/ <http://cr.openjdk.java.net/%7Eiveresov/8176887/webrev.00/>
>>>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8176887 <https://bugs.openjdk.java.net/browse/JDK-8176887>
>>>>>>>>>
>>>>>>>>> Thanks!
>>>>>>>>> igor
>>>>
>>>
>>
>
Loading...