perf_hooks: implementation of Performance Timing API #14680

jasnell · 2017-08-07T23:14:43Z

An initial implementation of the Performance Timing API for Node.js. This is the same Performance Timing API implemented by modern browsers with a number of Node.js specific properties. The User Timing mark() and measure() APIs are implemented, as is a Node.js specific flavor of the Frame Timing for measuring event loop duration.

const { performance } = require('perf_hooks');
performance.mark('A');
setTimeout(() => {
  performance.mark('B');
  performance.measure('A to B', 'A', 'B');
  const entry = performance.getEntriesByName('A to B', 'measure');
  console.log(entry.duration);
}, 10000);

The implementation is at the native layer and makes use of uv_hrtime().
This should enable eventual integeration with the Inspector protocol
so that the performance timeline can be viewed in debugging tools.

The implementation is extensible and should allow us to add new
performance entry types as we go (e.g. for measuring i/o perf,
etc).

Documentation and a test are provided.

/cc @mcollina

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

perf_hooks

Trott · 2017-08-07T23:21:45Z

Suggestion: Run make coverage and be sure that all the new code paths are covered by tests.

mscdex · 2017-08-07T23:22:21Z

Why deviate from browsers by putting this on process instead of making it "global" like we do with setTimeout, setInterval, etc.?

addaleax

I’ll try to give this a full review later :)

addaleax · 2017-08-07T23:18:46Z

doc/api/process.md

+or `'measure'`.
+
+### Class: PerformanceFrame
+<!~- YAML


Starting here, these comments have a broken start token

addaleax · 2017-08-07T23:26:43Z

lib/internal/performance.js

+}
+
+class Performance {
+  constructor() {}


Should this throw?

Refs: http://eslint.org/docs/rules/no-useless-constructor

No, it shouldn't. The constructor is not exposed to end users and those tho get there via process.performance.constructor won't hurt anything if they happen to create a new instance.

jasnell · 2017-08-07T23:45:08Z

@mscdex ... every time I suggest adding a global, people say no.

jasnell · 2017-08-07T23:45:26Z

@Trott ... I will be :-)

mscdex · 2017-08-08T00:01:00Z

@jasnell That might be the case in general, but for APIs shared with browsers it makes more sense to me IMHO. I can't imagine there would be that many people negatively affected by adding it as a "global" (real or otherwise).

jasnell · 2017-08-08T00:02:23Z

@mscdex ... I don't disagree, but I've run into opposition for URL, TextEncoder and TextDecoder, all of which are globals in the browser.

cjihrig · 2017-08-07T23:50:12Z

test/parallel/test-performance.js

+assert(process.performance);
+assert(process.performance.nodeTiming);
+assert(process.performance.nodeFrame);
+assert(process.performance.timeOrigin);


The next line makes this assertion unnecessary.

cjihrig · 2017-08-07T23:53:02Z

test/parallel/test-performance.js

