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

Thu Sep 4 10:38:31 UTC 2014

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