RFR (S): 8014972: Crash with specific values for -XX:InitialCodeCacheSize=500K -XX:ReservedCodeCacheSize=500k

[Albert Noll]

Hi Valdimir,

thanks for looking at the changes again.
Here is the new webrev:
http://cr.openjdk.java.net/~anoll/8014972/webrev.03/ 
<http://cr.openjdk.java.net/%7Eanoll/8014972/webrev.03/>


On 26.06.2013 19:45, Vladimir Kozlov wrote:
> Sorry, I missed 8015635, lets do max size check as separate fix.
I removed the check.

> You don't need next comment in flag description:
>
> "Decreasing the value can crash the VM when starting up."
>
> Main flag's text could be:
>
> "Minimum code cache size (in bytes) required to start VM"
>
Done.

> Comment in arguments.cpp should be:
>
> // Template Interpreter code is approximately 3X larger in debug builds.
>
Done.

> Use vm_page_size in first check for CodeCacheExpansionSize:
>
> parse_memory_size(tail, &long_CodeCacheExpansionSize, 
> os::vm_page_size());
>
> We don't check for value is multiplier because we do rounding (it is 
> normal practice in Hotspot):
>
> src/share/vm/code/codeCache.cpp:  CodeCacheExpansionSize = 
> round_to(CodeCacheExpansionSize, os::vm_page_size());
>
> So don't do second CodeCacheExpansionSize check.
>
Done.
> And don't forget to set CodeCacheMinimumUseSpace in closed sources.
>
Thanks, I've done that now too. There will be a separate mail for the 
closed changes.

Best,
Albert

