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