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

[Albert]

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