Re: RFR: SA: JDK-8189798: SA cleanup - part 1

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

Re: RFR: SA: JDK-8189798: SA cleanup - part 1

coleen.phillimore

http://cr.openjdk.java.net/~jgeorge/8189798/webrev.00/src/hotspot/share/runtime/stackValue.hpp.udiff.html

+ return (*(int *)&_integer_value == *(int *)&value->_integer_value);


I don't think the *(int*) casts for _integer_value are needed in these
files.  Can you remove them?

http://cr.openjdk.java.net/~jgeorge/8189798/webrev.00/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/cms/CompactibleFreeListSpace.java.udiff.html

I'm not really sure why you still have SA code for CMS, since CMS is
deprecated.  What does it do?  Can it be removed in a following cleanup?

This cleanup looks good.  And thank you for doing this since I broke SA
only yesterday.

thanks,
Coleen


On 11/2/17 12:54 AM, Jini George wrote:

> Could I please get one more review done for this ?
>
> Thanks,
> Jini.
>
> On 10/27/2017 9:19 PM, Jini George wrote:
>> Thank you very much, Serguei.
>>
>> -Jini.
>>
>> On 10/27/2017 2:22 PM, [hidden email] wrote:
>>> Hi Jini,
>>>
>>> The fix looks good to me.
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>> On 10/24/17 00:31, Jini George wrote:
>>>> Adding hotspot-dev too.
>>>>
>>>> Thanks,
>>>> Jini.
>>>>
>>>> On 10/24/2017 12:05 PM, Jini George wrote:
>>>>> Hello,
>>>>>
>>>>> As a part of SA next, I am working on writing a test case which
>>>>> compares the fields and the types of the fields of the SA java
>>>>> classes with the corresponding entries in the vmStructs tables.
>>>>> This, to some extent, would help in preventing errors in SA due to
>>>>> the changes in hotspot. As a precursor to this, I am in the
>>>>> process of making some cleanup related changes (mostly in SA). I
>>>>> plan to have the changes done in parts. For this webrev, most of
>>>>> the changes are for:
>>>>>
>>>>> 1. Avoiding having some values being redefined in SA. Instead have
>>>>> those exported through vmStructs, and read it in SA.
>>>>> (CompactibleFreeListSpace::_min_chunk_size_in_bytes,
>>>>> CompactibleFreeListSpace::IndexSetSize)
>>>>>
>>>>> Redefinition of hotspot values in SA makes SA error prone, when
>>>>> the value gets altered in hotspot and the corresponding
>>>>> modification gets missed out in SA.
>>>>>
>>>>> 2. To remove some unused code (JNIid.java).
>>>>> 3. Add the missing "CMSBitMap::_bmStartWord" in vmStructs.
>>>>> 4. Modify variable names in SA and hotspot to match the
>>>>> counterpart names, so that the comparison of the fields become
>>>>> easier. Most of the changes belong to this group.
>>>>>
>>>>> Could I please get reviews done for these precursor changes ?
>>>>>
>>>>> JBS Id: https://bugs.openjdk.java.net/browse/JDK-8189798
>>>>> webrev: http://cr.openjdk.java.net/~jgeorge/8189798/webrev.00/
>>>>>
>>>>> Thank you,
>>>>> Jini.
>>>>>
>>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: SA: JDK-8189798: SA cleanup - part 1

Jini George
Thank you very much, Coleen, for the review. My answers inline:

On 11/2/2017 5:09 PM, [hidden email] wrote:
>
> http://cr.openjdk.java.net/~jgeorge/8189798/webrev.00/src/hotspot/share/runtime/stackValue.hpp.udiff.html 
>
>
> + return (*(int *)&_integer_value == *(int *)&value->_integer_value);
>
>
> I don't think the *(int*) casts for _integer_value are needed in these
> files.  Can you remove them?

[Jini] I think since _integer_value is of type intptr_t (which could be
8 or 4 bytes long depending on the data model), the removal of the casts
could result in an incorrect comparison (mostly for the 64 bit
environment). Let me know if you disagree.

