RFR: 8072645: java.util.logging should use java.time to get more precise time stamps

Sat Feb 14 15:33:17 UTC 2015

Hi Daniel,

On 02/14/2015 01:38 PM, Daniel Fuchs wrote:
> Hi Peter,
>
> On 2/14/15 10:36 AM, Peter Levart wrote:
>> Hi Daniel,
>>
>> The "millis" property in your proposal just changes one part of 
>> LogRecord's notion of time (it doesn't change/reset the 
>> nanoAdjustment part). From compatibility standpoint, your ptoposal is 
>> changing the semantics of "millis" property. Previously "millis" was 
>> the sole property of record's notion of time, now it is only a 
>> component of it. Consider this legacy code:
>>
>> LogRecord r = new LogRecord(...); // timestamp1
>> ...
>> r.setMillis(System.currentTimeMillis()); // timestamp2
>>
>> What is the record's notion of time now? It is composed of "millis" 
>> from timestamp2 and "nanoAdjustment" from timestamp1. Not something 
>> that one would want, right?
>
> Excellent observation. I had missed that.
>> So what should be done instead is:
>> - deprecate "millis" writable property and document that it sets the 
>> record's time with the resolution of milliseconds and point to new 
>> "instantUTC" property as a replacement.
>> - add "instantUTC" writable property and document it as "the" method 
>> to be used to set record's time with full resolution
>> - optionally add read-only "nanoAdjustment" property (I don't think 
>> it is needed, since users should start using new time API instead of 
>> mangling with millis and nanos themselves)
>>
>> This lends itself to also change internal storage of LogRecord's 
>> time. You could just replace the "millis" field with a reference to 
>> "instantUTC". Serialization would have to be adjusted a bit (using 
>> serialPersistentFields) to stay compatible.
>>
>> What do you think?
>
> setMillis should definitely set the whole time for backward compatibility
> reasons. That will make it more than a simple setter, and therefore 
> deprecating
> it is probably the best thing to do as the method name could then become
> misleading (setTimeAsMillis() would be the expected name for such a
> behaviour).

Unfortunately. But there is javadoc to clear things out, so I don't find 
this too bad.

>
> That lets me think that there shouldn't be a setNanoAdjustment() either.

That's right. And I don't think we need a getter for nanoAdjustment 
either. See below...

> getMillis() and getNanoAdjustment() are OK and can stay, since they
> are consistent - are needed for compatibility reasons (at least 
> getMillis is),
> and will help with describing the serial form.
>
> As you noted, the only correct way to set the LogRecord time is to do 
> it in a
> single method:
> We could have either setInstant() or setTime(long millis, int nanos) - 
> setInstant
> being most probably the best alternative - since we already have a 
> getInstant(),
> and splitting the time into millis + nanos is a bit strange anyway - 
> since
> java.time favors seconds + nanos.

get/setInstant is the right name. As Stephen notes, Instant is timezone 
agnostic.

>
> For serialization - I think we will need to keep serializing a number 
> of milliseconds
> and a nano second adjustment. Whether the time stamp should be stored 
> internally
> as an Instant or as a number of milliseconds + a nano adjustment can 
> be discussed.
>
> I might favor the second as it would make serialization easier 
> (especially for
> documenting the serial form).
>
> Would you agree with that?

Well, internally I think it should be stored as an Instant. So you don't 
have to reconstruct Instant objects at each call to getInstant(). 
Serialization of LogRecord is not a frequent requirement, so the 
overhead should be moved to it, rather than to frequent code-paths. 
That's my opinion.

Documenting serializable format should be easy anyway. It doesn't have 
to be based directly on public getters/setters. For example:

The LogRecord's "instant" is serialized as two hypothetical fields to 
keep backwards compatibility:

long millis = getInstant().toEpochMilli();

int nanoOfMilli = getInstant().getNano() % 1000_000;

Regards, Peter

>
> best regards,
>
> -- daniel
>
>>
>> Regards, Peter
>>
>> On 02/13/2015 03:56 PM, Daniel Fuchs wrote:
>>> Hi,
>>>
>>> Please find below a patch for:
>>>
>>> 8072645: java.util.logging should use java.time to get more
>>>          precise time stamps
>>>
>>>
>>> http://cr.openjdk.java.net/~dfuchs/webrev_8072645/webrev.00/
>>>
>>> specdiff:
>>> http://cr.openjdk.java.net/~dfuchs/webrev_8072645/specdiff-logging-time/java/util/logging/package-summary.html 
>>>
>>>
>>> Overview:
>>> ---------
>>>
>>> The patch is made of the following pieces:
>>>
>>>  - LogRecord uses java.time.Clock's systemClock to get an
>>>    Instant in the best available resolution.
>>>
>>>    The instant is split into a number of milliseconds (a long)
>>>    and a nanosecond adjustment (an int).
>>>    The number of milliseconds is the same than what would have
>>>    been obtained by calling System.currentTimeMillis().
>>>
>>>  - LogRecord acquires a new serializable int nanoAdjustement field,
>>>    which can be used together with the number of milliseconds
>>>    to reconstruct the instant.
>>>
>>>  - SimpleFormatter is updated to pass a ZoneDateTime
>>>    instance to String.format, instead of a Date.
>>>
>>>    The effect of that is that the format string can now
>>>    be configure to print the full instant precision, if
>>>    needed.
>>>
>>>  - XMLformatter will add a new <nanos> element after the
>>>    <millis> element - if the value of the nanoAdjustment
>>>    field is not 0.
>>>
>>>    The <date> string will also contain the nano second
>>>    adjustment as well as the zone offset as formatted by
>>>    DateTimeFormatter.ISO_OFFSET_DATE_TIME
>>>
>>> Compatibility considerations:
>>> -----------------------------
>>>
>>> - The serial for of log record is backward/forward compatible.
>>>   I added a test to verify that.
>>>
>>> - XMLFormatter has acquired a new configurable property
>>>   '<FQCN>.printNanos' which allows to revert to the old
>>>   XML format, should the new format cause issues in
>>>   existing applications.
>>>
>>> - The logger.dtd will need to be updated, to support the
>>>   new optional <nanos> element. And for this matter,
>>>   should we update the logger.dtd or rather define a
>>>   logger-v2.dtd?
>>>   See planned modification:
>>>
>>> <http://cr.openjdk.java.net/~dfuchs/webrev_8072645/logger-dtd/logger.dtd.frames.html> 
>>>
>>>
>>> best regards,
>>>
>>> -- daniel
>>
>