fs: encapsulate FSReqWrap more #18112

jasnell · 2018-01-12T00:26:51Z

In further preparation for the Promises enabled fs API, further encapsulate FSReqWrap details.

This is another bit of code extracted from the WIP fs promises PR: #17739. Pulling this out in order to make the eventual fs promises PR easier to review.

/cc @addaleax @mcollina

Checklist

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

Affected core subsystem(s)

mcollina

LGTM

For completeness, let's do a benchmark run but I don't expect things to show up.

Benchmarks: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/92/

bnoordhuis · 2018-01-12T10:56:01Z

The point of the original code is to keep down the number of allocations. This PR undoes that but I don't understand why and the commit log does a poor job at explaining it.

jasnell · 2018-01-12T15:51:52Z

I'll iterate on this a bit more to reduce the allocations but the motivation is to make the introduction of the promises API easier. Looking at #17739, the idea is to introduce a FSReqPromise alternative to FSReqWrap that implements the same api but uses a Promise rather than callback, without requiring any of the fs specific code to care about which is being used.

bnoordhuis · 2018-01-12T15:57:23Z

src/node_file.cc

 void FSReqWrap::Dispose() {
+  if (info_ != nullptr)
+    info_->Dispose();
  this->~FSReqWrap();


Leaks this, i.e., the FSReqWrap. Should probably be just delete wrap at the call site and move the if (info_ != nullptr) bit to the destructor (and remove this method because it's no longer used after that.)

+1 ... any similar concerns with FSReqInfo::Dispose immediately below it?

bnoordhuis · 2018-01-12T16:00:54Z

src/node_file.h

-  size_t self_size() const override { return sizeof(*this); }
+  void* operator new(size_t size) = delete;
+  void* operator new(size_t size, char* storage) { return storage; }
+  char* inline_data() { return reinterpret_cast<char*>(this + 1); }


Maybe add a CHECK_EQ(data_, reinterpret_cast<char*>(this + 1)) here.

~~actually, I take that back. Adding that CHECK_EQ in here breaks things. I'll investigate further, but I think having the CHECK elsewhere may make more sense.~~

Looking at it, I believe the check actually needs to be:

CHECK_NE(data_, reinterpret_cast<char*>(this + 1));

I assume you mean it breaks because of the memcpy after the placement new?

bnoordhuis · 2018-01-12T16:03:09Z

src/node_file.cc

+  const bool copy = (data != nullptr && ownership == Ownership::COPY);
+  const size_t size = copy ? 1 + strlen(data) : 0;
+  char* const storage = new char[sizeof(*info_) + size];
+  info_ = new(storage) FSReqInfo(syscall, data, encoding);


It's arguably neater and less error prone to move this into FSReqInfo.

My apologies, but I need to ask: move which part into FSReqInfo?

The placement new logic. I.e., make it so there is only one way to create an FSReqInfo instance and make that logic part of the class itself.

jasnell · 2018-01-12T21:21:46Z

@addaleax @bnoordhuis ... ok, reworked this to drop the addition of FSReqInfo entirely to eliminate the extra allocation. Overall greatly simplified.

addaleax

This is still adding the extra allocation, if I read it correctly?

addaleax · 2018-01-12T21:27:20Z

src/node_file.cc

+  const bool copy = (data != nullptr && ownership == Ownership::COPY);
+  if (copy) {
+    size_t size = 1 + strlen(data);
+    data_ = static_cast<char*>(memcpy(Calloc<char>(size), data, size));


Malloc(size) should work just the same and doesn’t require zero-ing out memory that is being overwritten anyway

Also, these two lines are almost exactly reimplementing strdup().

good point about strdup :-)

addaleax · 2018-01-12T21:27:41Z

src/node_file.h

  const char* syscall_;
-  const char* data_;
+  const char* data_ = nullptr;
+  const char* buffer_ = nullptr;


buffer_ and ReleaseEarly() are essentially unused now, as far as I can tell?

Ownership::COPY (and therefore buffer_) is used within the WriteString() function, line 1008.
ReleaseEarly() is used with FSReqAfterScope (see line 157).

As far as I can tell, those are the only current uses and can likely be refactored away at some point but I think I'd rather prefer to avoid doing that in this PR

oh wait... ha! yeah, you're right. hold on a sec...

(it's definitely a friday... brain already disengaged for the weekend)

outdated

jasnell · 2018-01-12T22:02:59Z

@addaleax ... ok, there... should be fixed.

I'm toying with just going ahead and making data_ a std::string tho and just doing away with the Ownership thing entirely tho. What do you think?

bnoordhuis · 2018-01-13T22:39:03Z

src/node_file.cc

-  return that;
+void FSReqWrap::Reject(Local<Value> reject) {
+  Local<Value> argv[1] { reject };
+  MakeCallback(env()->oncomplete_string(), arraysize(argv), argv);


Can just be MakeCallback(env()->oncomplete_string(), 1, &reject);

Yep, may as well. Will do

bnoordhuis · 2018-01-13T22:42:29Z

src/node_file.cc

+  const bool copy = (data != nullptr && ownership == Ownership::COPY);
+  if (copy) {
+    // Setting buffer_ lets us know that we'll need to free on cleanup
+    data_ = buffer_ = strdup(data);


strdup() is almost certainly wrong. That only works for zero-terminated strings whereas data can be arbitrary binary data, as far as I can tell.

Ok yeah, looking at it, I think that current uses of this just end up working with strdup by accident. You're right tho, the code appears to have been originally written to support arbitrary binary data. Will change that back

bnoordhuis · 2018-01-13T22:45:52Z

src/node_file.h

-  const char* data_;
+
+  const char* data_ = nullptr;
+  char* buffer_ = nullptr;


Since you're making changes anyway, I'd change these to std::vector<char> if possible.

jasnell · 2018-01-15T15:17:52Z

@bnoordhuis @addaleax ... updated :-)

jasnell · 2018-01-15T15:18:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12539/

jasnell · 2018-01-15T16:53:48Z

CI is good.

jasnell · 2018-01-16T18:24:48Z

@mcollina @addaleax @bnoordhuis ... I'd like to get this landed so I can move forward with the promises support. PTAL

bnoordhuis · 2018-01-17T11:56:51Z

@jasnell Looks like this needs a rebase first.

jasnell · 2018-01-17T15:43:40Z

heh, yeah, another PR landed shortly after I posted the comment yesterday. Will rebase today :-)

jasnell · 2018-01-17T18:14:34Z

Rebased!

addaleax · 2018-01-17T18:20:39Z

@jasnell Idk, this still LGTM apart from the memory allocation change … can you explain how that improves encapsulation? The benchmark CI here seems to agree with the extra overhead not being a good idea…

jasnell · 2018-01-17T18:25:34Z

Which extra allocation? (Just wanna be sure)

addaleax · 2018-01-17T18:27:23Z

@jasnell If I understand correctly, this patch copies the data in the case of Ownership::COPY into a separately allocated buffer/std::vector instead of appending it directly to the FSReqWrap object … right?

jasnell · 2018-01-17T18:35:44Z

Ah yes. The FSReqWrap object memory was being allocated as sizeof(FSReqWrap) + the additional storage space necessary, then data was being copied in to that extra space. Now, the FSReqWrap is created and then the space to store data is allocated separately if Ownership == COPY.

The idea here is to introduce a variant on FSReqWrap that uses a Promise internally rather than a callback pattern. On the JS side, either a FSReqWrap or FSReqPromise object would be created and passed down to the C++ side, where it would be initialized with whatever data it needed (including the copied data if necessary. This means that the C++ code never actually has to worry about which form is being used -- it just uses whichever one it gets. On the JS side, the only difference between the callback and promise forms of the various function ends up being which FSReq*** object is passed in to the binding.

Previously, what was happening is that the user would create a FSReqWrap JS object, pass that in to the C++ layer, which would then create an internal FSReqWrap C++ instance.

addaleax · 2018-01-17T18:37:51Z

@jasnell Yeah, I get that, but it’s not quite obvious how the change to the allocation model factors in here – why not e.g. store the promise like the callback, as a property on the JS object?

jasnell · 2018-01-17T18:37:54Z

Starting another benchmarking run since this changed fairly significantly since the last run: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/99/

jasnell · 2018-01-17T18:50:43Z

One way we could keep the allocation the same would be to pass the data that is to be copied to the constructor of the FSReqWrap (and eventual FSReqPromise) objects. Let's see what the results of this current benchmark run show first.

jasnell · 2018-01-18T15:27:56Z

New run: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/101/

AndreasMadsen · 2018-01-18T16:03:04Z

@Matteo @jasnell Is this output easier to understand?

You can see that the improvement is only slightly higher than the accuracy, which is calculated with a 5% risk in mind.

                                                                                  improvement accuracy confidence
 fs/bench-readdir.js n=10000                                                          -7.20 %  ±9.94 %
 fs/bench-readdirSync.js n=10000                                                      -0.46 %  ±6.97 %
 fs/bench-realpath.js pathType="relative" n=10000                                      1.62 %  ±4.25 %
 fs/bench-realpath.js pathType="resolved" n=10000                                     -1.68 %  ±3.89 %
 fs/bench-realpathSync.js pathType="relative" n=10000                                  0.72 %  ±5.87 %
 fs/bench-realpathSync.js pathType="resolved" n=10000                                  0.16 %  ±1.61 %
 fs/bench-stat.js statType="fstat" n=200000                                           -2.39 %  ±4.50 %
 fs/bench-stat.js statType="lstat" n=200000                                           -2.94 %  ±5.34 %
 fs/bench-stat.js statType="stat" n=200000                                             2.30 %  ±4.29 %
 fs/bench-statSync.js statSyncType="fstatSync" n=1000000                              -1.15 %  ±5.69 %
 fs/bench-statSync.js statSyncType="lstatSync" n=1000000                               0.47 %  ±2.15 %
 fs/bench-statSync.js statSyncType="statSync" n=1000000                                0.62 %  ±2.99 %
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType="asc"         1.08 %  ±2.96 %
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType="buf"        -1.86 %  ±3.85 %
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType="utf"        -1.09 %  ±2.29 %
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType="asc"     -1.70 %  ±4.55 %
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType="buf"     -0.51 %  ±3.54 %
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType="utf"      0.21 % ±10.19 %
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType="asc"        -1.84 %  ±3.14 %
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType="buf"        -2.14 %  ±4.09 %
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType="utf"         1.17 %  ±3.53 %
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType="asc"        1.25 %  ±7.25 %
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType="buf"        6.19 %  ±5.13 %          *
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType="utf"        2.12 %  ±6.49 %
 fs/readfile.js concurrent=1 len=1024 dur=5                                           -0.33 %  ±3.64 %
 fs/readfile.js concurrent=1 len=16777216 dur=5                                       -3.03 %  ±5.35 %
 fs/readfile.js concurrent=10 len=1024 dur=5                                          -0.98 %  ±2.13 %
 fs/readfile.js concurrent=10 len=16777216 dur=5                                       0.14 %  ±1.72 %
 fs/readFileSync.js n=600000                                                          -4.79 %  ±7.59 %
 fs/write-stream-throughput.js size=1024 encodingType="asc" dur=5                     -1.60 %  ±8.77 %
 fs/write-stream-throughput.js size=1024 encodingType="buf" dur=5                      0.39 % ±12.28 %
 fs/write-stream-throughput.js size=1024 encodingType="utf" dur=5                     -0.77 %  ±5.77 %
 fs/write-stream-throughput.js size=1048576 encodingType="asc" dur=5                   0.18 %  ±0.99 %
 fs/write-stream-throughput.js size=1048576 encodingType="buf" dur=5                  23.42 % ±23.37 %          *
 fs/write-stream-throughput.js size=1048576 encodingType="utf" dur=5                   2.73 %  ±7.07 %
 fs/write-stream-throughput.js size=2 encodingType="asc" dur=5                         0.74 % ±11.24 %
 fs/write-stream-throughput.js size=2 encodingType="buf" dur=5                        -2.84 %  ±5.64 %
 fs/write-stream-throughput.js size=2 encodingType="utf" dur=5                       -15.35 % ±14.18 %          *
 fs/write-stream-throughput.js size=65535 encodingType="asc" dur=5                    -3.07 % ±10.67 %
 fs/write-stream-throughput.js size=65535 encodingType="buf" dur=5                   -12.87 % ±11.94 %          *
 fs/write-stream-throughput.js size=65535 encodingType="utf" dur=5                     3.28 %  ±9.81 %

mcollina · 2018-01-18T16:06:33Z

Yes it is!

jasnell · 2018-01-18T16:08:33Z

oh, I like that. @AndreasMadsen ... how easy would it be to modify benchmark/compare.R to give us that output?

AndreasMadsen · 2018-01-18T16:09:55Z

@jasnell It is easy. I have a local version of compare.R that does that. But it needs some cleanup :D

I have been tinkering with it ever since I discovered I was closing in on 100 commits :p

jasnell · 2018-01-18T16:30:34Z

CI looks good with only unrelated failures.

bnoordhuis

LGTM with some comments.

bnoordhuis · 2018-01-18T18:59:28Z

src/node_file.cc

+    memcpy(*buffer_, data, len);
+    data_ = *buffer_;
+  } else {
+    data_ = nullptr;


Superfluous assignment, implied by the CHECK_EQ().

tbh, the CHECK_EQ really should go inside the if block. Either way, this assignment is superfluous.

bnoordhuis · 2018-01-18T19:09:59Z

src/node_file.h

@@ -6,6 +6,8 @@
 #include "node.h"
 #include "req_wrap-inl.h"

+#include <vector>


Unused include.

bnoordhuis · 2018-01-18T19:10:32Z

src/node_file.h

-      data_ = nullptr;
-    }
-  }
+  virtual void ResolveStat();


Why are these methods virtual?

because the plan is to have FSReqPromise just extend from FSReqWrap and override those. That may change tho.

jasnell · 2018-01-18T21:02:50Z

@mcollina ... here are the results from the additional benchmark run...

                                                                                  improvement confidence    p.value
 fs/bench-readdir.js n=10000                                                          -0.94 %            0.84214689
 fs/bench-readdirSync.js n=10000                                                       2.10 %            0.36945312
 fs/bench-realpath.js pathType="relative" n=10000                                     -0.40 %            0.86629651
 fs/bench-realpath.js pathType="resolved" n=10000                                      1.47 %            0.50400409
 fs/bench-realpathSync.js pathType="relative" n=10000                                 -2.48 %            0.37638238
 fs/bench-realpathSync.js pathType="resolved" n=10000                                 -3.51 %            0.08741750
 fs/bench-stat.js statType="fstat" n=200000                                            1.91 %            0.39188037
 fs/bench-stat.js statType="lstat" n=200000                                            2.56 %            0.33751086
 fs/bench-stat.js statType="stat" n=200000                                            -3.82 %            0.11781119
 fs/bench-statSync.js statSyncType="fstatSync" n=1000000                              -1.41 %            0.65241462
 fs/bench-statSync.js statSyncType="lstatSync" n=1000000                              -1.20 %            0.21651357
 fs/bench-statSync.js statSyncType="statSync" n=1000000                                0.57 %            0.66756758
 fs/readfile.js concurrent=10 len=1024 dur=5                                          -0.84 %            0.43322526
 fs/readfile.js concurrent=10 len=16777216 dur=5                                       0.44 %            0.57766034
 fs/readfile.js concurrent=1 len=1024 dur=5                                            2.95 %          * 0.04407371
 fs/readfile.js concurrent=1 len=16777216 dur=5                                       -4.06 %            0.12604256
 fs/readFileSync.js n=600000                                                          -1.24 %            0.70076115
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType="asc"        -1.30 %            0.52916466
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType="buf"         2.52 %            0.28432425
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType="utf"        -1.21 %            0.31575345
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType="asc"     -0.95 %            0.57938871
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType="buf"      3.04 %            0.13517822
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType="utf"      1.46 %            0.74769224
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType="asc"         3.24 %            0.20716191
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType="buf"        -0.52 %            0.87752472
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType="utf"        -0.08 %            0.95162735
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType="asc"        4.24 %            0.28883775
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType="buf"        2.46 %            0.37526953
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType="utf"       -2.04 %            0.26334893
 fs/write-stream-throughput.js size=1024 encodingType="asc" dur=5                      8.57 %          * 0.03647149
 fs/write-stream-throughput.js size=1024 encodingType="buf" dur=5                      1.00 %            0.85881673
 fs/write-stream-throughput.js size=1024 encodingType="utf" dur=5                      1.44 %            0.51909346
 fs/write-stream-throughput.js size=1048576 encodingType="asc" dur=5                   4.77 %            0.32786814
 fs/write-stream-throughput.js size=1048576 encodingType="buf" dur=5                  -2.17 %            0.82563561
 fs/write-stream-throughput.js size=1048576 encodingType="utf" dur=5                  -3.27 %            0.23733300
 fs/write-stream-throughput.js size=2 encodingType="asc" dur=5                        16.80 %          * 0.01396570
 fs/write-stream-throughput.js size=2 encodingType="buf" dur=5                        -1.65 %            0.60227783
 fs/write-stream-throughput.js size=2 encodingType="utf" dur=5                        -8.70 %            0.23147447
 fs/write-stream-throughput.js size=65535 encodingType="asc" dur=5                     1.45 %            0.81105372
 fs/write-stream-throughput.js size=65535 encodingType="buf" dur=5                    -8.16 %            0.09510397
 fs/write-stream-throughput.js size=65535 encodingType="utf" dur=5                     3.66 %            0.46634498

jasnell · 2018-01-18T21:03:53Z

The variance in the throughput benchmarks from run to run do, in fact, appear to be gc related.

mcollina

LGTM

In further preparation for the Promises enabled fs API, further encapsulate FSReqWrap details. The intent here is to isolate, as much as possible, the functionality of the various fs functions from the details of how the results are notified. The promises implementation will use a `FSReqPromise` alternative to `FSReqWrap` that will use the same API so that both models can be used without changing any of the actual implementation details for the various methods.

jasnell · 2018-01-18T21:14:08Z

Ping @addaleax ... you good with this now also?

jasnell · 2018-01-18T21:14:49Z

One more post-squash post-nit-fix CI run: https://ci.nodejs.org/job/node-test-pull-request/12614/

addaleax · 2018-01-18T21:17:41Z

@jasnell Yeah, the code changes LGTM – it’s still not really clear to me how this is going to help, but it also doesn’t really make anything worse, so I’m good here. :)

jasnell · 2018-01-18T21:37:15Z

@addaleax ... hopefully that will be more apparent in the follow on PR. The approach is largely to make handling the Promise easier and to avoid an extra boundary cross to create the Promise.

In further preparation for the Promises enabled fs API, further encapsulate FSReqWrap details. The intent here is to isolate, as much as possible, the functionality of the various fs functions from the details of how the results are notified. The promises implementation will use a `FSReqPromise` alternative to `FSReqWrap` that will use the same API so that both models can be used without changing any of the actual implementation details for the various methods. PR-URL: #18112 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

jasnell · 2018-01-18T22:26:48Z

Landed in 4b9ba9b

MylesBorins · 2018-02-20T22:10:23Z

Does this make sense for v9.x? If so should it will need to be backported

In further preparation for the Promises enabled fs API, further encapsulate FSReqWrap details. The intent here is to isolate, as much as possible, the functionality of the various fs functions from the details of how the results are notified. The promises implementation will use a `FSReqPromise` alternative to `FSReqWrap` that will use the same API so that both models can be used without changing any of the actual implementation details for the various methods. PR-URL: nodejs#18112 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

jasnell added the fs Issues and PRs related to the fs subsystem / file system. label Jan 12, 2018

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. labels Jan 12, 2018

addaleax previously approved these changes Jan 12, 2018

View reviewed changes

mcollina approved these changes Jan 12, 2018

View reviewed changes

bnoordhuis reviewed Jan 12, 2018

View reviewed changes

addaleax reviewed Jan 12, 2018

View reviewed changes

jasnell force-pushed the fs-ref-reqwrap branch from 58ae07d to 8346f85 Compare January 12, 2018 22:00

bnoordhuis reviewed Jan 13, 2018

View reviewed changes

jasnell force-pushed the fs-ref-reqwrap branch from 8346f85 to d3e5477 Compare January 15, 2018 15:17

jasnell force-pushed the fs-ref-reqwrap branch from d3e5477 to ad908da Compare January 17, 2018 18:14

bnoordhuis approved these changes Jan 18, 2018

View reviewed changes

mcollina approved these changes Jan 18, 2018

View reviewed changes

jasnell force-pushed the fs-ref-reqwrap branch from 1d855cc to fa78aa5 Compare January 18, 2018 21:08

addaleax approved these changes Jan 18, 2018

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 18, 2018

jasnell closed this Jan 18, 2018

AndreasMadsen mentioned this pull request Jan 25, 2018

benchmark: make compare.R easier to understand #18373

Closed

2 tasks

MylesBorins added the backport-requested-v9.x label Feb 20, 2018

joyeecheung mentioned this pull request Mar 20, 2018

fs: fix memory leak in WriteString #19357

Closed

2 tasks

addaleax added dont-land-on-v8.x and removed backport-requested-v9.x labels Jun 29, 2018

fs: encapsulate FSReqWrap more #18112

fs: encapsulate FSReqWrap more #18112

Conversation

jasnell commented Jan 12, 2018 • edited Loading

Checklist

Affected core subsystem(s)

mcollina left a comment

Choose a reason for hiding this comment

bnoordhuis commented Jan 12, 2018

jasnell commented Jan 12, 2018

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 Jan 12, 2018 • 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

jasnell commented Jan 12, 2018

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 Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

jasnell commented Jan 12, 2018

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 Jan 15, 2018

jasnell commented Jan 15, 2018

jasnell commented Jan 15, 2018

jasnell commented Jan 16, 2018

bnoordhuis commented Jan 17, 2018

jasnell commented Jan 17, 2018

jasnell commented Jan 17, 2018

addaleax commented Jan 17, 2018

jasnell commented Jan 17, 2018

addaleax commented Jan 17, 2018

jasnell commented Jan 17, 2018

addaleax commented Jan 17, 2018

jasnell commented Jan 17, 2018

jasnell commented Jan 17, 2018

jasnell commented Jan 18, 2018

AndreasMadsen commented Jan 18, 2018

mcollina commented Jan 18, 2018

jasnell commented Jan 18, 2018

AndreasMadsen commented Jan 18, 2018

jasnell commented Jan 18, 2018

bnoordhuis 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

jasnell commented Jan 18, 2018

jasnell commented Jan 18, 2018

mcollina left a comment

Choose a reason for hiding this comment

jasnell commented Jan 18, 2018

jasnell commented Jan 18, 2018

addaleax commented Jan 18, 2018

jasnell commented Jan 18, 2018

jasnell commented Jan 18, 2018

MylesBorins commented Feb 20, 2018

jasnell commented Jan 12, 2018 •

edited

Loading

jasnell Jan 12, 2018 •

edited

Loading

jasnell Jan 12, 2018 •

edited

Loading