[9] RFR(S): 8034839: jvm hangs with gc/gctests/LoadUnloadGC test (original) (raw)

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 Cheap 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 setnext(T* item) { next = item; } + void setprev(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 containsimpl() as common method template<class T, class F> T* GenericHashtable<T, F>::contains(T* item) { if (item != NULL) { int idx = index(item); return containsimpl(item, idx); } return NULL; } template<class T, class F> bool GenericHashtable<T, F>::add(T* item) { if (item != NULL) { int idx = index(item); T* founditem = containsimpl(item, idx); if (founditem == NULL) { ... // code from addimpl() 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* founditem = containsimpl(item, idx); if (founditem != NULL) { ... // code from removeimpl() after (contains) check return founditem; } } 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 Cheap = 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 cheap. Albert added hashtable which can allocate in thread local resource area for temporary table and cheap 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 Cheap = false" instead of "bool resourcemark". It should be saved in a field because you need to free cheap in destructor if C-heap is used: ~GrowableArray() { if (onCheap()) clearanddeallocate(); } 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