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