[9] RFR (L): 8015774: Add support for multiple code heaps

[Tobias Hartmann]

Hi Vladimir,

thanks for the review.

On 29.08.2014 19:33, Vladimir Kozlov wrote:
> On 8/29/14 7:10 AM, Tobias Hartmann wrote:
>> Hi Vladimir,
>>
>> thanks for the review.
>>
>> On 29.08.2014 01:13, Vladimir Kozlov wrote:
>>> For the record, SegmentedCodeCache is enabled by default when 
>>> TieredCompilation is enabled and ReservedCodeCacheSize
>>> >= 240 MB. Otherwise it is false by default.
>>
>> Exactly.
>>
>>> arguments.cpp - in set_tiered_flags() swap SegmentedCodeCache 
>>> setting and segments size adjustment - do adjustment
>>> only if SegmentedCodeCache is enabled.
>>
>> Done.
>>
>>> Also I think each flag should be checked and adjusted separately. 
>>> You may bail out (vm_exit_during_initialization) if
>>> sizes do not add up.
>>
>> I think we should only increase the sizes if they are all default. 
>> Otherwise we would for example fail if the user sets
>> the NonMethodCodeHeapSize and the ProfiledCodeHeapSize because the 
>> NonProfiledCodeHeap size is multiplied by 5. What do
>> you think?
>
> But ReservedCodeCacheSize is scaled anyway and you will get sum of 
> sizes != whole size. We need to do something.

I agree. I changed it as you suggested first: The code heap sizes are 
scaled individually and we bail out if the sizes are not consistent with 
ReservedCodeCacheSize.

> BTW the error message for next check should print all sizes, user may 
> not know the default value of some which he did not specified on 
> command line.
>
> (NonMethodCodeHeapSize + NonProfiledCodeHeapSize + 
> ProfiledCodeHeapSize) != ReservedCodeCacheSize)

The error message now prints the sizes in brackets.

>>> And use
>>
>> I think the rest of this sentence is missing :)
>
> And use FLAG_SET_ERGO() when you scale. :)

Done. I also changed the implementation of CodeCache::initialize_heaps() 
accordingly.

>>> Align second line:
>>>
>>> 2461   } else if ((!FLAG_IS_DEFAULT(NonMethodCodeHeapSize) || 
>>> !FLAG_IS_DEFAULT(ProfiledCodeHeapSize) ||
>>> !FLAG_IS_DEFAULT(NonProfiledCodeHeapSize))
>>> 2462       && (NonMethodCodeHeapSize + NonProfiledCodeHeapSize + 
>>> ProfiledCodeHeapSize) != ReservedCodeCacheSize) {
>>
>> Done.
>>
>>> codeCache.cpp - in initialize_heaps() add new methods in C1 and C2 
>>> to return buffer_size they need. Add
>>> assert(SegmentedCodeCache) to this method to show that we call it 
>>> only in such case.
>>
>> Done.
>>
>>> You do adjustment only when all flags are default. But you still 
>>> need to check that you have space in
>>> NonMethodCodeHeap for scratch buffers.
>>
>> I added a the following check:
>>
>>    // Make sure we have enough space for the code buffers
>>    if (NonMethodCodeHeapSize < code_buffers_size) {
>>      vm_exit_during_initialization("Not enough space for code buffers 
>> in CodeCache");
>>    }
>
> I think, you need to take into account min_code_cache_size as in 
> arguments.cpp:
>  uint min_code_cache_size = (CodeCacheMinimumUseSpace DEBUG_ONLY(* 3)) 
> + CodeCacheMinimumFreeSpace;
>
> if (NonMethodCodeHeapSize < (min_code_cache_size+code_buffers_size)) {

True, I changed it.

> I would be nice if this code in initialize_heaps() could be moved 
> called during arguments parsing if we could get number of compiler 
> threads there. But I understand that we can't do that until 
> compilation policy is set :(

Yes, this is not possible because we need to know the number of C1/C2 
compiler threads.

New webrev: http://cr.openjdk.java.net/~thartmann/8015774/webrev.05/

Thanks,
Tobias

>
>>
>>> codeCache.hpp - comment alignment:
>>> +     // Creates a new heap with the given name and size, containing 
>>> CodeBlobs of the given type
>>> !   static void add_heap(ReservedSpace rs, const char* name, size_t 
>>> size_initial, int code_blob_type);
>>
>> Done.
>>
>>> nmethod.cpp - in new() can we mark nmethod allocation critical only 
>>> when SegmentedCodeCache is enabled?
>>
>> Yes, that's what we do with:
>>
>> 809   bool is_critical = SegmentedCodeCache;
>>
>> Or what are you referring to?
>
> Somehow I missed that SegmentedCodeCache is used already. It is fine 
> then.
>
> Thanks,
> Vladimir
>
>>
>> New webrev: http://cr.openjdk.java.net/~thartmann/8015774/webrev.04
>>
>> Thanks,
>> Tobias
>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 8/28/14 5:09 AM, Tobias Hartmann wrote:
>>>> Hi,
>>>>
>>>> the segmented code cache JEP is now targeted. Please review the final
>>>> implementation before integration. The previous RFR, including a short
>>>> description, can be found here [1].
>>>>
>>>> JEP: https://bugs.openjdk.java.net/browse/JDK-8043304
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8015774
>>>> Implementation: 
>>>> http://cr.openjdk.java.net/~thartmann/8015774/webrev.03/
>>>> JDK-Test fix:
>>>> http://cr.openjdk.java.net/~thartmann/8015774_jdk_test/webrev.00/
>>>>
>>>> Changes since the last review:
>>>> - Merged with other changes (for example, G1 class unloading 
>>>> changes [2])
>>>> - Fixed some minor bugs that showed up during testing
>>>> - Refactoring of 'NMethodIterator' and CodeCache implementation
>>>> - Non-method CodeHeap size increased to 5 MB
>>>> - Fallback solution: Store non-method code in the non-profiled code 
>>>> heap
>>>> if there is not enough space in the non-method code heap (see
>>>> 'CodeCache::allocate')
>>>>
>>>> Additional testing:
>>>> - BigApps (Weblogic, Dacapo, runThese, Kitchensink)
>>>> - Compiler and GC nightlies
>>>> - jtreg tests
>>>> - VM (NSK) Testbase
>>>> - More performance testing (results attached to the bug)
>>>>
>>>> Thanks,
>>>> Tobias
>>>>
>>>> [1]
>>>> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2014-April/014098.html 
>>>>
>>>>
>>>> [2] https://bugs.openjdk.java.net/browse/JDK-8049421
>>