RFR (S): 8008314 - Unimplemented() Atomic::load breaks the applications

[Bengt Rutisson]

Hi Jesper,

Thanks for fixing this! I don't feel very comfortable with this code, so 
don't consider this a full review. Just a couple of comments:

It seems to make sense to use uintx for _bytes_written since the flag to 
control the file size, GCLogFileSize, is uintx. But since the atomics 
you use are handling jint I think it looks a bit suspicious. jint is 
always 32 bits and uintx can be 32 or 64 bit. So, what happens here if 
_bytes_written is 64 bit?

  457     Atomic::add((jint)count, (jint*)&_bytes_written);

If _bytes_written is 0x00000000FFFFFFFF and count is 1, I would guess 
that we get 0x0 instead of 0x0000000100000000.

Also, in rotatingFileStream::rotate_log() you changed to:

if (_bytes_written <= GCLogFileSize) return;

instead of :

if (_bytes_writen < (jlong)GCLogFileSize) return;

isn't this a problem since you set _bytes_written to UINT_MAX if we wrap 
around? If someone sets GCLogFileSize on the command line to be UINT_MAX 
we will never rotate the file, right?

Also, we only call rotate_log() when we start a safepoint. If we wrap 
around once in write() but then call it again before a safepoint happens 
we can probably set _bytes_written to a value that looks small enough to 
not have to rotate the log. It seems to me that we would need a separate 
flag to say that we written enough to rotate the log.

Bengt



On 2/19/13 7:25 AM, Jesper Wilhelmsson wrote:
> Hi,
>
> Please review the following change:
>
> Webrev: http://cr.openjdk.java.net/~jwilhelm/8008314/
>
> Bug: https://jbs.oracle.com/bugs/browse/JDK-8008314
>
>
> The use of atomic::add(jlong,jlong*) breaks everything on ARM since it 
> uses atomic::load(jlong) which isn't implemented.
>
> The add was used to update a counter in rotating log file handling, 
> but it turns out that the counter that was updated doesn't need to be 
> a jlong.
>
> The counter is used to indicate that it is time to do a log file 
> rotation and by adding a saftey catch for overflow, we can use an 
> uintx for the counter.
>
> In practice this means that the upper "limit" for the log file size 
> becomes UINX_MAX-1 (used to be UINT_MAX) since I change from < to <= 
> in the comparison to the limit, but since this is not a hard limit the 
> difference will only be noticeable when someone has written UINT_MAX 
> characters to the log file and the next write will end up in the next 
> log file part instead of in the same (which one could argue is a more 
> correct behavior).
>
> Sine I changed all places where the counter was used I also fixed a 
> typo in its name.
>
> Thanks,
> /Jesper