[9] RFR(S): 8064892: Non-methods code cache overflow is not handled correctly

[Dmitrij Pochepko]

Hi,
> On 8/18/16 12:29 AM, Tobias Hartmann wrote:
>> Hi Vladimir,
>>
>> thanks for the review!
>>
>> On 17.08.2016 21:42, Vladimir Kozlov wrote:
>>> The print code (include guarding check) should be under #ifdef 
>>> ASSERT since the flag is debug flag.
>>
>> Why is that necessary? Develop flags are replaced by constants in 
>> product and such checks will be constant folded:
>> globals.hpp:583: // develop flags are settable / visible only during 
>> development and are constant in the PRODUCT version
>>
>> We have the same (unguarded) check below in codeCache.cpp:495 and in 
>> general we don't guard other develop flags with #ifdef ASSERT (for 
>> example, TraceICs).
>
> My mistake (I was confused by udiffs). I thought "(type != 
> code_blob_type && ..." check guards only assert code.
> Yes, you don't need #ifdef here.
>
>>
>>> Otherwise looks good. So JFR will get this new message instead. Right?
>>
>> By "new message", do you mean the code cache full message now 
>> reporting the 'orig_code_blob_type'? If so, yes, the originally 
>> requested code blob type is passed to the JFR event via 
>> CompileBroker::handle_full_code_cache() -> 
>> CodeCache::report_codemem_full() -> 
>> EventCodeCacheFull::set_codeBlobType(). But as before, a JFR event is 
>> triggered only if the entire code cache is full. We don't trigger a 
>> "Code cache full" event if only a single segment/heap is full and we 
>> are able to store the code in another heap. For this case, I added 
>> the PrintCodeCacheExtension output.
>
> Reading through bug report and it was requested originally always fire 
> an event even if there is space in other segments.
> But you don't do that (case 2.) with reasonable arguments and I agree. 
> But you need agreement from Dmitrij who filed the bug.
Original bug report was about jfr event sent for 2 of 3 heaps and 
missing for 3rd heap. Any fix which have logical behavior would be fine. 
So, this fix looks good to me.

Thanks,
Dmitrij
>
> Thanks,
> Vladimir
>
>>
>> Thanks,
>> Tobias
>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 8/17/16 4:04 AM, Tobias Hartmann wrote:
>>>> Hi,
>>>>
>>>> please review the following patch:
>>>> https://bugs.openjdk.java.net/browse/JDK-8064892
>>>> http://cr.openjdk.java.net/~thartmann/8064892/webrev.00/
>>>>
>>>> There are two problems with the current handling of code cache 
>>>> exhaustion:
>>>> 1) If the non-nmethod code heap is full, we always try to use the 
>>>> non-profiled or the profiled code heap as a fallback. If those 
>>>> heaps are full as well, we issue a warning stating that the last 
>>>> tried code heap is full. Like this, we never print a warning for 
>>>> the non-nmethod code heap.
>>>> 2) If a code heap is full but we still have enough space in another 
>>>> code heap, no warning or JFR event is triggered.
>>>>
>>>> I fixed 1) by keeping track of the originally requested code blob 
>>>> type and use it for printing the code cache full message if 
>>>> allocation in other heaps failed. Regarding 2), I don't think a 
>>>> code cache full message or JFR event should be triggered because 
>>>> the code cache is not full, we just failed to allocate in the 
>>>> preferred segment. In general, this is expected because the amount 
>>>> of code generated at runtime is not static but segment sizes are. 
>>>> The user should not worry about such fallbacks and therefore we 
>>>> should only print a warning if the entire code cache is full. 
>>>> However, to keep track of this, I added debug output that can be 
>>>> enabled via -XX:+PrintCodeCacheExtension and prints a message if 
>>>> allocation in the preferred segment failed. For example, "Extension 
>>>> of CodeHeap 'non-nmethods' failed. Trying to allocate in CodeHeap 
>>>> 'non-profiled nmethods'".
>>>>
>>>> Tested with JPRT and RBT (running).
>>>>
>>>> Thanks,
>>>> Tobias
>>>>