[9] RFR(S): 8034839: jvm hangs with gc/gctests/LoadUnloadGC test
Coleen Phillimore
coleen.phillimore at oracle.com
Thu Feb 27 08:53:42 PST 2014
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