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

[Vladimir Kozlov]

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