Update Clock interface, remove TimeProvider #2998

devinrsmith · 2022-10-13T21:38:40Z

Updates the Clock interface providing a much richer selection of methods, allowing the caller to best specify how they actually want to use the results. Removes TimeProvider interface and replaces with Clock. Allows the removal of a lot of extraneous allocation. An allocation-free Clock implementation is provided, but gracefully falls back to the Java UTC clock / Instant implementation.

Fixes #2992

…llers to more accurately describe what they are trying to do. Fixes deephaven#2992

devinrsmith · 2022-10-14T14:18:21Z

This is somewhat orthogonal to #2999, but there will be some conflicts in either case.

This PR is more about the clock interface and plumbing the callers intentions as deep as possible; while the other PR is more about making an allocation free system clock.

rbasralian · 2022-10-14T16:36:34Z

FishUtil/src/main/java/io/deephaven/util/clock/DeltaLocalClockImpl.java

-/**
- * Real-time local clock impl with a delta adjustment for all time values.
- */
-public class DeltaLocalClockImpl implements Clock, Serializable {


We use this in scripts sometimes when setting up a replayer.

I'll bring it back.

rbasralian · 2022-10-14T16:39:46Z

Base/src/main/java/io/deephaven/base/clock/Clock.java

+     * 
+     * @return the nano time
+     */
+    long nanoTime();


Not sure this belongs here...seems like we should just use System.nanoTime() and exclude this from the interface. If you want possibly-fake nanos we have currentTimeNanos() — what are the situations where it really would be right to use this nanoTime() instead of System.nanoTime()?

The idea is that you might want to replay the behavior that you would otherwise get w/ System.nanoTime() (either for unit test, or replaying data), but you don't want to pay the price of currentTimeNanos() in production.

For example, to use AutoTuningIncrementalReleaseFilter, there are three options:

Clock.nanoTime() - step replayable; fast nanoTime in production

System.nanoTime() - not replayable; fast nanoTime in production

Clock.currentTimeNanos() - step replayable; slower currentTimeNanos in production

If you don't care about replayability / unit-testing wrt your timings, then you should use System.nanoTime().

Ok, I'm still a bit skeptical of the legitimacy of case 1, but it does make the interface more complete

rbasralian · 2022-10-14T16:55:32Z

engine/time/src/main/java/io/deephaven/time/TimeProviderNanoBase.java

+
+    @Override
+    public final DateTime currentTime() {
+        return new DateTime(currentTimeNanos());


I think get rid of this class and just make this a default method in TimeProvider. You have to use nanos as the source for any TimeProvider anyway, since a TimeProvider's only job is providing a DateTime and you're limited to a single long when making a DateTime anyway

I'm strongly anti-default methods - I think it muddies the water, and potentially provides places where calls are virtualized where they otherwise wouldn't need to be.

For example, io.deephaven.engine.table.impl.replay.DataDrivenReplayer is explicitly based off of DateTime; if we accidentally forget to override currentTime(), then we'd still get correct, but inefficient, behavior.

I know we use it in a lot of other places, but if I had my way I would remove most of them.

Also, it's completely acceptable to create a TimeProvider wholly from System.currentTimeMillis() if you wanted.

rbasralian · 2022-10-14T17:13:53Z

engine/time/src/main/java/io/deephaven/time/DateTimeUtils.java

@@ -2336,4 +2376,38 @@ public static String getPartitionFromTimestampSeconds(@NotNull final DateTimeFor
        }
        return dateTimeFormatter.format(Instant.ofEpochMilli(timestampSeconds * 1_000));
    }
+
+    private enum TimeProviderImpl implements TimeProvider {


why enum?

I think it'd be more straightforward to just add this INSTANCE as a field in TimeProvider

IMO, when stateless singleton structures can be represented as enum, you gain niceties - equality, hashcode, etc for free.

Base/src/main/java/io/deephaven/base/clock/DeltaClock.java

rbasralian · 2022-10-24T14:26:04Z

Base/src/main/java/io/deephaven/base/clock/Clock.java

+     * 
+     * @return the nano time
+     */
+    long nanoTime();


Ok, I'm still a bit skeptical of the legitimacy of case 1, but it does make the interface more complete

rbasralian · 2022-10-24T23:34:15Z

py/embedded-server/deephaven_server/start_jvm.py

        '--add-opens=java.management/sun.management=ALL-UNNAMED',
+        # Allow our clock-impl project to access internals
+        '--add-opens=java.base/jdk.internal.misc=ALL-UNNAMED',


could just use --add-exports instead of --add-opens — maybe vaguely better not to enable reflection where we don't need it?

--add-opens is necessary from the runtime perspective. --add-exports is a compile-time flag.

Maybe there is some nuance I'm missing, will test.

Great find - --add-opens is more aggressive than --add-exports in allowing reflective access; something we don't need for hotspot-impl nor clock-impl. Updated to reflect this.

buildSrc/src/main/groovy/io/deephaven/project/util/CombinedJavadoc.groovy

clock-impl/src/main/java/io/deephaven/base/clock/SystemClockJdkInternalMiscVm.java

rcaudy · 2022-10-27T03:20:26Z

clock/src/main/java/io/deephaven/base/clock/Clock.java

+ *
+ * @see SystemClock
+ */
+public interface Clock {


Might want to ask @chipkent to look at your interface.

rcaudy · 2022-10-27T03:21:13Z

clock/src/main/java/io/deephaven/base/clock/ClockNanoBase.java

+
+    @Override
+    public final long nanoTime() {
+        return currentTimeNanos();


This is a kind of questionable implementation, since nanoTime() has different semantics than the other methods.

This is a larger question of "Should we have unit-testability for nanoTime logic; and should that interface be on Clock?"

Confirmed w/ Charles, we might want fine replayability of nanoTime for unit tests, even though we don't have them right now.

The next part is "should this be part of clock?"

It may be reasonable to have a separate interface since it is more bespoke than Clock; but what I don't want to encourage is users just deciding to use Clock#currentTimeNanos because it's more convenient than plumbing through an additional nanoTime interface. That said, there aren't many users of Clock#nanoTime (only AutoTuningIncrementalReleaseFilter and BaseIncrementalReleaseFilter), so it would be a small change against this PR.

Options:

Leave Clock#nanoTime as-is

Create new interface MeasureNanos.

Remove Clock#nanoTime, and explicitly use System.nanoTime in BaseIncrementalReleaseFilter/AutoTuningIncrementalReleaseFilter

I'm actually leaning 3 > 2 > 1; at least until we get to the point where we need nanoTime replayability for unit tests.

I kind of like #3 for now. It feels better not to conflate sources of wall-clock time with sources of monotonically-increasing measurement time. I like your point about #1 making it explicit, though.

Proceeded w/ 3

rcaudy · 2022-10-27T03:21:58Z

clock/src/main/java/io/deephaven/base/clock/DeltaClock.java

+
+    @Override
+    public Instant instantNanos() {
+        return delegate.instantNanos().plusNanos(deltaNanos);


We'd be better off implementing this and instantMillis() in terms of nanos, right? This allocates 2 Instants.

So, I'm not as concerned w/ the efficiency of non-standard clock implementations - regardless, I think the current implementation is the "correct" implementation, given that we should always delegate the same method to delegate given it knows more about its implementation than DeltaClock does.

If we really needed to optimize for applying deltas to clocks, that would suggest we should instead have an interface:

interface Clock { ... public Instant instantNanos(long deltaNanos); ... }

But I think we can agree that would be overkill.

clock/src/main/java/io/deephaven/base/clock/SystemClockInstance.java

clock/src/main/java/io/deephaven/base/clock/SystemClockMillisImpl.java

settings.gradle

clock/src/main/java/io/deephaven/base/clock/DeltaClock.java

clock/src/main/java/io/deephaven/base/clock/SystemClockInstance.java

rcaudy · 2022-10-27T18:17:07Z

engine/table/src/main/java/io/deephaven/engine/table/impl/replay/DataDrivenReplayer.java

@@ -20,8 +22,33 @@ public DataDrivenReplayer(DateTime startTime, DateTime endTime) {
    }

    @Override
-    public DateTime currentTime() {
-        return currentTime;
+    public long currentTimeMillis() {


Maybe we can move this up a level to avoid repetition, delegating to an abstract accessor for currentTime.

So, the hierarchy right now is a bit confusing, with DataDrivenReplayer and FixedStepReplayer (extends io.deephaven.engine.table.impl.replay.Replayer) having completely separate overrides than the parent class.

I think it could be cleaned up, but it would be a pretty major gutting that would be better done in isolation IMO.

rcaudy · 2022-10-27T18:18:02Z

engine/table/src/main/java/io/deephaven/engine/table/impl/replay/Replayer.java

 /**
 * Replay historical data as simulated real-time data.
 */
 public class Replayer implements ReplayerInterface, Runnable {
    private static final Logger log = LoggerFactory.getLogger(ShiftObliviousInstrumentedListener.class);
+    private static final int MILLIS_TO_NANOS_FACTOR = 1_000_000;


Maybe we want to prefer using TimeUnit?

I'll delegate back to DateTimeUtils.millisToNanos etc. And remove ENABLE_MICROTIME_HACK in the process.

engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableAggregationTest.java

rcaudy · 2022-10-27T18:37:50Z

engine/table/src/test/java/io/deephaven/engine/util/TimeTableTest.java

@@ -43,7 +44,12 @@ protected void setUp() throws Exception {
        super.setUp();

        now = new MutableLong(0);
-        timeProvider = () -> new DateTime(now.longValue());
+        clock = new ClockNanoBase() {


We do this a lot. Maybe we want one based on a MutableLong?

Elevated TestClock.

clock/src/main/java/io/deephaven/base/clock/DeltaClock.java

chipkent · 2022-10-28T19:19:37Z

clock/src/main/java/io/deephaven/base/clock/Clock.java

+    /**
+     * The {@link SystemClock}. Provides singleton semantics around {@link SystemClock#of()}.
+     *
+     * @return the system clock
+     */
+    static SystemClock systemUTC() {
+        return SystemClockInstance.INSTANCE;
+    }


I don't think this method belongs. It is specific to a single clock, and that clock can be obtained via other methods.

I think the method may actually be better named system()...

That said, it does serve an important service not fulfilled by other methods - it's essentially providing a singleton cache around SystemClock.of(). (SystemClockInstance itself is not public.)

chipkent · 2022-10-28T19:35:32Z

engine/table/src/main/java/io/deephaven/engine/table/impl/replay/ReplayerInterface.java


 import java.io.IOException;
 import java.util.TimerTask;

 /**
 * An interface for replaying historical data as simulated real-time data.
 */
-public interface ReplayerInterface {
+public interface ReplayerInterface extends Clock {


Thoughts on making the replayer instance a Clock vs returning a Clock? I might lean to the second, since it seems funny to say that the replayer is a clock. Also, "is a Clock" is causing a bunch of methods to be duplicated in many files.

I'm happy to change this to "replayer returns a clock" but it's not going to solve any of the duplication - the replayer and classes that extend replayer seem to be built fundamentally different, so I don't think there's an easy path to de-duplication.

I did find a slight way to simplify this by doing "returns a Clock" - and I agree it's a better interface.

chipkent · 2022-10-28T19:43:01Z

engine/time/src/main/java/io/deephaven/time/DateTime.java

-        long epochSecond = nanos / 1000000000;
-        long nanoAdjustment = nanos % 1000000000;
-        return Instant.ofEpochSecond(epochSecond, nanoAdjustment);
+        return Instant.ofEpochSecond(0, nanos);


This looked correct before.

This is a slight improvement, we don't have to do the math ourselves. The internals of ofEpochSecond can deal directly w/ long nanos.

chipkent · 2022-10-28T19:45:34Z

engine/time/src/main/java/io/deephaven/time/DateTimeUtils.java

     */
-    public static TimeProvider timeProvider;
+    public static Clock clock;


Having a static clock here is funky, since we can support multiple replayers, where we probably want the clock to be set based upon the context of the replayer. We are certainly no worse than we were, but we should have a ticket to address this at some point.

I agree - it would be much better to inject and pass along a Clock instead of having a static. I'll create an issue and link it from DateTimeUtils.

Presents more complete Clock and TimeProvider interfaces, allowing ca…

673a014

…llers to more accurately describe what they are trying to do. Fixes deephaven#2992

devinrsmith added bug Something isn't working clean up NoDocumentationNeeded labels Oct 13, 2022

devinrsmith added this to the Oct 2022 milestone Oct 13, 2022

devinrsmith self-assigned this Oct 13, 2022

devinrsmith added 6 commits October 13, 2022 15:24

Add instant

535cce7

DOh, fix constant

2a5c7b2

Create constant

2eb173a

Expose nanoTime

7e5e015

Update AutoTuningIncrementalReleaseFilter for nanoTime

be665d5

Use millis with scheduler instead of DateTime

b952865

devinrsmith mentioned this pull request Oct 14, 2022

Allocation-free precise and accurate clocks #2999

Closed

devinrsmith marked this pull request as ready for review October 14, 2022 14:18

devinrsmith requested a review from rcaudy October 14, 2022 14:18

rbasralian reviewed Oct 14, 2022

View reviewed changes

Reintroduce delta clock. Cleanup usage of DateTimeUtils methods.

fcd0592

devinrsmith requested a review from rbasralian October 14, 2022 22:00

devinrsmith added 4 commits October 15, 2022 15:32

Add back DateTimeUtils method

a523d4d

Create system time provider

388f6bf

Fix currentTime

ac80d76

Remove TimeProvider

4e442e6

rbasralian reviewed Oct 24, 2022

View reviewed changes

devinrsmith added 6 commits October 24, 2022 07:57

Update javadoc

8ee22e2

addExact for DeltaClock

fdd0fe3

SystemClockMillis

f138e12

Migrate to clock package

812f5e2

header

01f66ad

Configuration and VM based clock

bd924d0

javadoc

a6eb736

devinrsmith requested a review from rbasralian October 24, 2022 21:28

rbasralian previously approved these changes Oct 25, 2022

View reviewed changes

Use --add-exports instead of --add-opens where applicable

150e0aa

devinrsmith dismissed rbasralian’s stale review via 150e0aa October 25, 2022 15:56

devinrsmith requested a review from rbasralian October 25, 2022 16:09

rbasralian previously approved these changes Oct 25, 2022

View reviewed changes

rcaudy reviewed Oct 27, 2022

View reviewed changes

devinrsmith added 2 commits October 27, 2022 13:24

Review responses

67a84b9

Remove Clock#nanoTime interface

a17da7e

devinrsmith dismissed rbasralian’s stale review via a17da7e October 27, 2022 20:36

devinrsmith requested a review from rcaudy October 27, 2022 20:40

devinrsmith changed the title ~~Presents more complete Clock and TimeProvider interfaces, allowing ca…~~ Update Clock interface, remove TimeProvider Oct 27, 2022

rcaudy reviewed Oct 27, 2022

View reviewed changes

clock/src/main/java/io/deephaven/base/clock/DeltaClock.java Outdated Show resolved Hide resolved

delta clock javadoc

28deb67

devinrsmith requested a review from rcaudy October 27, 2022 21:35

rcaudy previously approved these changes Oct 27, 2022

View reviewed changes

chipkent requested changes Oct 28, 2022

View reviewed changes

devinrsmith mentioned this pull request Oct 28, 2022

Improve scaffolding around full system replay #3044

Open

Review responses - Chip

cf5ffce

devinrsmith dismissed rcaudy’s stale review via cf5ffce October 28, 2022 20:45

devinrsmith requested a review from chipkent October 28, 2022 20:45

chipkent approved these changes Oct 28, 2022

View reviewed changes

devinrsmith merged commit 76b42f9 into deephaven:main Oct 28, 2022

devinrsmith deleted the better-clock-timeprovider branch October 28, 2022 22:16

github-actions bot locked and limited conversation to collaborators Oct 28, 2022

Update Clock interface, remove TimeProvider #2998

Update Clock interface, remove TimeProvider #2998

Conversation

devinrsmith commented Oct 13, 2022 • edited Loading

devinrsmith commented Oct 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinrsmith Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinrsmith Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinrsmith Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcaudy Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinrsmith Oct 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinrsmith commented Oct 13, 2022 •

edited

Loading

devinrsmith Oct 14, 2022 •

edited

Loading

devinrsmith Oct 14, 2022 •

edited

Loading

devinrsmith Oct 14, 2022 •

edited

Loading

rcaudy Oct 27, 2022 •

edited

Loading

devinrsmith Oct 28, 2022 •

edited

Loading