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

[Vitaly Davidovich]

Hi Albert,

There's currently one instantiation of the hashtable that looks like this:

new GenericHashtable<DependencySignature, ResourceObj>(11027);

With size as template, it would be:
new GenericHashtable<DependencySignature, ResourceObj, 11027>();

So the size is specified in the same place (in terms of reading the code).
Your call though, this was just a suggestion.

Power of 2 hashtable sizes are an issue with poor quality hash functions
(I.e. ones that don't differ in low order bits), but if the hash function
is decent it shouldn't be an issue.  Prime size is used to mitigate that,
but there are other techniques that allow 2^n sizing (e.g. supplementary
hash function applied on top of hash code coming from original key
object).  FWIW, JDK's HashMap is 2^n sized with supplementary hashing.

Again, just a suggestion - it probably won't make a substantial difference
here.

Thanks

Sent from my phone
On Feb 25, 2014 8:06 AM, "Albert" <albert.noll at oracle.com> 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
>>>>
>>>>
>