> Thanks,
> Vladimir
>
> On 6/26/13 5:25 AM, Albert Noll wrote:
>> Hi Vladimir,
>> thanks for the feedback.
>>
>> I implemented the suggested changes. Here is the new webrev:
>> http://cr.openjdk.java.net/~anoll/8014972/webrev.02/
>> <http://cr.openjdk.java.net/%7Eanoll/8014972/webrev.02/>
>>
>> On 21.06.2013 19:38, Vladimir Kozlov wrote:
>>> I don't like name, may be CodeCacheMinimumUseSpace is better since it
>>> complements CodeCacheMinimumFreeSpace.
>> Done.
>>> An other thing this flag should be develop_pd() platform dependent
>>> because on different platforms you need different size. Zero or Core
>>> are not using template interpreter so they can run with smaller
>>> codecache.
>> Done. I set the CodeCacheMinimumUseSpace for Zero to 200k, since the
>> template interpreter requires around 200K.
>>
>>> Now back to changes. You did not add < 2*G check as I asked. No need
>>> for flag.
>> Sorry, I forgot to mention in the last mail that I thought that checking
>> the upper limit is a different bug (
>> https://jbs.oracle.com/bugs/browse/JDK-8015635). I added the check in
>> the current version.
>>
>>> First check should be since it can't be less then physical page size
>>> (4K) InitialCodeCacheSize should be multiplier of 
>>> CodeCacheExpansionSize:
>>>
>>>  if (InitialCodeCacheSize < CodeCacheExpansionSize)
>>>
>> I added an additional check in arguments.cpp that validates the
>> parameter  CodeCacheExpansionSize.
>> CodeCacheExpansionSize is now required to be
>>
>> 1) larger than 0 and
>> 2) a multiple of os::vm_page_size();
>>
>> So, the first check is: if(InitialCodeCacheSize <
>> (uintx)os::vm_page_size()). I hope that's fine.
>>
>> Best,
>> Albert
>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 6/21/13 1:35 AM, Albert Noll wrote:
>>>> Hi,
>>>>
>>>> Vladimir, thanks for your comments. The solution was indeed more
>>>> complicated than necessary.
>>>>
>>>> The current version is much simpler. I think the cleanest solution 
>>>> is to
>>>> introduce the parameter "BootstrapCodeCacheSize" (an alternative name
>>>> would be StartupCodeCacheSize) to define the amount of memory required
>>>> to startup the VM. This value does not depend on InitialCodeCacheSize
>>>> and CodeCacheMinimumFreeSpace, which makes the necessary checks 
>>>> simpler.
>>>>
>>>> webrev: http://cr.openjdk.java.net/~anoll/8014972/webrev.01/
>>>> <http://cr.openjdk.java.net/%7Eanoll/8014972/webrev.01/>
>>>>
>>>> Thanks,
>>>> Albert
>>>>
>>>>
>>>> On 20.06.2013 22:46, Vladimir Kozlov wrote:
>>>>> Also it could be confusing that MinimumCodeCacheSize is less then
>>>>> CodeCacheMinimumFreeSpace. So the definition should be:
>>>>>
>>>>> develop(uintx, MinimumInitialCodeCacheSize, 900*K,
>>>>>
>>>>> And in arguments.cpp you need to do next since
>>>>> CodeCacheMinimumFreeSpace could be set on command line:
>>>>>
>>>>> uint min_code_cache_size = (K * NOT_DEBUG(400) DEBUG_ONLY(1200)) +
>>>>> CodeCacheMinimumFreeSpace;
>>>>> min_code_cache_size = MIN2(MinimumCodeCacheSize, 
>>>>> min_code_cache_size);
>>>>>
>>>>> thanks,
>>>>> Vladimir
>>>>>
>>>>> On 6/20/13 1:27 PM, Vladimir Kozlov wrote:
>>>>>> Albert,
>>>>>>
>>>>>> Please, place new flag in globals.hpp after InitialCodeCacheSize
>>>>>> definition since they are related.
>>>>>>
>>>>>> MinimumInitialCodeCacheSize should be MinimumCodeCacheSize.
>>>>>> We also asked to check upper limit since we can use > 2Gb (MAXINT)
>>>>>> size
>>>>>> for CodeCache.
>>>>>>
>>>>>> Also your changes could be much simpler if you just add the check
>>>>>> (InitialCodeCacheSize < min_code_cache_size) before existing check
>>>>>> (ReservedCodeCacheSize < InitialCodeCacheSize).
>>>>>>
>>>>>> And, please, don't use different message for debug build.
>>>>>>
>>>>>> '* 3' for debug build could be done as:
>>>>>>
>>>>>> uint min_code_cache_size = MinimumInitialCodeCacheSize DEBUG_ONLY(*
>>>>>> 3) +
>>>>>> CodeCacheMinimumFreeSpace;
>>>>>>
>>>>>> thanks,
>>>>>> Vladimir
>>>>>>
>>>>>> On 6/20/13 4:02 AM, Albert Noll wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> thanks for reviewing this small patch.
>>>>>>>
>>>>>>> Albert
>>>>>>>
>>>>>>> jbs: https://jbs.oracle.com/bugs/browse/JDK-8014972
>>>>>>> webrev: http://cr.openjdk.java.net/~anoll/8014972/webrev.00/
>>>>>>> <http://cr.openjdk.java.net/%7Eanoll/8014972/webrev.00/>
>>>>>>>
>>>>>>>
>>>>>>> Problem:
>>>>>>> If -XX:ReservedCodeCacheSize is set too small, the VM crashes when
>>>>>>> starting up.
>>>>>>>
>>>>>>> Solution:
>>>>>>> Introduce a minimum code cache size (ReservedCodeCacheSize) that
>>>>>>> guarantees that the VM can startup. The minimum code cache size is
>>>>>>> defined by a new parameter (MinimumInitialCodeCacheSize) that
>>>>>>> cannot be
>>>>>>> changed in product build. My reasoning for this is that smaller
>>>>>>> values
>>>>>>> make the JVM crash at startup. However, in a development build, the
>>>>>>> parameter can be changed.
>>>>>>>
>>>>>>> The current patch sets MinimumInitialCodeCacheSize to 400k for all
>>>>>>> platforms. On Solaris, MinimumInitialCodeCacheSize could be set to
>>>>>>> 300k
>>>>>>> and the VM still starts up. However, I am not sure if it makes
>>>>>>> sense to
>>>>>>> make MinimumInitialCodeCacheSize platform-dependent so that we can
>>>>>>> potentially save 100k on Solaris. If there is a reason, please 
>>>>>>> let me
>>>>>>> know.
>>>>>>>
>>>>>>> Also note that the InitialCodeCacheSize must be > 0. The reason is
>>>>>>> that
>>>>>>> otherwise the following  assertion fails:
>>>>>>>
>>>>>>> Internal Error
>>>>>>> (/tmp/workspace/2-build-solaris-i586/jdk8/4767/hotspot/src/share/vm/memory/heap.cpp:50), 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> pid=27190, tid=2
>>>>>>> #  assert(0 <= beg && beg < _number_of_committed_segments) failed:
>>>>>>> interval begin out of bounds
>>>>>>>
>>>>>>>
>>>>>>> Testing:
>>>>>>> With the current value of MinimumInitialCodeCacheSize, the VM
>>>>>>> starts up
>>>>>>> on all Windows, Linux, and Solaris.
>>>>>>>
>>>>>>> Note that we can still get exceptions (and the JVM cannot 
>>>>>>> continue to
>>>>>>> execute) if the value for CodeCacheMinimumFreeSpace is to small (in
>>>>>>> that
>>>>>>> case, here is not enough space for adapters) .
>>>>
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20130627/2b113951/attachment-0001.html