[9] RFR(S): 8034839: jvm hangs with gc/gctests/LoadUnloadGC test
Coleen Phillimore
coleen.phillimore at oracle.com
Wed Feb 26 11:46:56 PST 2014
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...
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