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

[Vladimir Kozlov]

Albert,

It looks good to me.

Thanks,
Vladimir

On 2/25/14 10:56 PM, 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
>>>>>>
>>>
>