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

[Albert Noll]

Hi all,

could I have a second review for this? It;s only a couple of lines and 
Vladimir already looked at it.

Many thanks,
Albert

On 27.06.2013 07:49, Vladimir Kozlov wrote:
> 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) .
>>>>>>
>>>>
>>