[9] RFR(S): 8034839: jvm hangs with gc/gctests/LoadUnloadGC test
Albert
albert.noll at oracle.com
Thu Feb 27 06:25:02 PST 2014
Hi,
thanks for your feedback. That hashtable (ResourceHashtable) was exactly
what I was looking for.
Unfortunately, I did not know it existed...
I implemented a different version of 8034839 that uses ResourceHashtable
and compared
the results against the version of the newly added hashtable
(GenericHashtable). The performance
is almost the same.
GenericHashtable is little faster, since ResourceHashtable requires 2
lookups (1 to check
if the element is in the hashtable and the 2nd when adding the element)
to add a non-existing item
to the hashtable.
We could change ResourceHashtable such that it returns NULL if the item
is added to the hashtable and
a pointer to the previously stored value, if the item already existed.
The Java hashtable has the same
signature for put(). What do you think?
We can also leave ResourceHashtable as it is, I do not insist on this
change.
In any case, we should remove GenericHashtable. I filed
https://bugs.openjdk.java.net/browse/JDK-8035946 .
If we decide to do the suggested change in ResourceHashtable, I guess it
would be best to send the RFR
to hotspot-dev ?
Best,
Albert
On 02/26/2014 08:57 PM, Vladimir Kozlov wrote:
> I have asked Albert to try ResourceHashtable and if it gives us the
> same results we may remove the new one. I think the confusion come
> from the fact that it is in a different file. We thought all
> hashtables are in hashtable.hpp.
>
> Regards,
> Vladimir
>
> On 2/26/14 11:51 AM, Coleen Phillimore wrote:
>> On 2/26/2014 2:46 PM, Coleen Phillimore wrote:
>>> On 2/26/2014 2:14 PM, Vladimir Kozlov wrote:
>>>> Too late Coleen, it is pushed already. It was on review on
>>>> hotspot-dev since last week.
>>>> I did not know about ResourceHashtable. Why it is in different file?
>>>> I will let Albert investigate the possibility of use
>>>> ResourceHashtable.
>>>
>>> Bummer, I tagged it but didn't get to it. I was on vacation this
>>> week. There are a few more ad-hoc hashtables too in class file
>>> parser that I thought we could use Keith's hashtable for also. It's in
>>> the utilities directory...
>>
>> Sorry, to be clear, I think the runtime group should do the classfile
>> parser ones...
>> Coleen
>>
>>>
>>> Thanks,
>>> Coleen
>>>
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 2/26/14 10:32 AM, Coleen Phillimore wrote:
>>>>>
>>>>> Albert,
>>>>>
>>>>> This looks like nice work but it seems to add another completely
>>>>> different hashtable to the system. I was expecting this change
>>>>> to be
>>>>> generalizing BasicHashTable to allow resource allocation but it
>>>>> doesn't
>>>>> do that. It doesn't seem to make this easier to do either
>>>>> because it
>>>>> uses different names and doesn't have the hashing and other
>>>>> functionality that the general case needs. Was there a plan to do
>>>>> this?
>>>>>
>>>>> Keith McGuigan added a resource allocated hashtable also in
>>>>> src/share/vm/utilities/resourceHash.hpp. Can this not serve your
>>>>> needs?
>>>>>
>>>>> I think we shouldn't add a generalized class like this if it
>>>>> doesn't or
>>>>> can't support the general case - ie. the C_heap hashtable uses in the
>>>>> JVM, specificially for the symbol, string, and system dictionaries.
>>>>> It's
>>>>> just a lot of extra code and complex template parameters.
>>>>>
>>>>> Thanks,
>>>>> Coleen
>>>>>
>>>>> On 2/26/2014 1:56 AM, 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
>>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>
>>
More information about the hotspot-dev
mailing list