+}
+
+{
+  let n;


You don't need to hoist n.

cjihrig · 2017-08-07T23:59:58Z

src/node.cc

@@ -4633,6 +4639,7 @@ inline int Start(uv_loop_t* event_loop,

 int Start(int argc, char** argv) {
  atexit([] () { uv_tty_reset_mode(); });
+


Unrelated change.

cjihrig · 2017-08-08T00:00:20Z

src/node.cc

}
-

Unrelated change.

cjihrig · 2017-08-08T00:00:33Z

src/node.cc

@@ -4525,10 +4527,12 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
  Environment env(isolate_data, context);
  CHECK_EQ(0, uv_key_create(&thread_local_env));
  uv_key_set(&thread_local_env, &env);
+


Unrelated change.

vsemozhetbyt · 2017-08-08T00:12:34Z

doc/api/process.md

+Returns a list of all `PerformanceEntry` objects in chronological order
+with respect to `startTime` whose `performanceEntry.type` is equal to `type`.
+
+#### performance.getEntriesByName(name[, type])


It seems this should go before performance.getEntriesByType(type) alphabetically.

vsemozhetbyt · 2017-08-08T00:15:19Z

doc/api/process.md

@@ -1858,3 +2178,6 @@ cases:
 [note on process I/O]: process.html#process_a_note_on_process_i_o
 [process_emit_warning]: #process_process_emitwarning_warning_type_code_ctor
 [process_warning]: #process_event_warning
+[W3C Performance Timeline]: https://www.w3.org/TR/performance-timeline


These three references are out of the sorting order.

vsemozhetbyt · 2017-08-08T00:25:51Z

doc/api/process.md

+* `endMark` {string}
+
+Creates a new `PerformanceMeasure` entry in the Performance Timeline. A
+`PerformanceMeasure` is a subclass of `PerformanceMeasure` whose


is a subclass of PerformanceEntry?

vsemozhetbyt · 2017-08-08T00:33:20Z

doc/api/process.md

+
+The `endMark` argument must identify any *existing* `PerformanceMark` in the
+the Performance Timeline or any of the timestamp properties provided by the
+`PerformanceNodeTiming` class. The the named `endMark` does not exist, an


The the -> If the?

refack · 2017-08-08T03:15:22Z

Hooking it on process seems strange... IMHO either set it as global or make it a module.

TimothyGu

Code review will come later...

TimothyGu · 2017-08-08T03:40:22Z

doc/api/process.md

+The high resolution millisecond timestamp marking the starting time of the
+Performance Entry.
+
+#### performanceEntry.type


It's entryType in the spec...

TimothyGu · 2017-08-08T03:41:18Z

doc/api/process.md

+-->
+
+The Performance Timing API provides an implementation of the
+[W3C Performance Timeline][] specification. The purpose of the API


@domenic Should we link to the published version of the spec or the latest editor's draft?

Also this should reference User Timing and High Resolution Time specs since we also implement those.

Always the latest ED

Intentionally started with the first version, will be updating to the latest. The plan is to update incrementally.

TimothyGu · 2017-08-08T03:43:37Z

doc/api/process.md

+The [`timeOrigin`][] specifies the high-resolution millisecond timestamp from
+which all performance metric durations are measured.
+
+### Class: PerformanceEntry


In Level 2 there's also a toJSON() method in PerformanceEntry that just serializes the four properties.

TimothyGu · 2017-08-08T03:44:47Z

doc/api/process.md

+});
+```
+
+### Class: Performance


High Resolution Time also defines a toJSON() method.

jasnell · 2017-08-08T05:07:22Z

A couple of things that still need to be done (in case anyone wants to help ;-) ...)

Implement PerformanceObserver .. I've got a couple of ideas on this already so may get to it as early as tomorrow.
Ensure that the list of PerformanceEntry instances are in proper chronological order. PerformanceMeasure instances in particular.
Implement toJSON() on PerformanceEntry

vsemozhetbyt · 2017-08-08T20:41:30Z

doc/api/process.md

+
+### Class: process.PerformanceObserver(callback)
+
+* `callback` {Function}


I am not sure, but maybe it is worth documenting the callback's arguments? Is observer the same as the new process.PerformanceObserver() here? And maybe PerformanceObserverEntryList API is also worth documenting?

Yeah, i'm going to be updating to include those details :-)

vsemozhetbyt · 2017-08-08T20:47:53Z

doc/api/process.md

+```
+
+Because `PerformanceObserver` instances introduce their own additional
+performance overhead, instances should be left subscribed to notifications


It may be my mediocre English, but "instances should be left subscribed to notifications indefinitely" and "Users should disconnect observers as soon as they are no longer needed" seem contradictory. Should it be "should not be left"?

I just forgot the not in should not

jasnell · 2017-08-08T21:51:55Z

Putting this in a new core module makes sense. Currently, the perf_hooks module name is not taken on npm and has good alignment with async_hooks. More obvious other options like performance and perf are already taken.

jasnell · 2017-08-09T00:25:05Z

Ok, I've refactored this to use a new perf_hooks module...

e.g.

const {
  performance,
  PerformanceObserver
} = require('perf_hooks');

performance.mark('test');

thefourtheye · 2017-08-09T01:04:54Z

doc/api/perf_hooks.md

+The `startMark` argument may identify any *existing* `PerformanceMark` in the
+the Performance Timeline, or *may* identify any of the timestamp properties
+provided by the `PerformanceNodeTiming` class. If the named `startMark` does
+not exist, then `startMark` is set to `timeOrigin` by default.


This is the first occurrence of timeOrigin in this file. I think it is better to provide link here.

thefourtheye · 2017-08-09T01:07:08Z

doc/api/perf_hooks.md

+
+The `endMark` argument must identify any *existing* `PerformanceMark` in the
+the Performance Timeline or any of the timestamp properties provided by the
+`PerformanceNodeTiming` class. If the named `endMark` does not exist, an


If the endMark is not passed, shouldn't we throw an error instead of stringifying an undefined and using it?

We do throw.

thefourtheye · 2017-08-09T01:08:18Z

doc/api/perf_hooks.md

+
+* {number}
+
+The total number of milliseconds ellapsed for this entry. This value will not


thefourtheye · 2017-08-09T01:09:16Z

doc/api/perf_hooks.md

+
+* {number}
+
+The high resolution millisecond timestamp marking the starting time of the


In few other places in the documentation, high resolution is hyphenated. Perhaps we should unhyphenate them?

thefourtheye · 2017-08-09T01:09:57Z

doc/api/perf_hooks.md

+
+* {string}
+
+The type of the performance entry. Current may be one of: `'node'`, `'mark'`,


Did you mean Currently it may be?

thefourtheye · 2017-08-09T01:15:34Z

doc/api/perf_hooks.md

+* `options` {Object}
+  * `entryTypes` {Array} An array of strings identifying the types of
+    `PerformanceEntry` instances the observer is interested in. If not
+    provided an error will be throw.


thefourtheye · 2017-08-09T01:15:58Z

doc/api/perf_hooks.md

+    `PerformanceEntry` instances the observer is interested in. If not
+    provided an error will be throw.
+  * `buffered` {boolean} If true, the notification callback will be
+    called using `SetImmediate()` and multiple `PerformanceEntry` instance


setImmediate

thefourtheye · 2017-08-09T01:16:51Z

doc/api/process.md

@@ -1852,11 +1852,11 @@ cases:
 [Cluster]: cluster.html
 [Duplex]: stream.html#stream_duplex_and_transform_streams
 [LTS]: https://github.com/nodejs/LTS/
+[note on process I/O]: process.html#process_a_note_on_process_i_o


Unrelated change?

thefourtheye · 2017-08-09T01:28:50Z

doc/api/perf_hooks.md

+with respect to `startTime` whose `performanceEntry.entryType` is equal to
+`type`.
+
+### performance.mark([name])


If the same name is used, it resets, right? That has to be captured here.

@domenic ... one point of clarification on this. If I call performance.mark('A') twice, should there be two mark entries in the timeline returned by GetEntries() or only one?

As far as I can tell from skimming https://w3c.github.io/user-timing/#dom-performance-mark it creates two entries. Maybe @igrigorik can confirm.

thefourtheye · 2017-08-09T01:31:08Z

src/node_perf.h

+// * PerformanceFrame
+//
+// PerformanceNodeTiming provides information about Node.js specific
+// Node.js startup milestones.


Nit: This Node.js feels redundant

mcollina

LGTM with some nits.

mcollina · 2017-08-09T07:21:54Z

lib/perf_hooks.js

