Allocation-free precise and accurate clocks #2999
Conversation
|
This change is worth a before and after JMH benchmark for getting millis/micros/nanos in a tight loop. |
| final RealTimeClock impl; | ||
| switch (REAL_TIME_CLOCK_SOURCE) { | ||
| case "jdk-internals": | ||
| final Iterator<RealTimeClock> it = ServiceLoader.load(RealTimeClock.class).iterator(); |
There was a problem hiding this comment.
Can you safely try jdk-internals if you have something like "default"; then degrade to instant?
| test { | ||
| jvmArgs '--add-exports', 'java.base/jdk.internal.misc=ALL-UNNAMED' | ||
| systemProperty 'RealTimeClockLoader.timeSource', 'jdk-internals' | ||
| // systemProperty 'RealTimeClockLoader.timeSource', 'millis' |
There was a problem hiding this comment.
Ideally one would test both ways, though the millis and instant implementations are trivial. You might add a comment to that effect.
|
I've got a parallel draft PR that addresses a lot of deficiencies in our interfaces, but doesn't take any of the add-opens approach yet so I think both parts may be valuable. Will look soon. #2998 |
| @@ -3,17 +3,21 @@ | |||
| */ | |||
| package io.deephaven.base.clock; | |||
|
|
|||
| import io.deephaven.clock.RealTimeClock; | |||
| import org.jetbrains.annotations.NotNull; | |||
|
|
|||
| public interface Clock { | |||
There was a problem hiding this comment.
We should probably move this to the clock package.
| } | ||
| long currentTimeNanos(); | ||
|
|
||
| final class Null implements Clock { |
There was a problem hiding this comment.
enum is a better pattern IMO, but I noticed there aren't any real users of Null.
| cachedNowMicros = 0; | ||
| } | ||
| @NotNull | ||
| public final RealTimeClock REAL_TIME_CLOCK = RealTimeClock.loadImpl(); |
| /** | ||
| * Copyright (c) 2016-2022 Deephaven Data Labs and Patent Pending | ||
| */ | ||
| package io.deephaven.clock.impl; |
There was a problem hiding this comment.
We'll want to use a different license for clock-impl if we use the code as-is since we are using systemUTC rather directly here. That may not be true if we change our strategy for calling VM
| * {@link VM#getNanoTimeAdjustment}), between the current time and midnight, January 1, 1970 UTC. | ||
| */ | ||
| @Override | ||
| public long currentTimeNanos() { |
There was a problem hiding this comment.
We should test out some different strategies. We may be able to get away w/ simply:
return VM.getNanoTimeAdjustment(0);| /** | ||
| * Created by rbasralian on 10/13/22 | ||
| */ | ||
| public final class JavaInstantRealTimeClock implements RealTimeClock { |
There was a problem hiding this comment.
can be enum w/ single INSTANCE.
| import java.util.Optional; | ||
| import java.util.ServiceLoader; | ||
|
|
||
| public interface RealTimeClock { |
There was a problem hiding this comment.
We should probably have RealTimeClock extends Clock
|
|
||
| test { | ||
| jvmArgs '--add-exports', 'java.base/jdk.internal.misc=ALL-UNNAMED' | ||
| systemProperty 'RealTimeClockLoader.timeSource', 'jdk-internals' |
There was a problem hiding this comment.
IMO, we should have an "escape" hatch, but the majority of cases shouldn't need to specify the system property IMO.
| runtimeOnly project(':clock-impl') | ||
| } | ||
| extraJvmArgs += ['--add-exports', 'java.base/jdk.internal.misc=ALL-UNNAMED'] | ||
| extraJvmArgs += ['-DRealTimeClockLoader.timeSource=jdk-internals'] |
There was a problem hiding this comment.
again, this should be implicit based on the classpath for the majority of use-cases.
| @@ -43,6 +43,8 @@ Finally, Gradle can be used to update the build and run the application in a sin | |||
| `--add-opens java.management/sun.management=ALL-UNNAMED`. To disable this, set the gradle property `includeHotspotImpl` | |||
| to `false`. | |||
|
|
|||
| ... also clockImpl and java.base/jdk.internal.misc=ALL-UNNAMED | |||
|
For reference, here's the underlying JDK code path for VM.getNanoTimeAdjustment: https://github.com/openjdk/jdk/blob/jdk-17+35/src/hotspot/share/prims/jvm.cpp#L234-L282 https://github.com/openjdk/jdk/blob/jdk-17+35/src/hotspot/os/posix/os_posix.cpp#L1385-L1428 https://github.com/openjdk/jdk/blob/jdk-17+35/src/hotspot/os/windows/os_windows.cpp#L1013-L1059 |
|
Benchmarking nanoTime() is hard. It may be a little better w/ REALTIME clocks b/c they don't have to be monotonic. |
|
See #2998 |
Support for using
jdk.internal.misc.VM#getNanoTimeAdjustment,Instant.now(), orSystem.currentTimeMillis()as the source of "now" in the engine