[9] RFR(L): 8046809: vm/mlvm/meth/stress/compiler/deoptimize CodeCache is full.

[Albert Noll]

Can I have a second review?

Thanks,
Albert

On 10/14/2014 10:03 AM, Albert Noll wrote:
> Hi Vladimir,
>
> I filed a CCC request and added the 3 removed flags to the 
> obsolete_jvm_flags table.
>
> Here is the updated webrev:
> http://cr.openjdk.java.net/~anoll/8046809/webrev.05/
>
> Thanks,
> Albert
>
> On 10/14/2014 04:00 AM, Vladimir Kozlov wrote:
>> Looks good but you need to wait CCC approval for product flags removal.
>> And you need to add them to obsolete_jvm_flags table in arguments.cpp.
>>
>> Thanks,
>> Vladimir
>>
>> On 10/13/14 7:55 AM, Albert Noll wrote:
>>> Hi Vladimir,
>>>
>>> thanks for the feedback. Here is the updated webrev:
>>>
>>> http://cr.openjdk.java.net/~anoll/8046809/webrev.04/
>>>
>>> Best,
>>> Albert
>>>
>>> On 10/10/2014 06:57 PM, Vladimir Kozlov wrote:
>>>> On 10/10/14 7:30 AM, Albert Noll wrote:
>>>>> Tobias, Vladimir, Dean, Nils, thanks for looking at the patch and 
>>>>> for your feedback.
>>>>>
>>>>> @Tobias
>>>>> I have adapted your suggestions.
>>>>>
>>>>> @Vladimir
>>>>> I tried to add a condition to 
>>>>> SafepointSynchronize::is_cleanup_needed() that returns 'true' if 
>>>>> the code cache has less
>>>>> than 10% free space. Unfortunately, that does not fix the bug. A 
>>>>> safepoint interval of 1000ms seems to be too coarse
>>>>> to free space in the code cache fast enough.
>>>>
>>>> Okay, thank you for trying.
>>>>
>>>>>
>>>>> It seems that the concept of critical memory allocations 
>>>>> (allocations that must succeed for the VM to be able to
>>>>> continue to execute) is broken. For example, we need to compile 
>>>>> method handle intrinsics (otherwise we get a
>>>>> java.lang.VirtualMachineError: out of space in CodeCache for 
>>>>> method handle intrinsic). However, method handle intrinsic
>>>>> are not critical allocations. For this reason, 
>>>>> test/compiler/startup/SmallCodeCacheStartup.java fails with a 32-bit
>>>>> client
>>>>> version. We will get a new nightly bug soon... The current patch 
>>>>> fixes this.
>>>>>
>>>>> I want to keep the removal of critical allocations in this patch, 
>>>>> since aggressive sweeping (enabled by the VM
>>>>> operation that forces stack scanning) replaces the concept of 
>>>>> critical allocations in the code cache. I think
>>>>> these two changes belong together. If you still want me to, I will 
>>>>> do a separate change for critical allocation
>>>>> removal.
>>>>
>>>> Okay, I am fine with this removal.
>>>>
>>>>>
>>>>> @Dean
>>>>> I removed the corresponding code. It was a fragment that I missed 
>>>>> to delete.
>>>>>
>>>>> @Nils
>>>>> CodeCacheMinimumFreeSpace did not guarantee that the VM can 
>>>>> continue to execute. In the failing test
>>>>> that is reported in the bug, 500K was not enough to generate all 
>>>>> adapters. The test can be changed such
>>>>> that a CodeCacheMinimumFreeSpace size of 1m, 2m, 3m, etc. is too 
>>>>> small too. What value should we choose?
>>>>>
>>>>> Also, as noted above, method handle intrinsic need to be compiled 
>>>>> to be able to continue execution.
>>>>> Method handle intrinsic are currently not critical, so we must 
>>>>> make them critical allocations. As a consequence,
>>>>> we must re-evaluate the default size of CodeCacheMinimumFreeSpace.
>>>>>
>>>>> The current approach enables very aggressive sweeping, if the code 
>>>>> cache is 90% full. It is very likely that code
>>>>> will be flushed from the code cache in the next 5-10 invocations 
>>>>> of CodeCache::allocate(). In a sense, the remaining
>>>>> 10% can be considered as a 'critical region' that is used as a 
>>>>> 'buffer' until we free space in the code cache. This bug
>>>>> proves that the proposed strategy solves the problem better than 
>>>>> CodeCacheMinimumFreeSpace.
>>>>>
>>>>> Maybe we should provide the user with control over this threshold, 
>>>>> i.e., replace CodeCacheMinimumFreeSpace
>>>>> with a different command line flag that allows the user to specify 
>>>>> the percentage (currently 90%) at which aggressive
>>>>
>>>> Yes, it should be flag. I think it should be percentage.
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>>> sweeping starts. We could also use a fixed-size. I don't think 
>>>>> that having two thresholds (the threshold where we start
>>>>> aggressive sweeping AND CodeCacheMinimumFreeSpace) is necessary.
>>>>
>>>>>
>>>>> What do you think?
>>>>>
>>>>>
>>>>>
>>>>> The performance runs show that waking up the sweeper thread for 
>>>>> every allocation has a negative impact
>>>>> on startup time. To fix this, in the current patch, the sweeper 
>>>>> thread is only woken up, if the code cache is
>>>>> more than 10% occupied. I will issue a new performance run and 
>>>>> compare it against b34 (which includes
>>>>> the segmented code cache).
>>>>>
>>>>> Here is the new webrev:
>>>>> http://cr.openjdk.java.net/~anoll/8046809/webrev.03/
>>>>>
>>>>> Best,
>>>>> Albert
>>>>>
>>>>>
>>>>> On 10/10/2014 11:01 AM, Nils Eliasson wrote:
>>>>>> Hi, Albert
>>>>>>
>>>>>> Overall a very welcome change to move the sweeper into a separate 
>>>>>> thread.
>>>>>>
>>>>>> On 2014-10-09 10:24, Albert Noll wrote:
>>>>>>> The patch also removes CodeCacheMinimumFreeSpace and 'critical' 
>>>>>>> code cache allocations. Due to a bug in
>>>>>>> heap.cpp, the CodeCacheMinimumFreeSpace was in fact not reserved 
>>>>>>> for 'critical' allocations. The following
>>>>>>> lines produce an underflow if heap_unallocated_capacity() < 
>>>>>>> CodeCacheMinimumFreeSpace:
>>>>>>>
>>>>>>> segments_to_size(number_of_segments) > 
>>>>>>> (heap_unallocated_capacity() - CodeCacheMinimumFreeSpace)
>>>>>>>
>>>>>>> Since the critical part in the code cache was never used for its 
>>>>>>> intended purpose and we did not have problems
>>>>>>> due to that, we can remove it.
>>>>>>
>>>>>> Are you sure? The reasons for the CodeCacheMinimumFreeSpace and 
>>>>>> critical allocations where problems with code cache
>>>>>> fragmentation in long running applications, where small 
>>>>>> compilations would starve out the adaptors and cause VM
>>>>>> shutdown. You won't see this other than in the really long 
>>>>>> running tests. It might be broken - but then we should open
>>>>>> a bug and fix it.  (And in the long run we should handle the 
>>>>>> fragmentation with relocating code.)
>>>>>>
>>>>>> Regards,
>>>>>> //Nils
>>>>>
>>>
>