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:17:
> 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.

It is not atomic because it does not update the value atomically:

    value += add_value;

Not atomic.

> Why we can call atomic methods Atomic::load() and Atomic::store()
> which are used by new method but not this method?

Atomic::load and Atomic::store are required/expected to be atomic.

> And it is monotonic since value in memory only grows.

It should be monotonic but you need atomic updates for that to be true. 
A counter can be updated by multiple threads simultaneously and that can 
cause updates to appear out of order and hence the value can be seen to
decrease.

David
-----

>> 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
>> -----
>