Code review request for 7049774

[Chris Hegarty]

On 06/ 9/11 10:15 PM, David Holmes wrote:
> Hi Chris,
>
> Chris Hegarty said the following on 06/10/11 02:05:
>> I looks like this change may break the UID spec.
>>
>> "# time, a long equal to a time (as returned by
>> System.currentTimeMillis()) at which the VM that this UID was
>> generated in was alive, or zero for a well-known UID "
>>
>> 'time' may no longer be equal to currentTimeMillis.
>
> We should have added you to the conversation when a number of us
> discussed this. It doesn't actually say the the time field must be equal
> to the current value of currentTimeMillis, only that it must be a value
> of currentTimeMillis at which point the VM was alive. When time jumps
> backward we are in a bind because we want to preserve uniqueness even
> though the spec says uniqueness is not guaranteed in this case. By
> manually advancing the time we preserve uniqueness, don't "hang" waiting
> for the apparent time to advance, but play a little loose with the
> correlation of the time value.
>
>> It looks like count is incremented for every UID created. I wonder if
>> it may only be necessary to increment count if there is another UID
>> created with the same time. This would allow a much larger number of
>> UID's to be created before encountering the issue you are seeing?
>
> Right this was my reading of the spec too. We could/should be able to
> create 64K UIDs per millisecond. The problem is that if we check
> currentTimeMillis on every construction then performance will be abysmal.

Ah yes I get this now. I don't have a specific problem with the proposed 
change, only that it doesn't look pretty ;-)

Also, just to note that after this change time may now be a value that 
is not equal to a value returned by currentTimeMillis ( at any point in 
lifetime of the VM ). But I don't see a specific problem with this.

I wonder if it is even worth invoking currentTimeMillis more than once. 
That is, lastTime is initialized with currentTimeMillis when the class 
is loaded, then simply increment lastTime when count reaches its 
maximum. I see no real benefit of trying to use the actual current time, 
given the new approach.

But as I said, I don't have a specific problem with Sean's webrev if 
others are in agreement with it.

-Chris.

>
> David
>
>> I'm guess that this change is actually targeted to jdk8, rather than
>> the 'compare against' gate shown in the webrev.
>>
>> -Chris.
>>
>> On 06/ 9/11 04:26 PM, Seán Coffey wrote:
>>> 7049774 : UID construction appears to hang if time changed backwards
>>>
>>> I'm looking for code review of above reported issue. If system clock
>>> goes
>>> backwards on rmi server with active clients and 64k UID boundary is hit,
>>> the server will wait until system time progresses past the time at which
>>> UID class
>>> was instantiated.
>>>
>>> Fix is to not to use earlier times for such corner cases.
>>>
>>> bug ID should become public on bugs.sun.com over coming day.
>>>
>>> webrev : http://cr.openjdk.java.net/~coffeys/webrev.7049774.0/
>>>
>>> Regards,
>>> Sean.