RFR: SA: JDK-8189798: SA cleanup - part 1
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Tue Nov 7 13:59:06 UTC 2017
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, coleen.phillimore at oracle.com 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, serguei.spitsyn at oracle.com 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.
>>>>>>>
>>>>>
>>
More information about the hotspot-dev
mailing list