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

Wed Jun 4 06:42:07 UTC 2014

Thank you, Vladimir.

Best,
Tobias

On 04.06.2014 08:37, Vladimir Kozlov wrote:
> Looks good.
>
> Thanks,
> Vladimir
>
> On 6/3/14 11:04 PM, Tobias Hartmann wrote:
>> Hi Vladimir,
>>
>> On 03.06.2014 20:42, Vladimir Kozlov wrote:
>>> On 6/3/14 12:44 AM, Tobias Hartmann wrote:
>>>> Hi Vladimir,
>>>>
>>>> thanks for the review.
>>>>
>>>>> Much better. Please, use at least 'iter' name instead of 'it' 
>>>>> which is confused with English word.
>>>>> Also it may be better to call iterator NMethodIterator (with N).
>>>>
>>>> Done.
>>>>
>>>>> Use FLAG_SET_ERGO() (ergonomic) macro for SegmentedCodeCache setting.
>>>
>>> I still see:
>>>
>>> +     FLAG_SET_DEFAULT(SegmentedCodeCache, true);
>>
>> Sorry, I accidentally reverted it.
>>
>> New webrev: http://cr.openjdk.java.net/~thartmann/8015774/webrev.02/
>>
>> Thanks,
>> Tobias
>>
>>>
>>> Vladimir
>>>
>>>>
>>>> Done.
>>>>
>>>>> In CodeCache::allocate() you reversed your fix for 8029343: 
>>>>> "CodeCache::allocate increments '_number_of_blobs' even if
>>>>> allocation fails."
>>>>
>>>> That got lost during merging. Thanks!
>>>>
>>>> New webrev: http://cr.openjdk.java.net/~thartmann/8015774/webrev.01/
>>>>
>>>> Best,
>>>> Tobias
>>>>
>>>>> thanks,
>>>>> Vladimir
>>>>>
>>>>> On 6/2/14 4:06 AM, Tobias Hartmann wrote:
>>>>>> Hi Vladimir,
>>>>>>
>>>>>> thanks for the feedback.
>>>>>>
>>>>>> On 17.04.2014 20:54, Vladimir Kozlov wrote:
>>>>>>> The iterator look nice but I am not sure all our C++ compilers will
>>>>>>> optimize out the allocation in end().
>>>>>>>
>>>>>>> I prefer our stile:
>>>>>>>
>>>>>>>    for (CodeCacheMethodIterator it(); !it.end(); it.next()) {
>>>>>>>      nmethod* current = it.nmethod();
>>>>>>
>>>>>> I re-implemented the iterator (now called MethodIterator) to use the
>>>>>> following style which is also used by RelocIterator:
>>>>>>
>>>>>> MethodIterator it;
>>>>>> while(it.next()) {
>>>>>>      // use it.method()
>>>>>> }
>>>>>>
>>>>>> It is now used by the code cache implementation as well as by the
>>>>>> sweeper and other client code.
>>>>>>
>>>>>>> sweeper.cpp:
>>>>>>>
>>>>>>> *_current is not clear statement, as above I prefer explicit
>>>>>>> _current.nmethod(). And _current != CodeCacheMethodIterator::end()
>>>>>>> would be !_current.end()
>>>>>>>
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>> Further, I added a flag "SegmentedCodeCache" to completely
>>>>>> enable/disable the segmentation of the code cache. Per default it is
>>>>>> only enabled with TieredCompilation and a ReservedCodeCacheSize 
>>>>>> >= 240
>>>>>> MB. This setting makes sure we do not have any problems with 
>>>>>> small code
>>>>>> cache sizes on embedded systems.
>>>>>>
>>>>>> New webrev: http://cr.openjdk.java.net/~thartmann/8015774/webrev.00/
>>>>>>
>>>>>> Thanks,
>>>>>> Tobias
>>>>>>
>>>>>>> Implementation of is_full() looks fine.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Vladimir
>>>>>>>
>>>>>>> On 4/17/14 2:29 AM, Tobias Hartmann wrote:
>>>>>>>> Vladimir, thanks for the review.
>>>>>>>>
>>>>>>>> Updated webrev: 
>>>>>>>> http://cr.openjdk.java.net/~anoll/8015774/webrev.06/
>>>>>>>>
>>>>>>>> On 04/14/2014 11:38 PM, Vladimir Kozlov wrote:
>>>>>>>>> One thing is bothering me is codecache structure expose to 
>>>>>>>>> outside
>>>>>>>>> code (loop in compileBroker.cpp, nmethod.cpp and
>>>>>>>>> jvmtiCodeBlobEvents.cpp).
>>>>>>>>> It would be nice to hide it.
>>>>>>>>
>>>>>>>> I introduced a CodeCacheMethodIterator that iterates over all 
>>>>>>>> methods in
>>>>>>>> the code cache. It hides the code cache internal structures and 
>>>>>>>> is now
>>>>>>>> used in sweeper.cpp, nmethod.cpp and jvmtiCodeBlobEvents.cpp. In
>>>>>>>> compileBroker.cpp a new version of CodeCache::is_full is used to
>>>>>>>> determine if the code cache is full.
>>>>>>>>
>>>>>>>> I further added a new trace type CODEBLOBTYPE to tracetypes.xml to
>>>>>>>> display the code heap that is full in the CodeCacheFull JFR event.
>>>>>>>>
>>>>>>>>> Before pushing it we need to test. SA, dtrace and other 
>>>>>>>>> components are
>>>>>>>>> affected by these changes. Ask sqe and performance group for 
>>>>>>>>> help.
>>>>>>>>
>>>>>>>> I will do that next.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Tobias
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Vladimir
>>>>>>>>>
>>>>>>>>> On 4/14/14 6:05 AM, Tobias Hartmann wrote:
>>>>>>>>>> Hi Volker,
>>>>>>>>>>
>>>>>>>>>> please see comments inline.
>>>>>>>>>>
>>>>>>>>>> On 04/14/2014 02:54 PM, Volker Simonis wrote:
>>>>>>>>>>> Hi Tobias,
>>>>>>>>>>>
>>>>>>>>>>> is this change intended for jdk8 or 9?
>>>>>>>>>>
>>>>>>>>>> the change is intended for JDK 9. The webrev is based on the 
>>>>>>>>>> JDK 9
>>>>>>>>>> repository (http://hg.openjdk.java.net/jdk9/hs-comp/hotspot)
>>>>>>>>>> changeset 6280.
>>>>>>>>>>
>>>>>>>>>>>   In any case, I saw that your
>>>>>>>>>>> webrev is based on the hsx repository which I thought should 
>>>>>>>>>>> not be
>>>>>>>>>>> used any more.
>>>>>>>>>>
>>>>>>>>>> Why do you think that it is based on hsx? The earlier 
>>>>>>>>>> versions 00 - 03
>>>>>>>>>> actually were, because I already started developing the patch 
>>>>>>>>>> last
>>>>>>>>>> year,
>>>>>>>>>> but the newest one is based on the JDK 9 repository.
>>>>>>>>>>
>>>>>>>>>> As Roland noticed, the default code heap sizes for arm/ppc 
>>>>>>>>>> were wrong
>>>>>>>>>> (because tiered compilation is supported). The updated webrev 
>>>>>>>>>> can be
>>>>>>>>>> found at:
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~anoll/8015774/webrev.05/
>>>>>>>>>> <http://cr.openjdk.java.net/%7Eanoll/8015774/webrev.05/>
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Tobias
>>>>>>>>>>
>>>>>>>>>>> Could you please rebase your patch on jdk9 and also
>>>>>>>>>>> update the corresponding ppc64 files in the webrev (as far 
>>>>>>>>>>> as I can
>>>>>>>>>>> see, that's only src/cpu/ppc/vm/c2_globals_ppc.hpp). I'd 
>>>>>>>>>>> like to test
>>>>>>>>>>> this on Linux/PPC64 and AIX as well.
>>>>>>>>>>>
>>>>>>>>>>> Thank you and best regards,
>>>>>>>>>>> Volker
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Mon, Apr 14, 2014 at 1:56 PM, Tobias Hartmann
>>>>>>>>>>> <Tobias.Hartmann at oracle.com>  wrote:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> please review the new version of the following patch that adds
>>>>>>>>>>>> support for
>>>>>>>>>>>> multiple code heaps to the code cache.
>>>>>>>>>>>>
>>>>>>>>>>>> Bug:https://bugs.openjdk.java.net/browse/JDK-8015774
>>>>>>>>>>>> Webrev:http://cr.openjdk.java.net/~anoll/8015774/webrev.04/
>>>>>>>>>>>>
>>>>>>>>>>>> Short description:
>>>>>>>>>>>> This change implements support for multiple code heaps in 
>>>>>>>>>>>> the code
>>>>>>>>>>>> cache.
>>>>>>>>>>>> The interface of the code cache was changed accordingly and
>>>>>>>>>>>> references from
>>>>>>>>>>>> other components of the VM were adapted. This includes the 
>>>>>>>>>>>> indirect
>>>>>>>>>>>> references from:
>>>>>>>>>>>> - the Serviceability Agent: vmStructs and the Java code cache
>>>>>>>>>>>> interface
>>>>>>>>>>>> (sun.jvm.hotspot.code.CodeCache)
>>>>>>>>>>>> - the dtrace ustack helper script (jhelper.d)
>>>>>>>>>>>> - the pstack support library libjvm_db.c
>>>>>>>>>>>>
>>>>>>>>>>>> Currently the code cache contains the following three code 
>>>>>>>>>>>> heaps
>>>>>>>>>>>> each of
>>>>>>>>>>>> which contains CodeBlobs of a specific type:
>>>>>>>>>>>> - Non-Profiled methods: nmethods that are not profiled and 
>>>>>>>>>>>> native
>>>>>>>>>>>> methods
>>>>>>>>>>>> - Profiled methods: nmethods that are profiled
>>>>>>>>>>>> - Non-methods: Non-methods like buffers and adapters
>>>>>>>>>>>>
>>>>>>>>>>>> By default the non-method code heap uses 3 MB plus 
>>>>>>>>>>>> additional space
>>>>>>>>>>>> for the
>>>>>>>>>>>> compiler buffers that is dependent on the number of compiler
>>>>>>>>>>>> threads (see
>>>>>>>>>>>> CodeCache::initialize_heaps). The remaining code cache 
>>>>>>>>>>>> space is
>>>>>>>>>>>> distributed
>>>>>>>>>>>> equally among the non-profiled and the profiled code heaps.
>>>>>>>>>>>>
>>>>>>>>>>>> Tested:
>>>>>>>>>>>> JPRT, SPECjvm2008, SPECjbb2005, SPECjbb2013, Octane + Nashorn
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Tobias
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> -------- Original Message --------
>>>>>>>>>>>> Subject: Re: RFR (L): 8015774: Add support for multiple 
>>>>>>>>>>>> code heaps
>>>>>>>>>>>> Date: Mon, 21 Oct 2013 17:47:48 +0200
>>>>>>>>>>>> From: Albert Noll<albert.noll at oracle.com>
>>>>>>>>>>>> To: Azeem Jiva<azeem.jiva at oracle.com>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> That is fine. As we realized the last two weeks, there is 
>>>>>>>>>>>> more work
>>>>>>>>>>>> to be
>>>>>>>>>>>> done to make multiple code heaps work effectively.
>>>>>>>>>>>>
>>>>>>>>>>>> Albert
>>>>>>>>>>>>
>>>>>>>>>>>> Von meinem iPhone gesendet
>>>>>>>>>>>>
>>>>>>>>>>>> Am 21.10.2013 um 17:36 schrieb Azeem 
>>>>>>>>>>>> Jiva<azeem.jiva at oracle.com>:
>>>>>>>>>>>>
>>>>>>>>>>>> I still think we should hold off for JDK8u20
>>>>>>>>>>>>
>>>>>>>>>>>> -- 
>>>>>>>>>>>> Azeem Jiva
>>>>>>>>>>>> @javawithjiva
>>>>>>>>>>>>
>>>>>>>>>>>> On Oct 21, 2013, at 8:33 AM, Albert 
>>>>>>>>>>>> Noll<albert.noll at oracle.com>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Von meinem iPhone gesendet
>>>>>>>>>>>>
>>>>>>>>>>>> Anfang der weitergeleiteten E‑Mail:
>>>>>>>>>>>>
>>>>>>>>>>>> Von: Vladimir Kozlov<vladimir.kozlov at oracle.com>
>>>>>>>>>>>> Datum: 11. Oktober 2013 19:38:07 MESZ
>>>>>>>>>>>> An:hotspot-compiler-dev at openjdk.java.net
>>>>>>>>>>>> Betreff: Re: RFR (L): 8015774: Add support for multiple 
>>>>>>>>>>>> code heaps
>>>>>>>>>>>>
>>>>>>>>>>>> This looks acceptable.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Vladimir
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>