[9] RFR(S): 8034839: jvm hangs with gc/gctests/LoadUnloadGC test
Vladimir Kozlov
vladimir.kozlov at oracle.com
Wed Feb 26 11:57:54 PST 2014
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