Re: RFR: 8266846: Add java.time.InstantSource [v6]
On Thu, 3 Jun 2021 20:59:24 GMT, Stephen Colebourne wrote: >> 8266846: Add java.time.InstantSource > > Stephen Colebourne has updated the pull request incrementally with one > additional commit since the last revision: > > 8266846: Add java.time.InstantSource Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v6]
On Thu, 3 Jun 2021 20:59:24 GMT, Stephen Colebourne wrote: >> 8266846: Add java.time.InstantSource > > Stephen Colebourne has updated the pull request incrementally with one > additional commit since the last revision: > > 8266846: Add java.time.InstantSource Marked as reviewed by darcy (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v6]
On Thu, 3 Jun 2021 20:59:24 GMT, Stephen Colebourne wrote: >> 8266846: Add java.time.InstantSource > > Stephen Colebourne has updated the pull request incrementally with one > additional commit since the last revision: > > 8266846: Add java.time.InstantSource Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v6]
> 8266846: Add java.time.InstantSource Stephen Colebourne has updated the pull request incrementally with one additional commit since the last revision: 8266846: Add java.time.InstantSource - Changes: - all: https://git.openjdk.java.net/jdk/pull/4016/files - new: https://git.openjdk.java.net/jdk/pull/4016/files/f01c5cdd..d564956c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4016=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4016=04-05 Stats: 7 lines in 1 file changed: 6 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4016.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4016/head:pull/4016 PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v5]
On Sun, 30 May 2021 00:49:56 GMT, Stephen Colebourne wrote: >> 8266846: Add java.time.InstantSource > > Stephen Colebourne has updated the pull request incrementally with one > additional commit since the last revision: > > 8266846: Add java.time.InstantSource Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v5]
On Sun, 30 May 2021 00:49:56 GMT, Stephen Colebourne wrote: >> 8266846: Add java.time.InstantSource > > Stephen Colebourne has updated the pull request incrementally with one > additional commit since the last revision: > > 8266846: Add java.time.InstantSource Looks good to me. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v5]
> 8266846: Add java.time.InstantSource Stephen Colebourne has updated the pull request incrementally with one additional commit since the last revision: 8266846: Add java.time.InstantSource - Changes: - all: https://git.openjdk.java.net/jdk/pull/4016/files - new: https://git.openjdk.java.net/jdk/pull/4016/files/c7d9076b..f01c5cdd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4016=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4016=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4016.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4016/head:pull/4016 PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v4]
On Wed, 19 May 2021 21:58:08 GMT, Stephen Colebourne wrote: >> 8266846: Add java.time.InstantSource > > Stephen Colebourne has updated the pull request incrementally with one > additional commit since the last revision: > > 8266846: Add java.time.InstantSource I believe that the default execution mode is `agentvm` so it will leave unnecessary side effects. To make it run in `othervm` mode, possibly you will need to tweak TEST.properties (not tried though). - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v3]
On Tue, 18 May 2021 23:18:42 GMT, Stephen Colebourne wrote: >> 8266846: Add java.time.InstantSource > > Stephen Colebourne has updated the pull request incrementally with one > additional commit since the last revision: > > 8266846: Add java.time.InstantSource I've made the obvious changes to fix the offset reflection. However it does now mean that the offset is being altered for a singleton where previously it would have only affected Clock.systemUtc(). Is the test class spun up in its own JVM process? Just want to make sure that manipulating the singleton clock won't impact other tests. - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v4]
> 8266846: Add java.time.InstantSource Stephen Colebourne has updated the pull request incrementally with one additional commit since the last revision: 8266846: Add java.time.InstantSource - Changes: - all: https://git.openjdk.java.net/jdk/pull/4016/files - new: https://git.openjdk.java.net/jdk/pull/4016/files/425e01a8..c7d9076b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4016=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4016=02-03 Stats: 6 lines in 2 files changed: 2 ins; 1 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/4016.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4016/head:pull/4016 PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v3]
On Tue, 18 May 2021 23:18:42 GMT, Stephen Colebourne wrote: >> 8266846: Add java.time.InstantSource > > Stephen Colebourne has updated the pull request incrementally with one > additional commit since the last revision: > > 8266846: Add java.time.InstantSource my bad - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v3]
On Tue, 18 May 2021 23:18:42 GMT, Stephen Colebourne wrote: >> 8266846: Add java.time.InstantSource > > Stephen Colebourne has updated the pull request incrementally with one > additional commit since the last revision: > > 8266846: Add java.time.InstantSource The Error was caused by the movement of the field `offset`, from inner `SystemClock` class to `Clock`. - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v3]
On Tue, 18 May 2021 23:18:42 GMT, Stephen Colebourne wrote: >> 8266846: Add java.time.InstantSource > > Stephen Colebourne has updated the pull request incrementally with one > additional commit since the last revision: > > 8266846: Add java.time.InstantSource It's a side effect of JEP 403, i believe - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v3]
On Tue, 18 May 2021 23:18:42 GMT, Stephen Colebourne
wrote:
>> 8266846: Add java.time.InstantSource
>
> Stephen Colebourne has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8266846: Add java.time.InstantSource
Another test started failing with your fix. Apparently, the following piece in
java/time/test/java/time/TestClock_System.java throws
ExceptionInInitializerError.
static {
try {
offsetField =
Class.forName("java.time.Clock$SystemClock").getDeclaredField("offset");
offsetField.setAccessible(true);
} catch (ClassNotFoundException | NoSuchFieldException ex) {
throw new ExceptionInInitializerError(ex);
}
}
-
PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v3]
On Wed, 19 May 2021 08:41:08 GMT, Peter Levart wrote: >> This isn't my logic - it is existing code that has been moved. I'm not a fan >> of infinite retry loops as the can hang the system. But I'm happy to change >> it to work that way if there is a consensus to do so. > > I see the localOffset passed to VM.getNanoTimeAdjustment() has to be in the > range of current time +/- 2^32 seconds. This means the thread would have to > sleep for ~136 years before the exception is triggered. I think this is more > than enough. :-) Exactly. You could hit the 1ns if the operating system clock was concurrently moved backward by a NTP adjustment of ~ 1024 seconds at the same time that the offset is taken but this would be extremely unlikely - I don't believe it deserves any infinite retry loop. That was discussed at the time the enhancement that provides sub-millisecond precision was proposed, and it was rejected in favor of a one time retry. - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v2]
On Tue, 18 May 2021 23:22:49 GMT, Stephen Colebourne wrote: >> test/jdk/java/time/test/java/time/TestInstantSource.java line 86: >> >>> 84: assertEquals(test.instant(), instant); >>> 85: assertSame(test.equals(InstantSource..fixed(instant)); >>> 86: assertEquals(test.hashCode(), >>> InstantSource..fixed(instant).hashCode()); >> >> Not sure this would compile. > > I was expecting the GitHub Action to pickup coding and test issues (since my > dev setup can't compile or run tests). But it seems it doesn't. do that. The > latest commit should at least compile as I copied the test class to another > IDE location, but I have no way of knowing if the tests pass. GitHub Actions just verifies build and tier1 testing, while java.time tests are in tier2, so it won't catch the failure. The test case still does not compile without the `import java.time.InstantSource` statement. I've verified the test passed with that fix. - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v3]
On Tue, 18 May 2021 23:18:42 GMT, Stephen Colebourne wrote: >> 8266846: Add java.time.InstantSource > > Stephen Colebourne has updated the pull request incrementally with one > additional commit since the last revision: > > 8266846: Add java.time.InstantSource test/jdk/java/time/test/java/time/TestInstantSource.java line 33: > 31: import java.time.Duration; > 32: import java.time.Instant; > 33: import java.time.ZoneId; Needs `import java.time.InstantSource;` - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v3]
On Tue, 18 May 2021 23:14:53 GMT, Stephen Colebourne wrote: >> src/java.base/share/classes/java/time/Clock.java line 487: >> >>> 485: // it more unlikely to hit the 1ns in the future condition. >>> 486: localOffset = System.currentTimeMillis()/1000 - 1024; >>> 487: >> >> Is it possible that after a fresh localOffset is retrieved, the thread is >> preempted and when it is scheduled again after a pause, the >> getNanoTimeAdjustment below returns -1 ? Would it help if instead of >> throwing exception, there was an infinite retry loop? > > This isn't my logic - it is existing code that has been moved. I'm not a fan > of infinite retry loops as the can hang the system. But I'm happy to change > it to work that way if there is a consensus to do so. I see the localOffset passed to VM.getNanoTimeAdjustment() has to be in the range of current time +/- 2^32 seconds. This means the thread would have to sleep for ~136 years before the exception is triggered. I think this is more than enough. :-) - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v2]
On Tue, 18 May 2021 21:04:18 GMT, Naoto Sato wrote: >> Stephen Colebourne has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8266846: Add java.time.InstantSource > > test/jdk/java/time/test/java/time/TestInstantSource.java line 86: > >> 84: assertEquals(test.instant(), instant); >> 85: assertSame(test.equals(InstantSource..fixed(instant)); >> 86: assertEquals(test.hashCode(), >> InstantSource..fixed(instant).hashCode()); > > Not sure this would compile. I was expecting the GitHub Action to pickup coding and test issues (since my dev setup can't compile or run tests). But it seems it doesn't. do that. The latest commit should at least compile as I copied the test class to another IDE location, but I have no way of knowing if the tests pass. - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v3]
On Sun, 16 May 2021 07:39:21 GMT, Peter Levart wrote: >> Stephen Colebourne has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8266846: Add java.time.InstantSource > > src/java.base/share/classes/java/time/Clock.java line 487: > >> 485: // it more unlikely to hit the 1ns in the future condition. >> 486: localOffset = System.currentTimeMillis()/1000 - 1024; >> 487: > > Is it possible that after a fresh localOffset is retrieved, the thread is > preempted and when it is scheduled again after a pause, the > getNanoTimeAdjustment below returns -1 ? Would it help if instead of throwing > exception, there was an infinite retry loop? This isn't my logic - it is existing code that has been moved. I'm not a fan of infinite retry loops as the can hang the system. But I'm happy to change it to work that way if there is a consensus to do so. - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v3]
> 8266846: Add java.time.InstantSource Stephen Colebourne has updated the pull request incrementally with one additional commit since the last revision: 8266846: Add java.time.InstantSource - Changes: - all: https://git.openjdk.java.net/jdk/pull/4016/files - new: https://git.openjdk.java.net/jdk/pull/4016/files/7fa9d0ec..425e01a8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4016=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4016=01-02 Stats: 12 lines in 3 files changed: 1 ins; 3 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/4016.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4016/head:pull/4016 PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v2]
On Mon, 17 May 2021 17:19:19 GMT, Stephen Colebourne
wrote:
>> 8266846: Add java.time.InstantSource
>
> Stephen Colebourne has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8266846: Add java.time.InstantSource
src/java.base/share/classes/java/time/Instant.java line 275:
> 273: */
> 274: public static Instant now() {
> 275: return Clock.currentInstant();
Copyright year should change to 2021 for this change.
test/jdk/java/time/test/java/time/TestInstantSource.java line 59:
> 57: assertTrue(Math.abs(testMillis - millis) < 1000);
> 58: assertTrue(Math.abs(testInstantMillis - millis) < 1000);
> 59: assertSame(test.equals(InstantSource.system());
')' is missing. Couple other locations.
test/jdk/java/time/test/java/time/TestInstantSource.java line 86:
> 84: assertEquals(test.instant(), instant);
> 85: assertSame(test.equals(InstantSource..fixed(instant));
> 86: assertEquals(test.hashCode(),
> InstantSource..fixed(instant).hashCode());
Not sure this would compile.
-
PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v2]
On Mon, 17 May 2021 17:19:19 GMT, Stephen Colebourne wrote: >> 8266846: Add java.time.InstantSource > > Stephen Colebourne has updated the pull request incrementally with one > additional commit since the last revision: > > 8266846: Add java.time.InstantSource Changes requested by plevart (Reviewer). src/java.base/share/classes/java/time/Clock.java line 487: > 485: // it more unlikely to hit the 1ns in the future condition. > 486: localOffset = System.currentTimeMillis()/1000 - 1024; > 487: Is it possible that after a fresh localOffset is retrieved, the thread is preempted and when it is scheduled again after a pause, the getNanoTimeAdjustment below returns -1 ? Would it help if instead of throwing exception, there was an infinite retry loop? - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v2]
On Mon, 17 May 2021 17:13:58 GMT, Stephen Colebourne
wrote:
>> src/java.base/share/classes/java/time/Clock.java line 128:
>>
>>> 126: * Implementations should implement {@code Serializable} wherever
>>> possible and must
>>> 127: * document whether or not they do support serialization.
>>> 128: *
>>
>> The ImplSpec needs to say how it is implemented.
>> The 'implements InstantSource' can not mandate any particular
>> implementation. Its just an interface the real behavior comes from its
>> implementations. In this case Clock. Referring to the static methods of
>> InstantSource behavior may be sufficient because that behavior is concrete.
>
> There are plenty of examples of interfaces in `java.time` and elsewhere that
> apply restrictions to implementations. Nevertheless, for simplicity and
> expediency I have reinstated the `implSpec` on `Clock`
ok, thanks.
Assertions in interfaces are toothless. Tests can only be written for
implementations.
-
PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource
On Thu, 13 May 2021 20:52:33 GMT, Stephen Colebourne wrote: > 8266846: Add java.time.InstantSource FWIW, if there is a need to access `VM.getNanoTimeAdjustment(localOffset)` then that should be a separate issue. I don't personally think it would be approved given Valhalla is coming. - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v2]
On Mon, 17 May 2021 14:30:20 GMT, Roger Riggs wrote:
>> Stephen Colebourne has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8266846: Add java.time.InstantSource
>
> src/java.base/share/classes/java/time/Clock.java line 128:
>
>> 126: * Implementations should implement {@code Serializable} wherever
>> possible and must
>> 127: * document whether or not they do support serialization.
>> 128: *
>
> The ImplSpec needs to say how it is implemented.
> The 'implements InstantSource' can not mandate any particular implementation.
> Its just an interface the real behavior comes from its implementations. In
> this case Clock. Referring to the static methods of InstantSource behavior
> may be sufficient because that behavior is concrete.
There are plenty of examples of interfaces in `java.time` and elsewhere that
apply restrictions to implementations. Nevertheless, for simplicity and
expediency I have reinstated the `implSpec` on `Clock`
-
PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v2]
> 8266846: Add java.time.InstantSource Stephen Colebourne has updated the pull request incrementally with one additional commit since the last revision: 8266846: Add java.time.InstantSource - Changes: - all: https://git.openjdk.java.net/jdk/pull/4016/files - new: https://git.openjdk.java.net/jdk/pull/4016/files/befc7621..7fa9d0ec Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4016=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4016=00-01 Stats: 38 lines in 2 files changed: 31 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/4016.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4016/head:pull/4016 PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource
On Mon, 17 May 2021 14:13:06 GMT, Roger Riggs wrote:
>> 8266846: Add java.time.InstantSource
>
> src/java.base/share/classes/java/time/InstantSource.java line 165:
>
>> 163: */
>> 164: static InstantSource fixed(Instant fixedInstant) {
>> 165: return Clock.fixed(fixedInstant, ZoneOffset.UTC);
>
> Instant might also implement InstantSource, returning itself.
> This fixed method would be unnecessary because an Instant is itself a
> InstantSource.
While I can see the appeal, I don't think this would be a good choice. Doing so
would add three methods to `Instant` that are of no value. `millis()` would
duplicate `toEpochMilli()`, `withZone(zone)` would duplicate `atZone(zone)` and
`instant()` would return `this`. Moreover it doesn't respect the rationale of
the interface, which is explictly to return the current instant, not any random
instant.
Effectively, this proposal is a variation of Scala's implicit conversions which
were rejected during the development of JSR-310 as being more likely to cause
bugs than help developers,
-
PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource
On Mon, 17 May 2021 14:04:24 GMT, Roger Riggs wrote:
>> 8266846: Add java.time.InstantSource
>
> src/java.base/share/classes/java/time/InstantSource.java line 36:
>
>> 34: * Instances of this interface are used to find the current instant.
>> 35: * As such, an {@code InstantSource} can be used instead of {@link
>> System#currentTimeMillis()}.
>> 36: *
>
> The word 'current' is likely to misleading here. The specification of an
> interface does not have any context in which to describe what the instant
> represents or what it is relative to.
> Given the intended use cases, it is definitely not always related to
> System.currentTimeMillis()
> which is bound to the running system.
> i think the best you could say is that it returns an instant provided by the
> source as does the 'instance()' method.
This is the definition used by `Clock` since Java 8. It is also the purpose of
the interface. ie. this isn't an interface for providing instants in general,
but for providing the _current instant_. I can clarify wrt the meaning of
"current", but I don't see how that word can be avoided.
-
PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource
On Mon, 17 May 2021 07:50:22 GMT, Stephen Colebourne wrote: >> 8266846: Add java.time.InstantSource > > It would good to get confirmation in the review from @dholmes-ora or Mark > Kralj-Taylor that my changes to the precise system clock are correct. > Specifically, I moved the code from instance to static. I don't believe this > changes the thread-safety, but it does need double checking (there was no > discussion in the original thread concerning instance vs static). > > Original thread: > http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-May/066670.html @jodastephen While an implementation can certainly implement the now() method there is nothing useful it can do in the JVM to get anything more than millisecond precision. It needs to perform the logic in currentInstant() which requires a call to VM.getNanoTimeAdjustment(localOffset) which it doesn't have access to. This is precisely why it needs to call Clock to pass in a structure that can be populated with the millis and nanos. - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource
On Thu, 13 May 2021 20:52:33 GMT, Stephen Colebourne
wrote:
> 8266846: Add java.time.InstantSource
Changes requested by rriggs (Reviewer).
src/java.base/share/classes/java/time/Clock.java line 128:
> 126: * Implementations should implement {@code Serializable} wherever
> possible and must
> 127: * document whether or not they do support serialization.
> 128: *
The ImplSpec needs to say how it is implemented.
The 'implements InstantSource' can not mandate any particular implementation.
Its just an interface the real behavior comes from its implementations. In
this case Clock. Referring to the static methods of InstantSource behavior may
be sufficient because that behavior is concrete.
src/java.base/share/classes/java/time/InstantSource.java line 36:
> 34: * Instances of this interface are used to find the current instant.
> 35: * As such, an {@code InstantSource} can be used instead of {@link
> System#currentTimeMillis()}.
> 36: *
The word 'current' is likely to misleading here. The specification of an
interface does not have any context in which to describe what the instant
represents or what it is relative to.
Given the intended use cases, it is definitely not always related to
System.currentTimeMillis()
which is bound to the running system.
i think the best you could say is that it returns an instant provided by the
source as does the 'instance()' method.
src/java.base/share/classes/java/time/InstantSource.java line 64:
> 62: * @implSpec
> 63: * This interface must be implemented with care to ensure other classes
> operate correctly.
> 64: * All implementations must be thread-safe.
It would be useful to expand on the invariants that must be maintained, or
examples of incorrect implementations.
src/java.base/share/classes/java/time/InstantSource.java line 165:
> 163: */
> 164: static InstantSource fixed(Instant fixedInstant) {
> 165: return Clock.fixed(fixedInstant, ZoneOffset.UTC);
Instant might also implement InstantSource, returning itself.
This fixed method would be unnecessary because an Instant is itself a
InstantSource.
-
PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource
On Thu, 13 May 2021 20:52:33 GMT, Stephen Colebourne wrote: > 8266846: Add java.time.InstantSource It would good to get confirmation in the review from @dholmes-ora or Mark Kralj-Taylor that my changes to the precise system clock are correct. Specifically, I moved the code from instance to static. I don't believe this changes the thread-safety, but it does need double checking (there was no discussion in the original thread concerning instance vs static). Original thread: http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-May/066670.html - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource
On Mon, 17 May 2021 07:31:45 GMT, Stephen Colebourne
wrote:
>> src/java.base/share/classes/java/time/InstantSource.java line 93:
>>
>>> 91: * @since 17
>>> 92: */
>>> 93: public interface InstantSource {
>>
>> Should not we add `@FunctionalInterface`? I can easily imagine this
>> interface being used in tests where we can define the `InstantSource` with
>> lambdas.
>
> `@FunctionalInterface` isn't required for use by lambdas. I wasn't initially
> convinced using it here was the right choice.
Right, it's about signaling that it's safe to be used with a
lambda/method-reference, so it's not required but a nice to have, very like
@Override.
-
PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource
On Sat, 15 May 2021 21:32:54 GMT, Ralph Goers wrote: >> 8266846: Add java.time.InstantSource > > Can you possibly find a way that this can be used in a garbage free manner? > Providing a MutableInstant interface that allows the Instant object to be > provided and have its values set by a currentInstant(MutableInstant instant) > method would solve the problem nicely for Log4j - or any other app that is > trying to avoid allocating new objects. I believe this also aligns with what > [Michael Hixson](https://github.com/michaelhixson) is asking for. > > An alternative would be for java.time to maintain a pool of Instants that > apps like Log4j can somehow control, but that would seem far more complicated. @rgoers Michael's request has been handled, which was a simple change to the wording. What you need cannot be accomplished because `Instant` has a restricted design due to the Valhalla project. A `MutableInstant` class can have its own `now()` method that does whatever is necessary to initialize it, so there is no need to refer to it here. - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource
On Sat, 15 May 2021 20:53:28 GMT, Istvan Neuwirth
wrote:
>> 8266846: Add java.time.InstantSource
>
> src/java.base/share/classes/java/time/InstantSource.java line 93:
>
>> 91: * @since 17
>> 92: */
>> 93: public interface InstantSource {
>
> Should not we add `@FunctionalInterface`? I can easily imagine this interface
> being used in tests where we can define the `InstantSource` with lambdas.
`@FunctionalInterface` isn't required for use by lambdas. I wasn't initially
convinced using it here was the right choice.
-
PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource
On Thu, 13 May 2021 20:52:33 GMT, Stephen Colebourne wrote: > 8266846: Add java.time.InstantSource Can you possibly find a way that this can be used in a garbage free manner? Providing a MutableInstant interface that allows the Instant object to be provided and have its values set by a currentInstant(MutableInstant instant) method would solve the problem nicely for Log4j - or any other app that is trying to avoid allocating new objects. - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource
On Thu, 13 May 2021 20:52:33 GMT, Stephen Colebourne
wrote:
> 8266846: Add java.time.InstantSource
src/java.base/share/classes/java/time/InstantSource.java line 93:
> 91: * @since 17
> 92: */
> 93: public interface InstantSource {
Should not we add `@FunctionalInterface`? I can easily imagine this interface
being used in tests where we can define the `InstantSource` with lambdas.
-
PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource
On Thu, 13 May 2021 20:52:33 GMT, Stephen Colebourne wrote: > 8266846: Add java.time.InstantSource src/java.base/share/classes/java/time/InstantSource.java line 68: > 66: * @implSpec > 67: * This interface must be implemented with care to ensure other classes > operate correctly. > 68: * All implementations that can be instantiated must be final, immutable > and thread-safe. (I'm not an openjdk reviewer) I'm wondering if we can revisit this "immutable" requirement as it is being shuffled from `Clock` to `InstantSource`. This precludes an implementation that might be useful for testing, where the time provided by a single `InstantSource` instance can be adjusted manually in the test code. To me that is a notable downside (worth breaking the contract for), and it's not so clear what the upside is. "Immutable" seems like an odd way to describe the system clock anyway. Do you know if the people who already use their own `InstantSource` / `TimeSource` / `Supplier` have a mutable version that they use in tests? I would be surprised if they're all satisfied with `InstantSource.fixed`, but perhaps my intuition is wrong here. - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource
On Fri, 14 May 2021 07:19:03 GMT, Anthony Vanelverdinghe
wrote:
>> 8266846: Add java.time.InstantSource
>
> src/java.base/share/classes/java/time/Clock.java line 513:
>
>> 511: * {@link System#currentTimeMillis()} or equivalent.
>> 512: */
>> 513: static final class SystemInstantSource implements InstantSource,
>> Serializable {
>
> Is it possible to declare this as an enum? As doing so would simplify its
> implementation.
My feeling is that in this case it is better to keep control of the kind of
class being used.
-
PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource
On Thu, 13 May 2021 20:52:33 GMT, Stephen Colebourne
wrote:
> 8266846: Add java.time.InstantSource
src/java.base/share/classes/java/time/Clock.java line 2:
> 1: /*
> 2: * Copyright (c) 2012, 2019, 2021, Oracle and/or its affiliates. All
> rights reserved.
`2019, ` should be removed.
src/java.base/share/classes/java/time/Clock.java line 115:
> 113: * Where an application only requires the current instant {@code
> InstantSource} should
> 114: * be preferred to this class. For example, his might be the case where
> the time-zone
> 115: * is provided by a separate user localization subsystem.
`@see InstantSource` would be preferred here.
src/java.base/share/classes/java/time/InstantSource.java line 32:
> 30: import java.time.Clock.SystemClock;
> 31: import java.time.Clock.SystemInstantSource;
> 32: import java.time.Clock.TickClock;
Some of them are not used.
src/java.base/share/classes/java/time/InstantSource.java line 112:
> 110: * @return a source that uses the best available system clock, not
> null
> 111: */
> 112: public static InstantSource system() {
`public` is redundant here, same for other methods.
-
PR: https://git.openjdk.java.net/jdk/pull/4016
RFR: 8266846: Add java.time.InstantSource
8266846: Add java.time.InstantSource - Commit messages: - 8266846: Add java.time.InstantSource - 8266846: Add java.time.InstantSource Changes: https://git.openjdk.java.net/jdk/pull/4016/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4016=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8266846 Stats: 615 lines in 4 files changed: 519 ins; 83 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/4016.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4016/head:pull/4016 PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource
On Thu, 13 May 2021 20:52:33 GMT, Stephen Colebourne
wrote:
> 8266846: Add java.time.InstantSource
A nice addition to the JDK, thanks for taking this on.
src/java.base/share/classes/java/time/Clock.java line 114:
> 112: * provides access to the current instant, and does not provide access
> to the time-zone.
> 113: * Where an application only requires the current instant {@code
> InstantSource} should
> 114: * be preferred to this class. For example, his might be the case where
> the time-zone
his -> this
src/java.base/share/classes/java/time/Clock.java line 513:
> 511: * {@link System#currentTimeMillis()} or equivalent.
> 512: */
> 513: static final class SystemInstantSource implements InstantSource,
> Serializable {
Is it possible to declare this as an enum? As doing so would simplify its
implementation.
src/java.base/share/classes/java/time/InstantSource.java line 59:
> 57: * }
> 58: *
> 59: * This approach allows an alternate source, such as {@link
> #fixed(Instant) fixed}
alternate -> alternative? To me (being a non-native speaker) the latter reads
more naturally
src/java.base/share/classes/java/time/InstantSource.java line 62:
> 60: * or {@link #offset(InstantSource, Duration) offset} to be used during
> testing.
> 61: *
> 62: * The {@code system} factory method provide a source based on the best
> available
provide -> provides
src/java.base/share/classes/java/time/InstantSource.java line 63:
> 61: *
> 62: * The {@code system} factory method provide a source based on the best
> available
> 63: * system clock This may use {@link System#currentTimeMillis()}, or a
> higher
missing dot after "clock"
src/java.base/share/classes/java/time/InstantSource.java line 175:
> 173: /**
> 174: * Obtains a source that returns instants from the specified source
> with the
> 175: * specified duration added
missing dot to end the sentence
-
PR: https://git.openjdk.java.net/jdk/pull/4016
