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

[Albert]

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