> http://cr.openjdk.java.net/~jgeorge/8189798/webrev.00/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/cms/CompactibleFreeListSpace.java.udiff.html 
>
>
> I'm not really sure why you still have SA code for CMS, since CMS is
> deprecated.  What does it do?  Can it be removed in a following cleanup?

[Jini] My understanding is that we would need to have it as long as we
have it in Hotspot. Once the files
src/hotspot/share/gc/cms/compactibleFreeListSpace* get removed from
Hotspot, we can remove it from SA.

Thank you,
Jini.

>
> This cleanup looks good.  And thank you for doing this since I broke SA
> only yesterday.
>
> thanks,
> Coleen
>
>
> On 11/2/17 12:54 AM, Jini George wrote:
>> Could I please get one more review done for this ?
>>
>> Thanks,
>> Jini.
>>
>> On 10/27/2017 9:19 PM, Jini George wrote:
>>> Thank you very much, Serguei.
>>>
>>> -Jini.
>>>
>>> On 10/27/2017 2:22 PM, [hidden email] wrote:
>>>> Hi Jini,
>>>>
>>>> The fix looks good to me.
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 10/24/17 00:31, Jini George wrote:
>>>>> Adding hotspot-dev too.
>>>>>
>>>>> Thanks,
>>>>> Jini.
>>>>>
>>>>> On 10/24/2017 12:05 PM, Jini George wrote:
>>>>>> Hello,
>>>>>>
>>>>>> As a part of SA next, I am working on writing a test case which
>>>>>> compares the fields and the types of the fields of the SA java
>>>>>> classes with the corresponding entries in the vmStructs tables.
>>>>>> This, to some extent, would help in preventing errors in SA due to
>>>>>> the changes in hotspot. As a precursor to this, I am in the
>>>>>> process of making some cleanup related changes (mostly in SA). I
>>>>>> plan to have the changes done in parts. For this webrev, most of
>>>>>> the changes are for:
>>>>>>
>>>>>> 1. Avoiding having some values being redefined in SA. Instead have
>>>>>> those exported through vmStructs, and read it in SA.
>>>>>> (CompactibleFreeListSpace::_min_chunk_size_in_bytes,
>>>>>> CompactibleFreeListSpace::IndexSetSize)
>>>>>>
>>>>>> Redefinition of hotspot values in SA makes SA error prone, when
>>>>>> the value gets altered in hotspot and the corresponding
>>>>>> modification gets missed out in SA.
>>>>>>
>>>>>> 2. To remove some unused code (JNIid.java).
>>>>>> 3. Add the missing "CMSBitMap::_bmStartWord" in vmStructs.
>>>>>> 4. Modify variable names in SA and hotspot to match the
>>>>>> counterpart names, so that the comparison of the fields become
>>>>>> easier. Most of the changes belong to this group.
>>>>>>
>>>>>> Could I please get reviews done for these precursor changes ?
>>>>>>
>>>>>> JBS Id: https://bugs.openjdk.java.net/browse/JDK-8189798
>>>>>> webrev: http://cr.openjdk.java.net/~jgeorge/8189798/webrev.00/
>>>>>>
>>>>>> Thank you,
>>>>>> Jini.
>>>>>>
>>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: SA: JDK-8189798: SA cleanup - part 1

coleen.phillimore


On 11/7/17 4:16 AM, Jini George wrote:

> Thank you very much, Coleen, for the review. My answers inline:
>
> On 11/2/2017 5:09 PM, [hidden email] wrote:
>>
>> http://cr.openjdk.java.net/~jgeorge/8189798/webrev.00/src/hotspot/share/runtime/stackValue.hpp.udiff.html 
>>
>>
>> + return (*(int *)&_integer_value == *(int *)&value->_integer_value);
>>
>>
>> I don't think the *(int*) casts for _integer_value are needed in
>> these files.  Can you remove them?
>
> [Jini] I think since _integer_value is of type intptr_t (which could
> be 8 or 4 bytes long depending on the data model), the removal of the
> casts could result in an incorrect comparison (mostly for the 64 bit
> environment). Let me know if you disagree.

