[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-dev
Subject: Re: [9] RFR(S): 8034839: jvm hangs with gc/gctests/LoadUnloadGC test
From: Coleen Phillimore <coleen.phillimore () oracle ! com>
Date: 2014-02-27 18:50:51
Message-ID: 530F890B.9010800 () oracle ! com
[Download RAW message or body]
On 2/27/14 1:44 PM, Albert wrote:
> Hi,
>
> it seems that the proposed change to ResourceHashtable does not imply
> changes in
> src/share/vm/classfile/defaultMethods.cpp . I think the proposed
> change affects only
> performance of dependency checking.
Okay. That's good then.
Coleen
>
> Best,
> Albert
>
> On 02/27/2014 07:20 PM, Vladimir Kozlov wrote:
>> > in src/share/vm/classfile/defaultMethods.cpp. There are nsk defmeth
>> > tests that will exercise that code.
>>
>> Coleen, can we measure performance with them or they checks only
>> correctness?
>>
>> I am for improving performance of ResourceHashtable.
>>
>> Thanks,
>> Vladimir
>>
>> On 2/27/14 8:53 AM, Coleen Phillimore wrote:
>>>
>>> Albert, Thank you for doing this investigation. I'm sorry I didn't
>>> notice it sooner.
>>>
>>> On 2/27/14 9:25 AM, Albert wrote:
>>>> Hi,
>>>>
>>>> thanks for your feedback. That hashtable (ResourceHashtable) was
>>>> exactly what I was looking for.
>>>> Unfortunately, I did not know it existed...
>>>>
>>>> I implemented a different version of 8034839 that uses
>>>> ResourceHashtable and compared
>>>> the results against the version of the newly added hashtable
>>>> (GenericHashtable). The performance
>>>> is almost the same.
>>>> GenericHashtable is little faster, since ResourceHashtable requires 2
>>>> lookups (1 to check
>>>> if the element is in the hashtable and the 2nd when adding the
>>>> element) to add a non-existing item
>>>> to the hashtable.
>>>>
>>>> We could change ResourceHashtable such that it returns NULL if the
>>>> item is added to the hashtable and
>>>> a pointer to the previously stored value, if the item already existed.
>>>> The Java hashtable has the same
>>>> signature for put(). What do you think?
>>>> We can also leave ResourceHashtable as it is, I do not insist on this
>>>> change.
>>>
>>> That seems fine to do if it improves performance. There is only one
>>> other use of this hashtable now that would benefit from this change
>>> also
>>> in src/share/vm/classfile/defaultMethods.cpp. There are nsk defmeth
>>> tests that will exercise that code.
>>>
>>> Thanks,
>>> Coleen
>>>>
>>>> In any case, we should remove GenericHashtable. I filed
>>>> https://bugs.openjdk.java.net/browse/JDK-8035946 .
>>>> If we decide to do the suggested change in ResourceHashtable, I guess
>>>> it would be best to send the RFR
>>>> to hotspot-dev ?
>>>>
>>>> Best,
>>>> Albert
>>>>
>>>>
>>>> On 02/26/2014 08:57 PM, Vladimir Kozlov wrote:
>>>>> I have asked Albert to try ResourceHashtable and if it gives us the
>>>>> same results we may remove the new one. I think the confusion come
>>>>> from the fact that it is in a different file. We thought all
>>>>> hashtables are in hashtable.hpp.
>>>>>
>>>>> Regards,
>>>>> Vladimir
>>>>>
>>>>> On 2/26/14 11:51 AM, Coleen Phillimore wrote:
>>>>>> On 2/26/2014 2:46 PM, Coleen Phillimore wrote:
>>>>>>> On 2/26/2014 2:14 PM, Vladimir Kozlov wrote:
>>>>>>>> Too late Coleen, it is pushed already. It was on review on
>>>>>>>> hotspot-dev since last week.
>>>>>>>> I did not know about ResourceHashtable. Why it is in different
>>>>>>>> file?
>>>>>>>> I will let Albert investigate the possibility of use
>>>>>>>> ResourceHashtable.
>>>>>>>
>>>>>>> Bummer, I tagged it but didn't get to it. I was on vacation this
>>>>>>> week. There are a few more ad-hoc hashtables too in class file
>>>>>>> parser that I thought we could use Keith's hashtable for also.
>>>>>>> It's in
>>>>>>> the utilities directory...
>>>>>>
>>>>>> Sorry, to be clear, I think the runtime group should do the
>>>>>> classfile
>>>>>> parser ones...
>>>>>> Coleen
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Coleen
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Vladimir
>>>>>>>>
>>>>>>>> On 2/26/14 10:32 AM, Coleen Phillimore wrote:
>>>>>>>>>
>>>>>>>>> Albert,
>>>>>>>>>
>>>>>>>>> This looks like nice work but it seems to add another completely
>>>>>>>>> different hashtable to the system. I was expecting this change
>>>>>>>>> to be
>>>>>>>>> generalizing BasicHashTable to allow resource allocation but it
>>>>>>>>> doesn't
>>>>>>>>> do that. It doesn't seem to make this easier to do either
>>>>>>>>> because it
>>>>>>>>> uses different names and doesn't have the hashing and other
>>>>>>>>> functionality that the general case needs. Was there a plan
>>>>>>>>> to do
>>>>>>>>> this?
>>>>>>>>>
>>>>>>>>> Keith McGuigan added a resource allocated hashtable also in
>>>>>>>>> src/share/vm/utilities/resourceHash.hpp. Can this not serve
>>>>>>>>> your
>>>>>>>>> needs?
>>>>>>>>>
>>>>>>>>> I think we shouldn't add a generalized class like this if it
>>>>>>>>> doesn't or
>>>>>>>>> can't support the general case - ie. the C_heap hashtable uses in
>>>>>>>>> the
>>>>>>>>> JVM, specificially for the symbol, string, and system
>>>>>>>>> dictionaries.
>>>>>>>>> It's
>>>>>>>>> just a lot of extra code and complex template parameters.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Coleen
>>>>>>>>>
>>>>>>>>> On 2/26/2014 1:56 AM, Albert wrote:
>>>>>>>>>> Hi Vladimir,
>>>>>>>>>>
>>>>>>>>>> I agree that your version is more clear. Here is the updated
>>>>>>>>>> webrev:
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~anoll/8034939/webrev.02/
>>>>>>>>>>
>>>>>>>>>> Best,
>>>>>>>>>> Albert
>>>>>>>>>>
>>>>>>>>>> On 02/25/2014 09:17 PM, Vladimir Kozlov wrote:
>>>>>>>>>>> Can you align methods bodies?:
>>>>>>>>>>>
>>>>>>>>>>> + T* next() const { return _next; }
>>>>>>>>>>> + T* prev() const { return _prev; }
>>>>>>>>>>> + void set_next(T* item) { _next = item; }
>>>>>>>>>>> + void set_prev(T* item) { _prev = item; }
>>>>>>>>>>>
>>>>>>>>>>> I am not sure that your methods factoring will produce
>>>>>>>>>>> better code
>>>>>>>>>>> with all C++ compilers. You added switch which you assume will
>>>>>>>>>>> constant-fold but it is not guarantee.
>>>>>>>>>>>
>>>>>>>>>>> When I asked about refactoring I meant something a little
>>>>>>>>>>> simpler.
>>>>>>>>>>> To have inlined index(item) method in GenericHashtable:
>>>>>>>>>>>
>>>>>>>>>>> int index(T* item)) { assert(item != NULL, "missing null
>>>>>>>>>>> check");
>>>>>>>>>>> return item->key() % size(); }
>>>>>>>>>>>
>>>>>>>>>>> and have only your contains_impl() as common method
>>>>>>>>>>>
>>>>>>>>>>> template<class T, class F> T* GenericHashtable<T,
>>>>>>>>>>> F>::contains(T*
>>>>>>>>>>> item) {
>>>>>>>>>>> if (item != NULL) {
>>>>>>>>>>> int idx = index(item);
>>>>>>>>>>> return contains_impl(item, idx);
>>>>>>>>>>> }
>>>>>>>>>>> return NULL;
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> template<class T, class F> bool GenericHashtable<T, F>::add(T*
>>>>>>>>>>> item) {
>>>>>>>>>>> if (item != NULL) {
>>>>>>>>>>> int idx = index(item);
>>>>>>>>>>> T* found_item = contains_impl(item, idx);
>>>>>>>>>>> if (found_item == NULL) {
>>>>>>>>>>> ... // code from add_impl() after (!contains) check
>>>>>>>>>>> return true;
>>>>>>>>>>> }
>>>>>>>>>>> }
>>>>>>>>>>> return false;
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> template<class T, class F> T* GenericHashtable<T, F>::remove(T*
>>>>>>>>>>> item) {
>>>>>>>>>>> if (item != NULL) {
>>>>>>>>>>> int idx = index(item);
>>>>>>>>>>> T* found_item = contains_impl(item, idx);
>>>>>>>>>>> if (found_item != NULL) {
>>>>>>>>>>> ... // code from remove_impl() after (contains) check
>>>>>>>>>>> return found_item;
>>>>>>>>>>> }
>>>>>>>>>>> }
>>>>>>>>>>> return NULL;
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> I think it is more clear.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Vladimir
>>>>>>>>>>>
>>>>>>>>>>> On 2/25/14 5:04 AM, Albert wrote:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> Vladimir, Christian, Vitaly, thanks for looking at this and
>>>>>>>>>>>> your
>>>>>>>>>>>> feedback.
>>>>>>>>>>>>
>>>>>>>>>>>> @Vladimir:
>>>>>>>>>>>> No, this change is based on the version of 7194669. However,
>>>>>>>>>>>> the diff to before 7194669 are mainly code refactorings, which
>>>>>>>>>>>> make the
>>>>>>>>>>>> code more readable (for me).
>>>>>>>>>>>>
>>>>>>>>>>>> I have changed the parameter name, (bool C_heap = false),
>>>>>>>>>>>> adapted
>>>>>>>>>>>> the
>>>>>>>>>>>> 'add' function
>>>>>>>>>>>> according to your suggestion, and implemented the hashtable
>>>>>>>>>>>> destructor
>>>>>>>>>>>> as well as the
>>>>>>>>>>>> remove function.
>>>>>>>>>>>>
>>>>>>>>>>>> @Christian:
>>>>>>>>>>>> This for noticing this inconsistency. I fixed the parameter
>>>>>>>>>>>> names
>>>>>>>>>>>>
>>>>>>>>>>>> @Vitaly:
>>>>>>>>>>>> I would prefer to leave the size parameter as it is now.
>>>>>>>>>>>> While we
>>>>>>>>>>>> would
>>>>>>>>>>>> save some instructions,
>>>>>>>>>>>> I think that specifying the size of the hashtable where it is
>>>>>>>>>>>> used
>>>>>>>>>>>> makes
>>>>>>>>>>>> the code more readable.
>>>>>>>>>>>>
>>>>>>>>>>>> Shouldn't we, in general, try to avoid hash table sizes that
>>>>>>>>>>>> are an
>>>>>>>>>>>> exact power of 2?
>>>>>>>>>>>>
>>>>>>>>>>>> Here is the new webrev:
>>>>>>>>>>>> http://cr.openjdk.java.net/~anoll/8034939/webrev.01/
>>>>>>>>>>>>
>>>>>>>>>>>> Best,
>>>>>>>>>>>> Albert
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 02/21/2014 11:54 PM, Christian Thalinger wrote:
>>>>>>>>>>>>> On Feb 21, 2014, at 11:27 AM, Vladimir Kozlov
>>>>>>>>>>>>> <vladimir.kozlov@oracle.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Lets discuss it on hotspot-dev.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Note the current hashtable allocates only in c_heap. Albert
>>>>>>>>>>>>>> added
>>>>>>>>>>>>>> hashtable which can allocate in thread local resource
>>>>>>>>>>>>>> area for
>>>>>>>>>>>>>> temporary table and c_heap for long live table.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Albert,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> So you restored code in dependencies.?pp to one before
>>>>>>>>>>>>>> 7194669
>>>>>>>>>>>>>> fix.
>>>>>>>>>>>>>> Right?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I think you need to follow GrowableArray example to name
>>>>>>>>>>>>>> parameter
>>>>>>>>>>>>>> "bool C_heap = false" instead of "bool resource_mark". It
>>>>>>>>>>>>>> should be
>>>>>>>>>>>>>> saved in a field because you need to free c_heap in
>>>>>>>>>>>>>> destructor if
>>>>>>>>>>>>>> C-heap is used:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ~GrowableArray() { if (on_C_heap())
>>>>>>>>>>>>>> clear_and_deallocate(); }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Also I think you should avoid call to contains(item) in
>>>>>>>>>>>>>> add() to
>>>>>>>>>>>>>> avoid doing the same thing twice.
>>>>>>>>>>>>> …and you should stick to either item or element:
>>>>>>>>>>>>>
>>>>>>>>>>>>> + template<class T, class F> bool GenericHashtable<T,
>>>>>>>>>>>>> F>::add(T*
>>>>>>>>>>>>> item) {
>>>>>>>>>>>>> + template<class T, class F> bool GenericHashtable<T,
>>>>>>>>>>>>> F>::contains(T*
>>>>>>>>>>>>> element) {
>>>>>>>>>>>>>
>>>>>>>>>>>>>> You should implement remove().
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> Vladimir
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 2/21/14 12:04 AM, Albert wrote:
>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> could I get reviews for this small patch?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8034839
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Problem: The problem is that the patch (7194669) - which
>>>>>>>>>>>>>>> was
>>>>>>>>>>>>>>> supposed to
>>>>>>>>>>>>>>> speed-up dependency checking
>>>>>>>>>>>>>>> causes a performance regression. The reason
>>>>>>>>>>>>>>> for the
>>>>>>>>>>>>>>> performance regression is that most dependencies
>>>>>>>>>>>>>>> are unique, so we have the overhead of
>>>>>>>>>>>>>>> determining if
>>>>>>>>>>>>>>> the dependency is already checked plus the
>>>>>>>>>>>>>>> overhead of dependency checking. The
>>>>>>>>>>>>>>> overhead of
>>>>>>>>>>>>>>> searching is significant, since we perform
>>>>>>>>>>>>>>> a linear search on 6000+ items each time.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Solution: Use a hashtable instead of linear search to
>>>>>>>>>>>>>>> lookup
>>>>>>>>>>>>>>> already
>>>>>>>>>>>>>>> checked dependencies. The new hashtable
>>>>>>>>>>>>>>> is very rudimentary. It provides only the
>>>>>>>>>>>>>>> required
>>>>>>>>>>>>>>> functionality to solve this bug. However, the functionality
>>>>>>>>>>>>>>> can be easily extended as needed.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Testing: jprt, failing test case, nashorn. The failing test
>>>>>>>>>>>>>>> case
>>>>>>>>>>>>>>> completes in approx. the same time as before 7194669.
>>>>>>>>>>>>>>> For nashorn + Octane, this patch yields the
>>>>>>>>>>>>>>> following
>>>>>>>>>>>>>>> times spent for dependency checking:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> with this patch: 844s
>>>>>>>>>>>>>>> 7194669: 1080s
>>>>>>>>>>>>>>> before 7194669: 5223s
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> webrev:
>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~anoll/8034939/webrev.00/
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>> Albert
>>>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic