feat: add timing API #149

imurchie · 2019-12-17T21:36:09Z

We have a number of different mechanisms for getting the duration of operations. This is a first pass at making an extensible API for our use. Once the Performance API is out of super-experimental in Node, this could encapsulate that too, without needing application code to be messed with.

jlipps · 2019-12-17T22:07:46Z

lib/timing.js

+   */
+  start () {
+    // once Node 10 is no longer supported, this check can be removed
+    this.startTime = _.isFunction(process.hrtime.bigint)


startTime could be memoized during module load so it doesn't need to happen every time this class is used. Probably a very small optimization, but presumably when timing things we want to have as little overhead as possible.

and by startTime, i mean, the function used to get the start time

lib/timing.js

test/timing-specs.js

imurchie · 2019-12-18T18:28:07Z

Added the ability to just print the Timer itself, without calling getDuration(). E.g.,

const timer = new timing.Timer().start();
// ...
log.debug(`The operation took ${timer}`);

outputs

The operation took 42ms

lib/timing.js

mykola-mokhnach · 2019-12-18T18:38:41Z

lib/timing.js

+  /**
+   * Creates a timer
+   *
+   * @param {?string} units - the default units for conversion


I still cannot understand why units are needed. We could simply keep the absolute number of seconds/nanoseconds and have asSeconds, asNanos, etc. methods like TimeSpan in C# or Duration in Java have

imurchie · 2019-12-18T19:52:03Z

Alright. Not sure why it's preferable, but moved the conversions in the Duration class, so now it is used something like

const timer = new Timer().start();

// ...

log.debug(`This took ${timer.getDuration().asSeconds.toFixed(3)}s`)

mykola-mokhnach · 2019-12-18T20:46:16Z

lib/timing.js

+ * Class representing a duration, encapsulating the number and units.
+ */
+class Duration {
+  constructor (duration) {


I's rather rename the property to nanos

mykola-mokhnach · 2019-12-18T20:47:36Z

lib/timing.js

+    this._duration = duration;
+  }
+
+  get duration () {


isn't this getter redundant now?

imurchie · 2019-12-18T20:56:06Z

Basically. But mirroring the internal, read-only variable.

mykola-mokhnach · 2019-12-18T21:00:06Z

lib/timing.js

+  /**
+   * Get the duration since the timer was started
+   *
+   * @return {Duration} the duration, in the specified units


the param description should be updated

mykola-mokhnach · 2019-12-18T21:00:51Z

lib/timing.js

+    if (_.isArray(this.startTime)) {
+      // startTime was created using process.hrtime()
+      const [seconds, nanos] = process.hrtime(this.startTime);
+      nanoDuration = seconds * NS_PER_S + nanos;


we can return immediately here. There is no need for the nanoDuration intermediate variable

mykola-mokhnach · 2019-12-18T21:05:54Z

lib/timing.js

+/**
+ * Class representing a duration, encapsulating the number and units.
+ */
+class Duration {


In the future we could make the Duration class more advanced (e.g. add +/- operations, factory methods to create duration objects from different units (aka Duration.fromSeconds(x), etc)) and use it in other scenarios.
Or use the one, from moment.js lib.

Although, the current implementation is pretty enough for the task it is supposed to solve

jlipps approved these changes Dec 17, 2019

View reviewed changes