Maybe I read the field type wrong.  You can leave this as it is.

>
>> http://cr.openjdk.java.net/~jgeorge/8189798/webrev.00/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/cms/CompactibleFreeListSpace.java.udiff.html 
>>
>>
>> I'm not really sure why you still have SA code for CMS, since CMS is
>> deprecated.  What does it do?  Can it be removed in a following cleanup?
>
> [Jini] My understanding is that we would need to have it as long as we
> have it in Hotspot. Once the files
> src/hotspot/share/gc/cms/compactibleFreeListSpace* get removed from
> Hotspot, we can remove it from SA.

Ok.
Thanks,
Coleen

>
> Thank you,
> Jini.
>
>>
>> This cleanup looks good.  And thank you for doing this since I broke
>> SA only yesterday.
>>
>> thanks,
>> Coleen
>>
>>
>> On 11/2/17 12:54 AM, Jini George wrote:
>>> Could I please get one more review done for this ?
>>>
>>> Thanks,
>>> Jini.
>>>
>>> On 10/27/2017 9:19 PM, Jini George wrote:
>>>> Thank you very much, Serguei.
>>>>
>>>> -Jini.
>>>>
>>>> On 10/27/2017 2:22 PM, [hidden email] wrote:
>>>>> Hi Jini,
>>>>>
>>>>> The fix looks good to me.
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>> On 10/24/17 00:31, Jini George wrote:
>>>>>> Adding hotspot-dev too.
>>>>>>
>>>>>> Thanks,
>>>>>> Jini.
>>>>>>
>>>>>> On 10/24/2017 12:05 PM, Jini George wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> As a part of SA next, I am working on writing a test case which
>>>>>>> compares the fields and the types of the fields of the SA java
>>>>>>> classes with the corresponding entries in the vmStructs tables.
>>>>>>> This, to some extent, would help in preventing errors in SA due
>>>>>>> to the changes in hotspot. As a precursor to this, I am in the
>>>>>>> process of making some cleanup related changes (mostly in SA). I
>>>>>>> plan to have the changes done in parts. For this webrev, most of
>>>>>>> the changes are for:
>>>>>>>
>>>>>>> 1. Avoiding having some values being redefined in SA. Instead
>>>>>>> have those exported through vmStructs, and read it in SA.
>>>>>>> (CompactibleFreeListSpace::_min_chunk_size_in_bytes,
>>>>>>> CompactibleFreeListSpace::IndexSetSize)
>>>>>>>
>>>>>>> Redefinition of hotspot values in SA makes SA error prone, when
>>>>>>> the value gets altered in hotspot and the corresponding
>>>>>>> modification gets missed out in SA.
>>>>>>>
>>>>>>> 2. To remove some unused code (JNIid.java).
>>>>>>> 3. Add the missing "CMSBitMap::_bmStartWord" in vmStructs.
>>>>>>> 4. Modify variable names in SA and hotspot to match the
>>>>>>> counterpart names, so that the comparison of the fields become
>>>>>>> easier. Most of the changes belong to this group.
>>>>>>>
>>>>>>> Could I please get reviews done for these precursor changes ?
>>>>>>>
>>>>>>> JBS Id: https://bugs.openjdk.java.net/browse/JDK-8189798
>>>>>>> webrev: http://cr.openjdk.java.net/~jgeorge/8189798/webrev.00/
>>>>>>>
>>>>>>> Thank you,
>>>>>>> Jini.
>>>>>>>
>>>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: SA: JDK-8189798: SA cleanup - part 1

Jini George
Thank you, Coleen.

- Jini.

On 11/7/2017 7:29 PM, [hidden email] wrote:

