Request for reviews (M): 7017124: VM statistic use 32-bit values which may overflow

[David Holmes]

Vladimir Kozlov said the following on 02/04/11 12:28:
> Vladimir Kozlov wrote:
>> David Holmes wrote:
>>> Vladimir Kozlov said the following on 02/04/11 10:05:
>>>> Added new Atomic method inc_counter() to increment long values,
>>>> it is not precise on MP (lock is not used) but it is fine.
>>>
>>> I strongly disagree! At least it has no place being in the Atomic:: 
>>> set of functions as it is not atomic. It is not even monotonic! It is 
>>
>> It is atomic since it loads and stores all 64 bits of julong atomically.
>> Why we can call atomic methods Atomic::load() and Atomic::store()
>> which are used by new method but not this method?
>> And it is monotonic since value in memory only grows.
> 
> OK, I am taking monotonic statement back. Yes, the value may decrease
> if a thread loads value before other threads but stores it after other
> threads already updated it. But I still want to use atomic load and store
> since otherwise the value will be garbage.

I totally agree that you want the functionality of inc_counter, with 
atomic load and store, but that function is not itself atomic and so 
does not belong in the Atomic class.

David

> Vladimir
> 
>>
>>> extremely deceptive to do this. By all means change to julong where 
>>> needed, but unless they are truly going to be atomic updates (I 
>>> understand the overhead makes that prohibitive) then leave them as 
>>> simply  x+=i; statements.
>>>
>>>> Fixed several output formats to use VM FORMAT macros.
>>>>
>>>> I have to remove v9 check assert from Atomic_move_long() since
>>>> it is called before VM_Version is initialized and C2 is built
>>>> for v9 only anyway.
>>>>
>>>> Removed unused Chunk::clean_chunk_pool().
>>>
>>> Used by CI.
>>
>> Restored.
>>
>>>> I fixed only statistic which was interesting to me.
>>>
>>> So the CR is misleading. It purports to deal with 32-bit stat 
>>> overflows but in fact only deals with some stats, and primarily you 
>>> are using this to define PrintMallocStat - so the CR should reflect 
>>> that: "Fix some VM stats to avoid 32-bit overflow"
>>
>> OK.
>>
>>> globals.hpp:
>>>
>>> +           "print malloc/free statictics")       \
>>>
>>> Typo: statictics
>>
>> Fixed.
>>
>>> src/share/vm/opto/phase.cpp
>>>
>>> This change seems unrelated to the CR.
>>
>> I removed it from these changes.
>>
>> Thanks,
>> Vladimir
>>
>>>
>>> David
>>> -----
>>