RFR (M): 8016749: -XX:+UseISM fails an assert(obj->is_oop()) when running SPECjbb2005

[Albert Noll]

Hi Valdimir,

thanks for looking at the patch. I implemented all changes that you 
proposed.
Here is the new webrev:
http://cr.openjdk.java.net/~anoll/8016749/webrev.03/ 
<http://cr.openjdk.java.net/%7Eanoll/8016749/webrev.03/>

Could I get a second review for this?

Mant thanks,
Albert


On 10.07.2013 07:50, Vladimir Kozlov wrote:
> In general it is good. Few comments about style:
>
> Add () for (&& (>)))
>
> if (UseLargePages && (alignment_hint > (size_t)vm_page_size())) {
>
> In os::large_page_init() reverse condition to execute code in method 
> instead of check and return.
>
> Add space: //Upon
>
> Don't convert to one line next code:
>
> 5435     if (!UseNUMA && ForceNUMA) {
> 5436       UseNUMA = true;
> 5437     }
>
> Thanks,
> Vladimir
>
> On 7/3/13 12:41 AM, Albert Noll wrote:
>> Hi Valdimir,
>>
>> you are right. These methods should not be called when there is no 
>> support for ISM. I added the fatal(#method_name
>> should not be called on Solaris). In addition, I found more code in 
>> os_solaris.cpp that is only there to provide support
>> for Solaris 8. I removed that code as well.
>>
>> A jprt run was successful. I also checked that large pages can still 
>> be used.
>>
>> Many thanks for your help,
>> Albert
>>
>> Here is the new webrev:
>> http://cr.openjdk.java.net/~anoll/8016749/webrev.02/ 
>> <http://cr.openjdk.java.net/%7Eanoll/8016749/webrev.02/>
>>
>>
>>
>> On 02.07.2013 17:14, Vladimir Kozlov wrote:
>>> On 7/2/13 8:07 AM, Vladimir Kozlov wrote:
>>>> Albert,
>>>>
>>>> reserve_memory_special() had assert which indicated that it is only 
>>>> called with UseISM.
>>>> Look on code in runtime/virtualspace.cpp - 
>>>> os::reserve_memory_special() is only call when
>>>> os::can_commit_large_page_memory() return false. With your changes 
>>>> it always return 'true' (which is correct).
>>>> I think you can replace whole body of reserve_memory_special() with 
>>>> fatal("this obsolete method should not be called on
>>>> Solaris"); or something.
>>>
>>> Remove 'obsolete' from the message sine the method is not obsolete 
>>> on other platforms.
>>>
>>> Vladimir
>>>
>>>> The same is for release_memory_special() but, please, verify.
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 7/2/13 6:41 AM, Albert Noll wrote:
>>>>> Christian, Vladimir, thanks for your feedback.
>>>>>
>>>>> I added the removed flags to the ObsoleteFlag list.
>>>>>
>>>>> Vladimir, you are right. Removing all mpss-related code does not 
>>>>> allow to use large pages anymore. I followed your
>>>>> suggestions and replaced 'UseMPSS' with 'UseLargePages'. I also 
>>>>> checked if large pages work with 'pmap -xs' when the
>>>>> changes in webrev.01 (see below).
>>>>>
>>>>> Here is the new webrev:
>>>>> http://cr.openjdk.java.net/~anoll/8016749/webrev.01/ 
>>>>> <http://cr.openjdk.java.net/%7Eanoll/8016749/webrev.01/>
>>>>>
>>>>> A jprt run with the above changes was successful.
>>>>>
>>>>> Thanks again,
>>>>> Albert
>>>>>
>>>>> On 02.07.2013 04:28, Vladimir Kozlov wrote:
>>>>>> Albert,
>>>>>>
>>>>>> I don't think it is incorrect changes. Only UseISM is deprecated. 
>>>>>> We still use MPSS for large pages:
>>>>>>
>>>>>> // UseLargePages UseMPSS UseISM
>>>>>> //     true      true     false => JVM will use MPSS for large 
>>>>>> page memory.
>>>>>> //                                 This is the default behavior.
>>>>>>
>>>>>> //     true      false    false => Unless future Solaris provides 
>>>>>> other
>>>>>> //                                 mechanism to use large page 
>>>>>> memory, this
>>>>>> //                                 combination is equivalent to 
>>>>>> -UseLargePages,
>>>>>> //                                 VM will not use large page memory
>>>>>>
>>>>>>
>>>>>> UseMPSS is true by default and most likely it will not work 
>>>>>> without it. So you should leave current functionality but
>>>>>> replace UseMPSS with UseLargePages flag checks in os_solaris.cpp.
>>>>>>
>>>>>> Also verify that large pages are used by java (pmap -xs) after 
>>>>>> your changes.
>>>>>>
>>>>>> thanks,
>>>>>> Vladimir
>>>>>>
>>>>>> On 7/1/13 8:35 AM, Albert Noll wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> thanks for reviewing this patch:
>>>>>>>
>>>>>>> jbs: https://jbs.oracle.com/bugs/browse/JDK-8016749
>>>>>>> webrev: http://cr.openjdk.java.net/~anoll/8016749/webrev.00/
>>>>>>> <http://cr.openjdk.java.net/%7Eanoll/8016749/webrev.00/>
>>>>>>>
>>>>>>> Problem: ISM and MPSS are part of Solaris 9, and we do not 
>>>>>>> support this
>>>>>>> version of Solaris anymore. Instead, we use LargePages for 
>>>>>>> Solaris 10+.
>>>>>>>
>>>>>>> Solution: Remove all code that relates to ISM and MPSS (i.e., 
>>>>>>> code that
>>>>>>> relates to the 'UseISM' and 'UseMPSS' flags).
>>>>>>>
>>>>>>> Testing: the following jprt run passes successfully.
>>>>>>>
>>>>>>> jprt submit -stree . -ot 'solaris.*' -emailalbert.noll at oracle.com
>>>>>>>
>>>>>>> Best,
>>>>>>> Albert
>>>>>
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20130710/20f785f7/attachment.html