>
>
> On 11/7/17 4:16 AM, Jini George wrote:
>> Thank you very much, Coleen, for the review. My answers inline:
>>
>> On 11/2/2017 5:09 PM, [hidden email] wrote:
>>>
>>> http://cr.openjdk.java.net/~jgeorge/8189798/webrev.00/src/hotspot/share/runtime/stackValue.hpp.udiff.html 
>>>
>>>
>>> + return (*(int *)&_integer_value == *(int *)&value->_integer_value);
>>>
>>>
>>> I don't think the *(int*) casts for _integer_value are needed in
>>> these files.  Can you remove them?
>>
>> [Jini] I think since _integer_value is of type intptr_t (which could
>> be 8 or 4 bytes long depending on the data model), the removal of the
>> casts could result in an incorrect comparison (mostly for the 64 bit
>> environment). Let me know if you disagree.
>
> Maybe I read the field type wrong.  You can leave this as it is.
>>
>>> http://cr.openjdk.java.net/~jgeorge/8189798/webrev.00/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/cms/CompactibleFreeListSpace.java.udiff.html 
>>>
>>>
>>> I'm not really sure why you still have SA code for CMS, since CMS is
>>> deprecated.  What does it do?  Can it be removed in a following cleanup?
>>
>> [Jini] My understanding is that we would need to have it as long as we
>> have it in Hotspot. Once the files
>> src/hotspot/share/gc/cms/compactibleFreeListSpace* get removed from
>> Hotspot, we can remove it from SA.
>
> Ok.
> Thanks,
> Coleen
>>
>> Thank you,
>> Jini.
>>
>>>
>>> This cleanup looks good.  And thank you for doing this since I broke
>>> SA only yesterday.
>>>
>>> thanks,
>>> Coleen
>>>
>>>
>>> On 11/2/17 12:54 AM, Jini George wrote:
>>>> Could I please get one more review done for this ?
>>>>
>>>> Thanks,
>>>> Jini.
>>>>
>>>> On 10/27/2017 9:19 PM, Jini George wrote:
>>>>> Thank you very much, Serguei.
>>>>>
>>>>> -Jini.
>>>>>
>>>>> On 10/27/2017 2:22 PM, [hidden email] wrote:
>>>>>> Hi Jini,
>>>>>>
>>>>>> The fix looks good to me.
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>> On 10/24/17 00:31, Jini George wrote:
>>>>>>> Adding hotspot-dev too.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Jini.
>>>>>>>
>>>>>>> On 10/24/2017 12:05 PM, Jini George wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> As a part of SA next, I am working on writing a test case which
>>>>>>>> compares the fields and the types of the fields of the SA java
>>>>>>>> classes with the corresponding entries in the vmStructs tables.
>>>>>>>> This, to some extent, would help in preventing errors in SA due
>>>>>>>> to the changes in hotspot. As a precursor to this, I am in the
>>>>>>>> process of making some cleanup related changes (mostly in SA). I
>>>>>>>> plan to have the changes done in parts. For this webrev, most of
>>>>>>>> the changes are for:
>>>>>>>>
>>>>>>>> 1. Avoiding having some values being redefined in SA. Instead
>>>>>>>> have those exported through vmStructs, and read it in SA.
>>>>>>>> (CompactibleFreeListSpace::_min_chunk_size_in_bytes,
>>>>>>>> CompactibleFreeListSpace::IndexSetSize)
>>>>>>>>
>>>>>>>> Redefinition of hotspot values in SA makes SA error prone, when
>>>>>>>> the value gets altered in hotspot and the corresponding
>>>>>>>> modification gets missed out in SA.
>>>>>>>>
>>>>>>>> 2. To remove some unused code (JNIid.java).
>>>>>>>> 3. Add the missing "CMSBitMap::_bmStartWord" in vmStructs.
>>>>>>>> 4. Modify variable names in SA and hotspot to match the
>>>>>>>> counterpart names, so that the comparison of the fields become
>>>>>>>> easier. Most of the changes belong to this group.
>>>>>>>>
>>>>>>>> Could I please get reviews done for these precursor changes ?
>>>>>>>>
>>>>>>>> JBS Id: https://bugs.openjdk.java.net/browse/JDK-8189798
>>>>>>>> webrev: http://cr.openjdk.java.net/~jgeorge/8189798/webrev.00/
>>>>>>>>
>>>>>>>> Thank you,
>>>>>>>> Jini.
>>>>>>>>
>>>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: SA: JDK-8189798: SA cleanup - part 1

