[9] RFR(S): 8034839: jvm hangs with gc/gctests/LoadUnloadGC test
Vladimir Kozlov
vladimir.kozlov at oracle.com
Thu Feb 27 10:20:43 PST 2014
> in src/share/vm/classfile/defaultMethods.cpp. There are nsk defmeth
> tests that will exercise that code.
Coleen, can we measure performance with them or they checks only
correctness?
I am for improving performance of ResourceHashtable.
Thanks,
Vladimir
On 2/27/14 8:53 AM, Coleen Phillimore wrote:
>
> Albert, Thank you for doing this investigation. I'm sorry I didn't
> notice it sooner.
>
> On 2/27/14 9:25 AM, Albert wrote:
>> 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.
>
> That seems fine to do if it improves performance. There is only one
> other use of this hashtable now that would benefit from this change also
> in src/share/vm/classfile/defaultMethods.cpp. There are nsk defmeth
> tests that will exercise that code.
>
> Thanks,
> Coleen
>>
>> 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