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

[Vladimir Kozlov]

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