David Holmes
In reply to this post by coleen.phillimore
Hi Jini,

> On 11/7/17 4:16 AM, Jini George wrote:
>> Thank you very much, Coleen, for the review. My answers inline:
>>
>> On 11/2/2017 5:09 PM, [hidden email] wrote:
>>>
>>> http://cr.openjdk.java.net/~jgeorge/8189798/webrev.00/src/hotspot/share/runtime/stackValue.hpp.udiff.html 
>>>
>>>
>>> + return (*(int *)&_integer_value == *(int *)&value->_integer_value);
>>>
>>>
>>> I don't think the *(int*) casts for _integer_value are needed in
>>> these files.  Can you remove them?
>>
>> [Jini] I think since _integer_value is of type intptr_t (which could
>> be 8 or 4 bytes long depending on the data model), the removal of the
>> casts could result in an incorrect comparison (mostly for the 64 bit
>> environment). Let me know if you disagree.

You're comparing two _integer_value fields that are both intptr_t. The
important part you've overlooked is the comment preceding this:

        // [phh] compare only low addressed portions of intptr_t slots
-      return (*(int *)&_i == *(int *)&value->_i);
+      return (*(int *)&_integer_value == *(int *)&value->_integer_value);

For some we reason although intptr_t we're only interested in the lower
32-bits. I have no idea why, nor why we would truncate the value when
printing.

David
-----
Reply | Threaded
Open this post in threaded view
|

Re: RFR: SA: JDK-8189798: SA cleanup - part 1

Jini George
Hi David,

If we don't retain the cast, wouldn't that mean that we would be
comparing 2 64 bit values in a 64 bit environment which would not be the
intended comparison ?

Thanks,
Jini.

On 11/8/2017 7:49 AM, David Holmes wrote:

> Hi Jini,
>
>> On 11/7/17 4:16 AM, Jini George wrote:
>>> Thank you very much, Coleen, for the review. My answers inline:
>>>
>>> On 11/2/2017 5:09 PM, [hidden email] wrote:
>>>>
>>>> http://cr.openjdk.java.net/~jgeorge/8189798/webrev.00/src/hotspot/share/runtime/stackValue.hpp.udiff.html 
>>>>
>>>>
>>>> + return (*(int *)&_integer_value == *(int *)&value->_integer_value);
>>>>
>>>>
>>>> I don't think the *(int*) casts for _integer_value are needed in
>>>> these files.  Can you remove them?
>>>
>>> [Jini] I think since _integer_value is of type intptr_t (which could
>>> be 8 or 4 bytes long depending on the data model), the removal of the
>>> casts could result in an incorrect comparison (mostly for the 64 bit
>>> environment). Let me know if you disagree.
>
> You're comparing two _integer_value fields that are both intptr_t. The
> important part you've overlooked is the comment preceding this:
>
>         // [phh] compare only low addressed portions of intptr_t slots
> -      return (*(int *)&_i == *(int *)&value->_i);
> +      return (*(int *)&_integer_value == *(int *)&value->_integer_value);
>
> For some we reason although intptr_t we're only interested in the lower
> 32-bits. I have no idea why, nor why we would truncate the value when
> printing.
>
> David
> -----
Reply | Threaded
Open this post in threaded view
|

Re: RFR: SA: JDK-8189798: SA cleanup - part 1

David Holmes
Hi Jini,

On 8/11/2017 4:28 PM, Jini George wrote:
> Hi David,
>
> If we don't retain the cast, wouldn't that mean that we would be
> comparing 2 64 bit values in a 64 bit environment which would not be the
> intended comparison ?

