Re: Proposal for new interface: TimeSource
As I commented on the PR, the test needs to run in othervm mode: https://github.com/openjdk/jdk/pull/4016#issuecomment-844551175 --- a/test/jdk/java/time/test/TEST.properties +++ b/test/jdk/java/time/test/TEST.properties @@ -1,5 +1,5 @@ # java.time tests use TestNG TestNG.dirs = .. -othervm.dirs = java/time/chrono java/time/format +othervm.dirs = java/time lib.dirs = /test/lib /test/jdk/tools/lib lib.build = jdk.test.lib.RandomFactory This will run the test case in java/time directory explicitly in othervm mode, but other tests in java/time too, which may be unnecessary. I am not aware of other way to specify it individually. Naoto On 5/27/21 5:03 PM, Stephen Colebourne wrote: Hi all, Is there anything I need to do to progress the CSR and/or PR? thanks Stephen On Thu, 13 May 2021 at 22:05, Stephen Colebourne wrote: On Wed, 12 May 2021 at 18:41, Roger Riggs wrote: Will you be posting a PR for the implementation? It is frequently helpful to evaluate the CSR and the implementation concurrently. PR: https://github.com/openjdk/jdk/pull/4016 Issue: https://bugs.openjdk.java.net/browse/JDK-8266846 CSR: https://bugs.openjdk.java.net/browse/JDK-8266847 The PR takes a middle ground approach to the implementation. It is not practical to remove the existing package-scoped Clock implementation classes (SystemClock, TickClock, FixedClock, OffsetClock) despite the fact that these would be better expressed as classes that only implement `InstantSource`. However, given that "system" is the 99%+ use case, I do believe it is worth adding a dedicated `SystemInstantSource` class, as per the PR. To achieve this I moved the actual logic using `VM.getNanoAdjustment()` into a static method which can then be called directly from three places - Instant.now(), SystemClock and SystemInstantSource. Previously, every instance of SystemClock performed the VM/offset calculations separately. The new logic performs them once based on a single shared static variable. I have no reason to believe this changes the memory model or performance, but I must flag it up for reviewers. When initially discussing the proposal, I planned to add a new static method `Clock.of(InstantSource, ZoneId)`. When implementing the change I found that the method was adding no value as the instance method `InstantSource.withZone(ZoneId)` achieves the same outcome, so I omitted it. The Mac test failure appears to be unconnected to the change. Thanks for any and all reviews! Stephen
Re: Proposal for new interface: TimeSource
On 5/27/2021 5:03 PM, Stephen Colebourne wrote: Hi all, Is there anything I need to do to progress the CSR and/or PR? The CSR is in Provisional state. To request the second phase of CSR review, the assignee can Finalize the CSR; for more details see https://wiki.openjdk.java.net/display/csr/Main HTH, -Joe
Re: Proposal for new interface: TimeSource
Hi all, Is there anything I need to do to progress the CSR and/or PR? thanks Stephen On Thu, 13 May 2021 at 22:05, Stephen Colebourne wrote: > > On Wed, 12 May 2021 at 18:41, Roger Riggs wrote: > > Will you be posting a PR for the implementation? > > It is frequently helpful to evaluate the CSR and the implementation > > concurrently. > > PR: https://github.com/openjdk/jdk/pull/4016 > Issue: https://bugs.openjdk.java.net/browse/JDK-8266846 > CSR: https://bugs.openjdk.java.net/browse/JDK-8266847 > > The PR takes a middle ground approach to the implementation. > > It is not practical to remove the existing package-scoped Clock > implementation classes (SystemClock, TickClock, FixedClock, > OffsetClock) despite the fact that these would be better expressed as > classes that only implement `InstantSource`. However, given that > "system" is the 99%+ use case, I do believe it is worth adding a > dedicated `SystemInstantSource` class, as per the PR. > > To achieve this I moved the actual logic using > `VM.getNanoAdjustment()` into a static method which can then be called > directly from three places - Instant.now(), SystemClock and > SystemInstantSource. Previously, every instance of SystemClock > performed the VM/offset calculations separately. The new logic > performs them once based on a single shared static variable. I have no > reason to believe this changes the memory model or performance, but I > must flag it up for reviewers. > > When initially discussing the proposal, I planned to add a new static > method `Clock.of(InstantSource, ZoneId)`. When implementing the change > I found that the method was adding no value as the instance method > `InstantSource.withZone(ZoneId)` achieves the same outcome, so I > omitted it. > > The Mac test failure appears to be unconnected to the change. > > Thanks for any and all reviews! > Stephen
Re: Proposal for new interface: TimeSource
On Wed, 12 May 2021 at 18:41, Roger Riggs wrote: > Will you be posting a PR for the implementation? > It is frequently helpful to evaluate the CSR and the implementation > concurrently. PR: https://github.com/openjdk/jdk/pull/4016 Issue: https://bugs.openjdk.java.net/browse/JDK-8266846 CSR: https://bugs.openjdk.java.net/browse/JDK-8266847 The PR takes a middle ground approach to the implementation. It is not practical to remove the existing package-scoped Clock implementation classes (SystemClock, TickClock, FixedClock, OffsetClock) despite the fact that these would be better expressed as classes that only implement `InstantSource`. However, given that "system" is the 99%+ use case, I do believe it is worth adding a dedicated `SystemInstantSource` class, as per the PR. To achieve this I moved the actual logic using `VM.getNanoAdjustment()` into a static method which can then be called directly from three places - Instant.now(), SystemClock and SystemInstantSource. Previously, every instance of SystemClock performed the VM/offset calculations separately. The new logic performs them once based on a single shared static variable. I have no reason to believe this changes the memory model or performance, but I must flag it up for reviewers. When initially discussing the proposal, I planned to add a new static method `Clock.of(InstantSource, ZoneId)`. When implementing the change I found that the method was adding no value as the instance method `InstantSource.withZone(ZoneId)` achieves the same outcome, so I omitted it. The Mac test failure appears to be unconnected to the change. Thanks for any and all reviews! Stephen
Re: Proposal for new interface: TimeSource
Hi Stephen,
A useful enhancement.
Will you be posting a PR for the implementation?
It is frequently helpful to evaluate the CSR and the implementation
concurrently.
Thanks, Roger
On 5/10/21 1:06 AM, Aaron Scott-Boddendijk wrote:
Yes please.
I often have people ask how they should solve exactly this problem and we
have several code-bases that have their own implementations of essentially
this interface.
We've used it not only for the request-contextual time localisation but for
controlling the stepping for data-processing and simulation.
A standard interface might also mean this makes its way into 1st-class
testing framework parameterisation.
--
Aaron Scott-Boddendijk
On Fri, May 7, 2021 at 11:28 AM Stephen Colebourne
wrote:
This is a proposal to add a new interface to java.time.*
public interface TimeSource {
public static TimeSource system() { ... }
public abstract Instant instant();
public default long millis() {
return instant().toEpochMilli();
}
public default Clock withZone(ZoneId zoneId) {
return Clock.of(this, zoneId);
}
}
The existing `Clock` abstract class would be altered to implement the
interface.
A new static method `Clock.of(TimeSource, ZoneId)` would be added.
No changes are needed to any other part of the API.
(Could add `Instant.now(TimeSource)`, but it isn't entirely necessary)
Callers would pass around and use `TimeSource` directly, for example
by dependency injection.
Why add this interface?
I've seen various indications that there is a desire for an interface
representing a supplier of `Instant`. Specifically this desire is
driven by the "unnecessary" time zone that `java.time.Clock` contains.
Put simply, if the only thing you want is an `Instant`, then `Clock`
isn't the right API because it forces you to think about time zones.
A key thing that this interface allows is the separation of the OS
Clock from the time-zone (which should generally be linked to user
localization). A good architecture would pass `TimeSource` around the
system and combine it with a time-zone held in a `UserContext` to get
a `Clock`. The current design of java.time.* does not enable that
because the `TimeSource` concept does not exist. Developers either
have to write their own interface, or use `Clock` and ignore the time
zone.
A `Supplier` obviously performs similar functionality, but it
lacks discoverability and understandability. Plus, injecting
generified interfaces tends to be painful.
Downsides?
None really, other than a new type in the JDK that probably should
have been in Java 8.
See this ThreeTen-Extra discussion
- https://github.com/ThreeTen/threeten-extra/issues/150
Joda-Time has a `MillisProvider` that is similar:
-
https://www.joda.org/joda-time/apidocs/org/joda/time/DateTimeUtils.MillisProvider.html
Time4J has a `TimeSource`:
-
https://github.com/MenoData/Time4J/blob/master/base/src/main/java/net/time4j/base/TimeSource.java
Spring has a `DateTimeContext` that separates the user localization as
per the `UserContext` described above:
-
https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/format/datetime/standard/DateTimeContext.html
There is a similar concept `TimeSource` in `sun.net.httpserver`
There may be a case to name the interface `InstantSource`, however
`TimeSource` is a fairly widely understood name for this concept.
There is the potential to extend the interface with something similar
to Kotlin's `TimeMark` that would allow access to the monotonic clock,
suitable for measurement of durations without any leap second effects:
- https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.time/-time-mark/
Feedback?
Stephen
Re: Proposal for new interface: TimeSource
Yes please.
I often have people ask how they should solve exactly this problem and we
have several code-bases that have their own implementations of essentially
this interface.
We've used it not only for the request-contextual time localisation but for
controlling the stepping for data-processing and simulation.
A standard interface might also mean this makes its way into 1st-class
testing framework parameterisation.
--
Aaron Scott-Boddendijk
On Fri, May 7, 2021 at 11:28 AM Stephen Colebourne
wrote:
> This is a proposal to add a new interface to java.time.*
>
> public interface TimeSource {
> public static TimeSource system() { ... }
> public abstract Instant instant();
> public default long millis() {
> return instant().toEpochMilli();
> }
> public default Clock withZone(ZoneId zoneId) {
> return Clock.of(this, zoneId);
>}
> }
>
> The existing `Clock` abstract class would be altered to implement the
> interface.
> A new static method `Clock.of(TimeSource, ZoneId)` would be added.
> No changes are needed to any other part of the API.
> (Could add `Instant.now(TimeSource)`, but it isn't entirely necessary)
>
> Callers would pass around and use `TimeSource` directly, for example
> by dependency injection.
>
>
> Why add this interface?
> I've seen various indications that there is a desire for an interface
> representing a supplier of `Instant`. Specifically this desire is
> driven by the "unnecessary" time zone that `java.time.Clock` contains.
> Put simply, if the only thing you want is an `Instant`, then `Clock`
> isn't the right API because it forces you to think about time zones.
>
> A key thing that this interface allows is the separation of the OS
> Clock from the time-zone (which should generally be linked to user
> localization). A good architecture would pass `TimeSource` around the
> system and combine it with a time-zone held in a `UserContext` to get
> a `Clock`. The current design of java.time.* does not enable that
> because the `TimeSource` concept does not exist. Developers either
> have to write their own interface, or use `Clock` and ignore the time
> zone.
>
> A `Supplier` obviously performs similar functionality, but it
> lacks discoverability and understandability. Plus, injecting
> generified interfaces tends to be painful.
>
> Downsides?
> None really, other than a new type in the JDK that probably should
> have been in Java 8.
>
>
> See this ThreeTen-Extra discussion
> - https://github.com/ThreeTen/threeten-extra/issues/150
>
> Joda-Time has a `MillisProvider` that is similar:
> -
> https://www.joda.org/joda-time/apidocs/org/joda/time/DateTimeUtils.MillisProvider.html
>
> Time4J has a `TimeSource`:
> -
> https://github.com/MenoData/Time4J/blob/master/base/src/main/java/net/time4j/base/TimeSource.java
>
> Spring has a `DateTimeContext` that separates the user localization as
> per the `UserContext` described above:
> -
> https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/format/datetime/standard/DateTimeContext.html
>
> There is a similar concept `TimeSource` in `sun.net.httpserver`
>
> There may be a case to name the interface `InstantSource`, however
> `TimeSource` is a fairly widely understood name for this concept.
>
>
> There is the potential to extend the interface with something similar
> to Kotlin's `TimeMark` that would allow access to the monotonic clock,
> suitable for measurement of durations without any leap second effects:
> - https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.time/-time-mark/
>
> Feedback?
>
> Stephen
>
Proposal for new interface: TimeSource
This is a proposal to add a new interface to java.time.*
public interface TimeSource {
public static TimeSource system() { ... }
public abstract Instant instant();
public default long millis() {
return instant().toEpochMilli();
}
public default Clock withZone(ZoneId zoneId) {
return Clock.of(this, zoneId);
}
}
The existing `Clock` abstract class would be altered to implement the interface.
A new static method `Clock.of(TimeSource, ZoneId)` would be added.
No changes are needed to any other part of the API.
(Could add `Instant.now(TimeSource)`, but it isn't entirely necessary)
Callers would pass around and use `TimeSource` directly, for example
by dependency injection.
Why add this interface?
I've seen various indications that there is a desire for an interface
representing a supplier of `Instant`. Specifically this desire is
driven by the "unnecessary" time zone that `java.time.Clock` contains.
Put simply, if the only thing you want is an `Instant`, then `Clock`
isn't the right API because it forces you to think about time zones.
A key thing that this interface allows is the separation of the OS
Clock from the time-zone (which should generally be linked to user
localization). A good architecture would pass `TimeSource` around the
system and combine it with a time-zone held in a `UserContext` to get
a `Clock`. The current design of java.time.* does not enable that
because the `TimeSource` concept does not exist. Developers either
have to write their own interface, or use `Clock` and ignore the time
zone.
A `Supplier` obviously performs similar functionality, but it
lacks discoverability and understandability. Plus, injecting
generified interfaces tends to be painful.
Downsides?
None really, other than a new type in the JDK that probably should
have been in Java 8.
See this ThreeTen-Extra discussion
- https://github.com/ThreeTen/threeten-extra/issues/150
Joda-Time has a `MillisProvider` that is similar:
-
https://www.joda.org/joda-time/apidocs/org/joda/time/DateTimeUtils.MillisProvider.html
Time4J has a `TimeSource`:
-
https://github.com/MenoData/Time4J/blob/master/base/src/main/java/net/time4j/base/TimeSource.java
Spring has a `DateTimeContext` that separates the user localization as
per the `UserContext` described above:
-
https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/format/datetime/standard/DateTimeContext.html
There is a similar concept `TimeSource` in `sun.net.httpserver`
There may be a case to name the interface `InstantSource`, however
`TimeSource` is a fairly widely understood name for this concept.
There is the potential to extend the interface with something similar
to Kotlin's `TimeMark` that would allow access to the monotonic clock,
suitable for measurement of durations without any leap second effects:
- https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.time/-time-mark/
Feedback?
Stephen
