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

[Vladimir Kozlov]

Looks good to me.

You need look from an other reviewer for this.

Thanks,
Vladimir

On 6/26/13 10:40 PM, Albert Noll wrote:
> 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) .
>>>>>
>>>
>