[9] RFR(S): 8034839: jvm hangs with gc/gctests/LoadUnloadGC test

Albert albert.noll at oracle.com
Thu Feb 27 10:44:49 PST 2014 

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.

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

More information about the hotspot-dev mailing list