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

[Coleen Phillimore]

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