+  }
+
+  getEntries() {
+    return Array.from(this[kEntries]);


Last time I checked, Array.from was extremely slow. Can we do without?

the response has to be an array. I'll be looking at this more tho

mcollina · 2017-08-09T07:24:42Z

lib/perf_hooks.js

+// Set up the callback used to receive PerformanceObserver notifications
+function observersCallback(entry) {
+  const type = mapTypes(entry.entryType);
+  const set = getObserversSet(type);


why are we using a Set? Typically iterating over an array is way faster, and overhead matters here.

The set is to ensure that items are only added once. I'm looking in to see if we need to keep that requirement.

mcollina · 2017-08-09T07:25:08Z

lib/perf_hooks.js

+      // If we are, schedule a setImmediate call if one hasn't already
+      if (!observer[kQueued]) {
+        observer[kQueued] = true;
+        setImmediate(doNotify.bind(observer));


why setImmediate and not nextTick? Can you add a comment?

The purpose is to allow multiple items to collect. nextTick fires too quickly for that purpose

mcollina · 2017-08-09T07:27:48Z

lib/perf_hooks.js

+});
+
+module.exports = {
+  performance: new Performance(),


I usually prefer to have top-level functions for singletons, as the module is already a singleton, but feel free to ignore.

do you mean having module.exports = new Performance() instead? The challenge with that is the PerformanceObserver class, which also hangs off module.exports here.

mcollina · 2017-08-25T12:09:37Z

Confirming the LGTM.

BTW, I think also @refack approved this.

An initial implementation of the Performance Timing API for Node.js. This is the same Performance Timing API implemented by modern browsers with a number of Node.js specific properties. The User Timing mark() and measure() APIs are implemented, garbage collection timing, and node startup milestone timing. ```js const { performance } = require('perf_hooks'); performance.mark('A'); setTimeout(() => { performance.mark('B'); performance.measure('A to B', 'A', 'B'); const entry = performance.getEntriesByName('A to B', 'measure')[0]; console.log(entry.duration); }, 10000); ``` The implementation is at the native layer and makes use of uv_hrtime(). This should enable *eventual* integration with things like Tracing and Inspection. The implementation is extensible and should allow us to add new performance entry types as we go (e.g. for measuring i/o perf, etc). Documentation and a test are provided. PR-URL: nodejs/node#14680 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell · 2017-08-25T17:06:54Z

btw, I've opened #15022 to establish a guideline for next time :-)

refack · 2017-08-27T14:51:41Z

@mcollina I had the same concerns as mentioned above about trace events, but those were addressed.
It was my suggestion to make this into a "module" but that was just the scoping solution that made most sense.
IMHO this should be considered a a single API, not really a new module.

As general feedback postmortem:
I have no objections on this PR (+0)
+1 on it being scoped as a module
+1 on it having being marked as experimental
+1 on red X when reviewing Collaborators
+1 for guidelines

An initial implementation of the Performance Timing API for Node.js. This is the same Performance Timing API implemented by modern browsers with a number of Node.js specific properties. The User Timing mark() and measure() APIs are implemented, garbage collection timing, and node startup milestone timing. ```js const { performance } = require('perf_hooks'); performance.mark('A'); setTimeout(() => { performance.mark('B'); performance.measure('A to B', 'A', 'B'); const entry = performance.getEntriesByName('A to B', 'measure')[0]; console.log(entry.duration); }, 10000); ``` The implementation is at the native layer and makes use of uv_hrtime(). This should enable *eventual* integration with things like Tracing and Inspection. The implementation is extensible and should allow us to add new performance entry types as we go (e.g. for measuring i/o perf, etc). Documentation and a test are provided. PR-URL: nodejs/node#14680 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

An initial implementation of the Performance Timing API for Node.js. This is the same Performance Timing API implemented by modern browsers with a number of Node.js specific properties. The User Timing mark() and measure() APIs are implemented, garbage collection timing, and node startup milestone timing. ```js const { performance } = require('perf_hooks'); performance.mark('A'); setTimeout(() => { performance.mark('B'); performance.measure('A to B', 'A', 'B'); const entry = performance.getEntriesByName('A to B', 'measure')[0]; console.log(entry.duration); }, 10000); ``` The implementation is at the native layer and makes use of uv_hrtime(). This should enable *eventual* integration with things like Tracing and Inspection. The implementation is extensible and should allow us to add new performance entry types as we go (e.g. for measuring i/o perf, etc). Documentation and a test are provided. PR-URL: #14680 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Notable Changes * build: * Snapshots are now re-enabled in V8 #14875 * console: * Implement minimal `console.group()`. #14910 * deps: * upgrade libuv to 1.14.1 #14866 * update nghttp2 to v1.25.0 #14955 * dns: * Add `verbatim` option to dns.lookup(). When true, results from the DNS resolver are passed on as-is, without the reshuffling that Node.js otherwise does that puts IPv4 addresses before IPv6 addresses. #14731 * fs: * add fs.copyFile and fs.copyFileSync which allows for more efficient copying of files. #15034 * inspector: * Enable async stack traces #13870 * module: * Add support for ESM. This is currently behind the `--experimental-modules` flag and requires the .mjs extension. `node --experimental-modules index.mjs` #14369 * napi: * implement promise #14365 * os: * Add support for CIDR notation to the output of the networkInterfaces() method. #14307 * perf_hooks: * An initial implementation of the Performance Timing API for Node.js. This is the same Performance Timing API implemented by modern browsers with a number of Node.js specific properties. The User Timing mark() and measure() APIs are implemented, as is a Node.js specific flavor of the Frame Timing for measuring event loop duration. #14680 * tls: * multiple PFX in createSecureContext [#14793](#14793) * Added new collaborators: * BridgeAR – Ruben Bridgewater PR-URL: #15308

Notable Changes * build: * Snapshots are now re-enabled in V8 nodejs#14875 * console: * Implement minimal `console.group()`. nodejs#14910 * deps: * upgrade libuv to 1.14.1 nodejs#14866 * update nghttp2 to v1.25.0 nodejs#14955 * dns: * Add `verbatim` option to dns.lookup(). When true, results from the DNS resolver are passed on as-is, without the reshuffling that Node.js otherwise does that puts IPv4 addresses before IPv6 addresses. nodejs#14731 * fs: * add fs.copyFile and fs.copyFileSync which allows for more efficient copying of files. nodejs#15034 * inspector: * Enable async stack traces nodejs#13870 * module: * Add support for ESM. This is currently behind the `--experimental-modules` flag and requires the .mjs extension. `node --experimental-modules index.mjs` nodejs#14369 * napi: * implement promise nodejs#14365 * os: * Add support for CIDR notation to the output of the networkInterfaces() method. nodejs#14307 * perf_hooks: * An initial implementation of the Performance Timing API for Node.js. This is the same Performance Timing API implemented by modern browsers with a number of Node.js specific properties. The User Timing mark() and measure() APIs are implemented, as is a Node.js specific flavor of the Frame Timing for measuring event loop duration. nodejs#14680 * tls: * multiple PFX in createSecureContext [nodejs#14793](nodejs#14793) * Added new collaborators: * BridgeAR – Ruben Bridgewater PR-URL: nodejs#15308

sam-github · 2017-09-22T21:42:28Z

doc/api/perf_hooks.md

+`PerformanceNodeTiming` class. If the named `endMark` does not exist, an
+error will be thrown.
+
+### performance.nodeFrame


@jasnell I'm not finding this property on the performance object, and I can't find any sign of a PerformanceFrame or a PerformanceNodeFrame classes anywhere other than the docs. Did this not get implemented because it depends on libuv/libuv#1489, or is there something else I am missing?

That should have been removed. I may have forgotten to do so. Yes, the libuv pr needs to land before that can return

The node frame (aka loop) timing API did not land, it depends on libuv/libuv#1489 which is still a WIP. See: nodejs#14680 (comment)

The node frame (aka loop) timing API did not land, it depends on libuv/libuv#1489 which is still a WIP. See: #14680 (comment) PR-URL: #15641 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

The node frame (aka loop) timing API did not land, it depends on libuv/libuv#1489 which is still a WIP. See: nodejs/node#14680 (comment) PR-URL: nodejs/node#15641 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

The node frame (aka loop) timing API did not land, it depends on libuv/libuv#1489 which is still a WIP. See: #14680 (comment) PR-URL: #15641 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

gibfahn · 2018-01-15T22:11:58Z

Release team were -1 on landing on v6.x, if you disagree let us know.

fanatid · 2019-09-03T17:02:42Z

Sorry if this wrong place for ask, but I'm not sure where will be better.
I'm going through perf_hooks and can not find where entryType equal to node used. Is it used at all? If not, why this was added?
https://github.com/fanatid/notes/tree/master/2019-09-12-node-perf-hooks

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 7, 2017

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 7, 2017

jasnell requested review from mcollina and addaleax August 7, 2017 23:15

jasnell force-pushed the performance-api branch from 5bc0b95 to 8a059b9 Compare August 7, 2017 23:21

vsemozhetbyt added the performance Issues and PRs related to the performance of Node.js. label Aug 7, 2017

addaleax reviewed Aug 7, 2017

View reviewed changes

cjihrig reviewed Aug 8, 2017

View reviewed changes

vsemozhetbyt reviewed Aug 8, 2017

View reviewed changes

TimothyGu reviewed Aug 8, 2017

View reviewed changes

hiroppy added the process Issues and PRs related to the process subsystem. label Aug 8, 2017

benjamingr mentioned this pull request Aug 8, 2017

performance should work in Node sinonjs/fake-timers#128

Closed

vsemozhetbyt reviewed Aug 8, 2017

View reviewed changes

jasnell force-pushed the performance-api branch from e33f50c to 7ddf8a8 Compare August 9, 2017 00:23

thefourtheye reviewed Aug 9, 2017

View reviewed changes

mcollina approved these changes Aug 9, 2017

View reviewed changes

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

sam-github reviewed Sep 22, 2017

View reviewed changes

sam-github added a commit to sam-github/node that referenced this pull request Sep 27, 2017

perf_hooks: remove docs for unimplemented API

05e6a59

The node frame (aka loop) timing API did not land, it depends on libuv/libuv#1489 which is still a WIP. See: nodejs#14680 (comment)

sam-github mentioned this pull request Sep 27, 2017

perf_hooks: remove docs for unimplemented API #15641

Closed

4 tasks

gibfahn added the dont-land-on-v6.x label Jan 15, 2018

antonyt mentioned this pull request Jan 23, 2018

doc: Add doc for performance.clearGC() #18331

Closed

2 tasks

Trott mentioned this pull request Oct 6, 2018

Investigate flaky sequential/test-performance #23291

Closed

+              }
+              {
+                let n;

		@@ -4633,6 +4639,7 @@ inline int Start(uv_loop_t* event_loop,

		int Start(int argc, char** argv) {
		atexit([] () { uv_tty_reset_mode(); });


		### Class: process.PerformanceObserver(callback)

		* `callback` {Function}


		* {number}

		The total number of milliseconds ellapsed for this entry. This value will not


		* {number}

		The high resolution millisecond timestamp marking the starting time of the


		* {string}

		The type of the performance entry. Current may be one of: `'node'`, `'mark'`,

perf_hooks: implementation of Performance Timing API #14680

perf_hooks: implementation of Performance Timing API #14680

Conversation

jasnell commented Aug 7, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Trott commented Aug 7, 2017 • edited Loading

mscdex commented Aug 7, 2017

addaleax left a 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

jasnell commented Aug 7, 2017

jasnell commented Aug 7, 2017

mscdex commented Aug 8, 2017

jasnell commented Aug 8, 2017

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

refack commented Aug 8, 2017

TimothyGu left a 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

jasnell commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

jasnell commented Aug 8, 2017

jasnell commented Aug 9, 2017

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

mcollina left a 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

mcollina commented Aug 25, 2017

jasnell commented Aug 25, 2017

refack commented Aug 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn commented Jan 15, 2018

fanatid commented Sep 3, 2019 • edited Loading

jasnell commented Aug 7, 2017 •

edited

Loading

Trott commented Aug 7, 2017 •

edited

Loading

jasnell Aug 8, 2017 •

edited

Loading

fanatid commented Sep 3, 2019 •

edited

Loading