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

[Tobias Hartmann]

Thank you, Vladimir.

Best,
Tobias

On 05.09.2014 18:10, Vladimir Kozlov wrote:
> Looks good.
>
> Thanks,
> Vladimir
>
> On 9/5/14 1:48 AM, Tobias Hartmann wrote:
>> Hi Vladimir,
>>
>> thanks again for the review.
>>
>> On 04.09.2014 20:49, Vladimir Kozlov wrote:
>>> The test misses @run command.
>>
>> I thought it is not necessary if we do not specify any VM options. I 
>> added it.
>>
>>> Why you get ""TieredCompilation is disabled in this release."? 
>>> Client VM?
>>
>> Yes, with a client VM. On a 32 bit machine with a 32 bit build of the 
>> VM we may have a client version by default.
>>
>>> What happens if we run with TieredStopAtLevel=1?
>>
>> By now, we use all heaps but I changed 'CodeCache::heap_available' to 
>> not use the profiled code heap in this case. I
>> added some more checks and comments to the test. Thanks for catching 
>> this!
>>
>> New webrev: http://cr.openjdk.java.net/~thartmann/8015774/webrev.07/
>>
>> Thanks,
>> Tobias
>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 9/4/14 3:38 AM, Tobias Hartmann wrote:
>>>> Thank you, Vladimir.
>>>>
>>>> I added a test that checks the result of segmented code cache 
>>>> related VM
>>>> options.
>>>>
>>>> New webrev: http://cr.openjdk.java.net/~thartmann/8015774/webrev.06/
>>>>
>>>> Can I get a second review please?
>>>>
>>>> Best,
>>>> Tobias
>>>>
>>>> On 04.09.2014 00:45, Vladimir Kozlov wrote:
>>>>> Looks good. You need second review.
>>>>>
>>>>> And, please, add a WB test which verifies a reaction on flags 
>>>>> settings
>>>>> similar to what test/compiler/codecache/CheckUpperLimit.java does.
>>>>> Both positive (SegmentedCodeCache is enabled) and negative (sizes 
>>>>> does
>>>>> not match or ReservedCodeCacheSize is small) and etc.
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>> On 9/3/14 4:10 AM, Tobias Hartmann wrote:
>>>>>> 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
>>>>>>>>
>>>>>>
>>>>
>>