Yes. I was pointing out the reason for the cast.

David

> Thanks,
> Jini.
>
> On 11/8/2017 7:49 AM, David Holmes wrote:
>> Hi Jini,
>>
>>> On 11/7/17 4:16 AM, Jini George wrote:
>>>> Thank you very much, Coleen, for the review. My answers inline:
>>>>
>>>> On 11/2/2017 5:09 PM, [hidden email] wrote:
>>>>>
>>>>> http://cr.openjdk.java.net/~jgeorge/8189798/webrev.00/src/hotspot/share/runtime/stackValue.hpp.udiff.html 
>>>>>
>>>>>
>>>>> + return (*(int *)&_integer_value == *(int *)&value->_integer_value);
>>>>>
>>>>>
>>>>> I don't think the *(int*) casts for _integer_value are needed in
>>>>> these files.  Can you remove them?
>>>>
>>>> [Jini] I think since _integer_value is of type intptr_t (which could
>>>> be 8 or 4 bytes long depending on the data model), the removal of
>>>> the casts could result in an incorrect comparison (mostly for the 64
>>>> bit environment). Let me know if you disagree.
>>
>> You're comparing two _integer_value fields that are both intptr_t. The
>> important part you've overlooked is the comment preceding this:
>>
>>         // [phh] compare only low addressed portions of intptr_t slots
>> -      return (*(int *)&_i == *(int *)&value->_i);
>> +      return (*(int *)&_integer_value == *(int
>> *)&value->_integer_value);
>>
>> For some we reason although intptr_t we're only interested in the
>> lower 32-bits. I have no idea why, nor why we would truncate the value
>> when printing.
>>
>> David
>> -----
Reply | Threaded
Open this post in threaded view
|

Re: RFR: SA: JDK-8189798: SA cleanup - part 1

Jini George
Ok, Thank you!

-Jini.

On 11/8/2017 12:00 PM, David Holmes wrote:

> Hi Jini,
>
> On 8/11/2017 4:28 PM, Jini George wrote:
>> Hi David,
>>
>> If we don't retain the cast, wouldn't that mean that we would be
>> comparing 2 64 bit values in a 64 bit environment which would not be
>> the intended comparison ?
>
> Yes. I was pointing out the reason for the cast.
>
> David
>
>> Thanks,
>> Jini.
>>
>> On 11/8/2017 7:49 AM, David Holmes wrote:
>>> Hi Jini,
>>>
>>>> On 11/7/17 4:16 AM, Jini George wrote:
>>>>> Thank you very much, Coleen, for the review. My answers inline:
>>>>>
>>>>> On 11/2/2017 5:09 PM, [hidden email] wrote:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~jgeorge/8189798/webrev.00/src/hotspot/share/runtime/stackValue.hpp.udiff.html 
>>>>>>
>>>>>>
>>>>>> + return (*(int *)&_integer_value == *(int *)&value->_integer_value);
>>>>>>
>>>>>>
>>>>>> I don't think the *(int*) casts for _integer_value are needed in
>>>>>> these files.  Can you remove them?
>>>>>
>>>>> [Jini] I think since _integer_value is of type intptr_t (which
>>>>> could be 8 or 4 bytes long depending on the data model), the
>>>>> removal of the casts could result in an incorrect comparison
>>>>> (mostly for the 64 bit environment). Let me know if you disagree.
>>>
>>> You're comparing two _integer_value fields that are both intptr_t.
>>> The important part you've overlooked is the comment preceding this:
>>>
>>>         // [phh] compare only low addressed portions of intptr_t slots
>>> -      return (*(int *)&_i == *(int *)&value->_i);
>>> +      return (*(int *)&_integer_value == *(int
>>> *)&value->_integer_value);
>>>
>>> For some we reason although intptr_t we're only interested in the
>>> lower 32-bits. I have no idea why, nor why we would truncate the
>>> value when printing.
>>>
>>> David
>>> -----