RFR (M): 8006952: Slow VM due to excessive code cache freelist iteration

Nils Eliasson nils.eliasson at oracle.com
Tue Apr 16 03:16:30 PDT 2013 

Latest webrev: http://cr.openjdk.java.net/~neliasso/8006952/webrev.09/ 
<http://cr.openjdk.java.net/%7Eneliasso/8006952/webrev.09/>

Updates after comments from Vladimir K. and Niclas.

- Removed 'red_zone' naming and refined comments
- Refined naming in heap.cpp/hpp on conversion methods 
size(...)/number_of_segments(...) to 
size_to_segments(...)/segments_to_size(...)
- Bool flags as verbs (critical -> is_critical)
- Removed get_largest_block() and all its uses.

Enjoy!

//Nils

On 2013-04-15 12:36, Nils Eliasson wrote:
>
> Hi,
>
> On 2013-04-12 19:21, Vladimir Kozlov wrote:
>> Nils,
>>
>> It is sad that you have to reverse parfait fix but I agree that your 
>> changes are more clean.
>
> Please - don't be! I taken extra precaution and have verified that 
> there is a null check between the call to new and the constructor. 
> This is consistent with both the C++ spec and the behaviour we rely on 
> when running out of code cache. The CodeCache::has_space would have 
> forced me to do it more complex, it would have to copy/wrap the 
> heap::allocate algorithm but returning a bool instead of actually 
> allocating.
>
> Sorry for forgetting to add it to the change log in my request. I did 
> not intend to revert it without being open about it.
>
> I don't understand why Parfait is confused about 'this' being null in 
> the constructor. I will open a bug on Parfait if I can find where to 
> submit it.
>
>>
>> After these changes largest_free_block() is used only in statistics 
>> output which is useless since it is not participating in codecache 
>> allocations.  May be we should remove it and use 
>> heap_unallocated_capacity() instead (do we need lock then?):
>
> Yes, good idea. Must check that I don't break any log tool if I change 
> the output though.
>
>>
>> size_t CodeCache::largest_free_block() {
>>   return _heap->heap_unallocated_capacity();
>> }
>>
>> Also red_zone_start() is confusing because it does not show how it is 
>> related to heap_unallocated_capacity(). In one place you use one:
>>
>>  211     if (size(length) > (heap_unallocated_capacity() - 
>> CodeCacheMinimumFreeSpace)) {
>>
>> and in an other use second:
>>
>>  437     if (((size_t)cur + length) >= (size_t)red_zone_start()) {
>>
>> Can we just use only heap_unallocated_capacity()?
>
> I agree that it is confusing. The background is that they serve a bit 
> different purpose. In one case we are allocating from the freelist and 
> need to check against the adress where the 'red zone/critical 
> allocation area begins.' In the other it is a block allocation from 
> the contiguous heap where we know heap_unallocated_capacity is the 
> size of the block at the end of the heap. I'll come up with something 
> better.
>
> Thanks for taking your time!
> //Nils
>
>>
>> Thanks,
>> Vladimir
>>
>> On 4/12/13 3:19 AM, Nils Eliasson wrote:
>>> I have now incorporated both Vladimirs and Andreas suggestion:
>>>
>>> Changes to previous revision includes:
>>> 1) Updated the checks in both heap::allocate and heap::search_freelist
>>> so that non critical allocation never stride into the
>>> CodeCacheMinimumFreeSpace.
>>> 2) Inverted the critical condition - now all allocations default to not
>>> critical. Only the places where we would go fatal if the allocation
>>> returns null are critical.
>>> 3) Refactored some names and removed duplicate methods in heap.cpp.
>>>
>>> http://cr.openjdk.java.net/~neliasso/8006952/webrev.08/
>>> <http://cr.openjdk.java.net/%7Eneliasso/8006952/webrev.08/>
>>>
>>> This changes also opens up for decreasing the CodeCacheMinimumFreeSpace
>>> since we now guarantee that only critical allocation will end up there.
>>> And they will not grow to 500K when compilation have stopped anyways.
>>>
>>> Kind regards,
>>> //Nils
>>>
>>>
>>> On 2013-02-08 15:54, Nils Eliasson wrote:
>>>> Vladimir and Andreas, thanks for your feedback! I am considering your
>>>> suggestions.
>>>>
>>>> My fix is a step in the right direction but I realize it is not 
>>>> enough.
>>>>
>>>> Initially, it works good reserving the last part of the heap block.
>>>> But if the application has a bigger active code footprint that what
>>>> can fit in the code cache, there will be new flushes that will
>>>> increase fragmentation and slowly reduce the usable part of the code
>>>> cache. So I will add functionality for backing off.
>>>>
>>>> //Nils
>>>>
>>>> Schoesser, Andreas skrev 2013-02-01 10:58:
>>>>> Hi Nils,
>>>>>
>>>>> this is IMO a good idea since we experienced the same problem with
>>>>> one of our ports. I think in our case it was not the nmethod sweeper
>>>>> continuously running (there's MinCodeCacheFlushingInterval to only
>>>>> speculatively disconnect nmethods every 30 seconds) but from the
>>>>> compiler loop constantly polling the freelist without the nmethod
>>>>> sweeper making any progress.
>>>>>
>>>>> You also might consider to propagate the "critical" flag down to
>>>>> search_freelist(). Then, a simple address compare would do to keep
>>>>> the end of the code cache free from nmethods:
>>>>> - For non-critical blobs, search_freelist() may only fit free blocks
>>>>> < CodeCache::end() - CodeCacheMinimumFreeSpace
>>>>> - For critical blocks, search_freelist() may fit all free blocks
>>>>>
>>>>> Best regards,
>>>>> Andreas
>>>>>
>>>>> -----Original Message-----
>>>>> From: hotspot-compiler-dev-bounces at openjdk.java.net
>>>>> [mailto:hotspot-compiler-dev-bounces at openjdk.java.net] On Behalf Of
>>>>> Nils Eliasson
>>>>> Sent: Donnerstag, 31. Januar 2013 19:41
>>>>> To: hotspot-compiler-dev at openjdk.java.net
>>>>> Subject: Re: RFR (M): 8006952: Slow VM due to excessive code cache
>>>>> freelist iteration
>>>>>
>>>>> http://cr.openjdk.java.net/~neliasso/8006952/webrev.04/
>>>>>
>>>>> //N
>>>>>
>>>>> Vladimir Kozlov skrev 2013-01-30 21:16:
>>>>>> Nils, you sent webrev for other bug fix.
>>>>>>
>>>>>> Vladimir
>>>>>>
>>>>>> On 1/30/13 7:43 AM, Nils Eliasson wrote:
>>>>>>> Thanks for the link, interesting background.
>>>>>>>
>>>>>>> Even if finding the largest block on the freelist is free, we would
>>>>>>> still have a misbehavior when the free heap block is less than 1,5M
>>>>>>> and
>>>>>>> the flushing begins to create a 1,5M freelist block.
>>>>>>>
>>>>>>> Unallocated memory is a good measurement if have some continuous
>>>>>>> memory
>>>>>>> in reserve for critical blobs. I am thinking of using the 
>>>>>>> thresholds
>>>>>>> against unallocated memory, but forbidding nmethods from using the
>>>>>>> last
>>>>>>> 0,5M (MinimumFreeSpace) from the heap block. That would prevent 
>>>>>>> the VM
>>>>>>> from fragmenting the last continuous memory space, and allow the 
>>>>>>> VM to
>>>>>>> use all of the freelist.
>>>>>>>
>>>>>>> Like this:
>>>>>>> http://cr.openjdk.java.net/~neliasso/8002364/webrev.03
>>>>>>>
>>>>>>> //Nils
>>>>>>>
>>>>>>> Vladimir Kozlov skrev 2013-01-29 20:35:
>>>>>>>> Nils,
>>>>>>>>
>>>>>>>> You are reversing 7025742 fix. I would prefer to keep track of a
>>>>>>>> largest free block during allocation in CodeCache (by using 
>>>>>>>> ordered
>>>>>>>> list or other technique) to avoid scanning the list.
>>>>>>>>
>>>>>>>> Vladimir
>>>>>>>>
>>>>>>>> On 1/29/13 2:56 AM, Nils Eliasson wrote:
>>>>>>>>> Remove continuous free block requirement for code cache 
>>>>>>>>> flushing and
>>>>>>>>> minimum free space.
>>>>>>>>>
>>>>>>>>> This causes a degenerate behavior when the VM are repeatedly
>>>>>>>>> flushing
>>>>>>>>> the code cache trying to free up a continuous 1,5M block. 
>>>>>>>>> Since the
>>>>>>>>> freelist is fragmented a significant part of the code cache is 
>>>>>>>>> kept
>>>>>>>>> unallocated. A significant amount of time is spent traversing the
>>>>>>>>> freelist holding the code cache lock or waiting for other threads
>>>>>>>>> doing
>>>>>>>>> the same.
>>>>>>>>>
>>>>>>>>> I don't see any reason for why a continuous block is required. 
>>>>>>>>> Code
>>>>>>>>> cache flushing threshold (CodeCacheFlushingMinimumFreeSpace) is
>>>>>>>>> default
>>>>>>>>> 1,5M and code cache minimum threshold 
>>>>>>>>> (CodeCacheMinimumFreeSpace) is
>>>>>>>>> 0,5M. Finding such a block on the freelist is often unrealistic,
>>>>>>>>> and has
>>>>>>>>> not any purpose. The largest nmethods are in the order of 
>>>>>>>>> 250k, and
>>>>>>>>> they
>>>>>>>>> are not allocated when code cache is starting to run out. All
>>>>>>>>> critical
>>>>>>>>> adapters are small and will fit easily in the freelist or in the
>>>>>>>>> remaining heap space.
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~neliasso/8006952/webrev.02/
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Nils Eliasson
>>>>
>>>
>

More information about the hotspot-compiler-dev mailing list