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

Fri Sep 5 08:48:33 UTC 2014

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
>>>>>>
>>>>
>>