finagle-base-http: add MapBackedHeaderMap #805

martijnhoekstra · 2019-10-04T08:13:33Z

Problem

The default headermap implementation is not future-proof, and relies on HashMap internals that will change in scala 2.13

Solution

Provide an alternative headermap implemtation that is backed by Map, an interface that will work across versions.

Result

The option to use a 2.13-proof headermap.

martijnhoekstra · 2019-10-04T08:14:47Z

Needs to be benchmarked. I'll cherry-pick the benchmark extensions from https://github.com/twitter/finagle/pull/797/files#diff-b61dcd2fdf0b7a577f018c14e2d83800 into this, and we can bench-off

martijnhoekstra · 2019-10-04T09:59:52Z

flaky test error presumed spurious

martijnhoekstra · 2019-10-04T12:57:47Z

https://docs.google.com/spreadsheets/d/1C402zjjqmEhC-Aw5LOf_XwzjYUFcPX1XT0kI-B_7lVQ/edit?usp=sharing has benchmarks (while laptop wasn't fully idle, so a bit dirty).

get takes four times as long in this impl vs baseline. How that reflects to real world performance is anybodies guess without macrobenchmarking/end-to-end tests

mosesn · 2019-10-04T15:17:51Z

Thanks! I really appreciate you breaking up the patch, it's much easier to read now.

mosesn · 2019-10-04T15:40:21Z

How does it work if it's backed by a java hashmap? I wonder if that will make it easier to avoid allocs etc

martijnhoekstra · 2019-10-04T15:53:30Z

I can run another bench backed with a java hashmap, but if we want real insights in allocs, we should run under JFR, and analyse those results. I never did that before though.

I'm also not too sure the performance bottleneck hinges on allocation. That's also something that JFR dumps or flamegraphs could help us out more with than looking at microbenchmarks and guessing.

This weekend my volunteer/life balance will favour the living, so if there will be anything from me, it'll be after the weekend.

mosesn · 2019-10-04T16:50:48Z

sgtm, no rush! jmh has an alloc mode also, I think it's -prof gc

martijnhoekstra · 2019-10-04T17:25:33Z

Also related, the 2.13 Map implementation changed for performance reasons, and should be faster than the 2.12 Map. If performance of the 2.13 one is good (that's a big if, but still), you could just keep the current impl as the default for 2.12. It's just a matter of making what HeaderMap.apply returns version specific.

bryce-anderson · 2019-10-04T18:04:22Z

I have an idea along the lines moses is suggesting that might be worth trying. We should try using a Java collection as the backing collection but use the java.util.TreeMap which allows you to provide a Comparitor[String] that is not case sensitive. For example this should work and be pretty snappy:

val SharedComparitor = new Comparator[String] {
  def compare(o1: String, o2: String): Int = {
    // Shorter strings are always less, regardless of their content
    val lenthDiff = key1.length - key2.length
    if (lenthDiff != 0) lenthDiff
    else {
      @tailrec
      def go(i: Int): Int = {
        if (i == key1.length) 0 // end, they are equal.
        else {
          val char1 = hashChar(key1.charAt(i))
          val char2 = hashChar(key2.charAt(i))
          val diff = char1 - char2
          if (diff == 0) go(i + 1)
          else diff
        }
      }
      go(0)
    }
  }

There's a really solid chance this would be faster than what is currently in master.

martijnhoekstra · 2019-10-04T18:43:18Z

Sounds good! I also still want to give the trie approach another go. I also want to understand why the current approach is so much faster in finding an entry than the scala map backed approach is.

bryce-anderson · 2019-10-04T18:56:15Z

I'm general, String.toLowerCase is really expensive both in terms of course and allocations, so that's probably where you're losing perf.

…

On Fri, Oct 4, 2019, 12:43 PM Martijn Hoekstra ***@***.***> wrote: Sounds good! I also still want to give the trie approach another go. I also want to understand why the current approach is so much faster in finding an entry than the scala map backed approach is. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#805?email_source=notifications&email_token=AAWP3GU3K6IYLTFN76WHBSDQM6FFRA5CNFSM4I5MU742YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAMRV7A#issuecomment-538516220>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAWP3GQDZ3J634VH53QSOQLQM6FFRANCNFSM4I5MU74Q> .

martijnhoekstra · 2019-10-04T21:24:59Z

That sounds plausible. For the plain tree version I'm afraid to lose a lot of performance on traversing a common prefix several times (e.g. content-*). At least it'll be good to have options to compare.

martijnhoekstra · 2019-10-07T09:23:30Z

Some more benchmark results at this google doc (ran with too few iterations for too short for real results) indicate that the java tree map approach so far is indeed comes pretty close to the implementation master.

I'd propose going forward replacing the implementation in master with the java tree map backed version for all versions.

codecov-io · 2019-10-07T11:34:58Z

Codecov Report

Merging #805 into develop will increase coverage by 0.14%.
The diff coverage is 1.6%.

@@             Coverage Diff             @@
##           develop     #805      +/-   ##
===========================================
+ Coverage    68.27%   68.41%   +0.14%     
===========================================
  Files          844      841       -3     
  Lines        24839    23821    -1018     
  Branches      1833     1854      +21     
===========================================
- Hits         16958    16297     -661     
+ Misses        7881     7524     -357

Impacted Files	Coverage Δ
...tter/finagle/http/headers/MapBackedHeaderMap.scala	`0% <0%> (ø)`
...ter/finagle/http/headers/JMapBackedHeaderMap.scala	`0% <0%> (ø)`
...cala/com/twitter/finagle/http/headers/Header.scala	`46.15% <0%> (-10.99%)`	⬇️
.../com/twitter/finagle/http/HeaderMapBenchmark.scala	`0% <0%> (ø)`	⬆️
...finagle/http/headers/JTreeMapBackedHeaderMap.scala	`0% <0%> (ø)`
...com/twitter/finagle/http/headers/HeadersHash.scala	`79.31% <100%> (-20.69%)`	⬇️
.../twitter/finagle/netty4/http/Netty4HttpCodec.scala	`0% <0%> (-100%)`	⬇️
...com/twitter/finagle/http/service/NullService.scala	`0% <0%> (-100%)`	⬇️
...http2/transport/client/Http2ClientDispatcher.scala	`0% <0%> (-100%)`	⬇️
...finagle/http/filter/AddResponseHeadersFilter.scala	`0% <0%> (-100%)`	⬇️
... and 172 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32bc9f4...60e8df5. Read the comment docs.

bryce-anderson

I agree that the j.u.TreeMap approach looks promising, promising enough to become the main implementation.

finagle-base-http/src/main/scala/com/twitter/finagle/http/headers/JMapBackedHeaderMap.scala

finagle-base-http/src/main/scala/com/twitter/finagle/http/headers/MapBackedHeaderMap.scala

bryce-anderson · 2019-10-08T19:57:54Z

@martijnhoekstra, I just wanted to give you a heads up, but I'm working on some optimizations in Headers.scala. It looks like conflicts will be minimal, but just wanted you to know.

martijnhoekstra · 2019-10-08T20:10:54Z

Thanks for letting me know, I'm sure the conflicts won't be a big deal.

martijnhoekstra · 2019-10-09T08:38:54Z

addressed the feedback, and rebased for the Header changes

martijnhoekstra · 2019-10-11T08:01:12Z

@bryce-anderson I think I got them all. Do you have any additional issues?

bryce-anderson

Sorry it took so long to check this out, I must have missed it in my inbox.
This all looks really good, pretty much just nit-picks for performance etc. Thanks for taking care of this!

bryce-anderson · 2019-10-15T15:24:00Z

finagle-base-http/src/main/scala/com/twitter/finagle/http/headers/Header.scala

+    def iterator: Iterator[HeaderMap.NameValue] =
+      if (next == null) Iterator.single(this)
+      else {
+        var cur: Header = this


If you move cur into the definition of AbstractIterator you can avoid lifting it to the the heap via scala.runtime.ObjectRef.

bryce-anderson · 2019-10-15T15:26:42Z

finagle-base-http/src/main/scala/com/twitter/finagle/http/headers/JMapBackedHeaderMap.scala

+  final def iterator: Iterator[(String, String)] = underlying.synchronized {
+    copyHeaders.flatMap(_.iterator.map(nv => (nv.name, nv.value)))
+  }


This looks like nameValueIterator.map(nv => (nv.name, nv.value)).

bryce-anderson · 2019-10-15T15:27:55Z

finagle-base-http/src/main/scala/com/twitter/finagle/http/headers/JMapBackedHeaderMap.scala

+ * a mutable [[Map[String, Header]]], where the map key
+ * is forced to lower case
+ */
+private[http] trait JMapBackedHeaderMap extends HeaderMap {


This feels like it should be an abstract class, and probably just vanilla private.

bryce-anderson · 2019-10-15T15:34:55Z

finagle-base-http/src/main/scala/com/twitter/finagle/http/headers/JTreeMapBackedHeaderMap.scala

+
+    def getAll(key: String): Seq[String] = underlying.synchronized {
+      underlying.get(key.toLowerCase) match {
+        case null => Nil
+        case r: Header.Root => r.values
+      }
+    }


Indentation is off.

bryce-anderson · 2019-10-15T15:37:01Z

finagle-base-http/src/main/scala/com/twitter/finagle/http/headers/JTreeMapBackedHeaderMap.scala

+ * a mutable [[Map[String, Header]]], where the map key
+ * is forced to lower case
+ */
+final private[http] class JTreeMapBackedHeaderMap extends JMapBackedHeaderMap {


What is the value of separating this from JMapBackedHeaderMap? I don't anticipate we'll want to change the underlying map all that often.

fwiw, I'm not strongly opinionated about this, it's just that having an abstract backing map that is synchronized on in two different traits/classes makes me queasy.

It made sense when the HashMap variety still existed. Not so much anymore.

bryce-anderson · 2019-10-15T15:37:43Z

finagle-base-http/src/main/scala/com/twitter/finagle/http/headers/JTreeMapBackedHeaderMap.scala

+
+  // Does not validate key and value.
+  def addUnsafe(key: String, value: String): this.type  = underlying.synchronized {
+    def header = Header.root(key, value)


header is only needed if we hit the null case. Can we move its instantiation there?

It's a def for that reason, but it can go inside the match. The only reason it's outside now is neater indentation, so that's not much of a reason.

Oh, I didn't notice it was a def. :) Maybe just inline it where header is used and we'll both be happy.

As an aside, do you know if that def allocates a lambda, or does that get lifted out into the object itself?

Yes, I inlined it.

As for the allocation, I assumed it gets hoysted to a method all the way to the top, especially since it doesn't capture anything and doesn't escape scope, but I'm not 100% sure either way.

bryce-anderson · 2019-10-15T15:38:27Z

finagle-base-http/src/main/scala/com/twitter/finagle/http/headers/JTreeMapBackedHeaderMap.scala

+  }
+
+  def get(key: String): Option[String] = underlying.synchronized {
+    Option(underlying.get(key)).map(_.value)


Let's do a match to avoid allocating a second Some and the lambda used to map.

bryce-anderson

Only one more piece of feedback, but this looks really good! Thanks again for tackling it.

bryce-anderson · 2019-10-15T18:40:25Z

finagle-base-http/src/main/scala/com/twitter/finagle/http/headers/JTreeMapBackedHeaderMap.scala

+  override def keysIterator: Iterator[String] = underlying.synchronized {
+    //the common case has a single element in Headers. Prevent unneeded List
+    //allocations for that case (don't flatMap)
+    val valuesIterator = copyHeaders
+    var currentEntries: Iterator[String] = Iterator.empty
+    new AbstractIterator[String]{
+      def hasNext: Boolean = currentEntries.hasNext || valuesIterator.hasNext
+      def next(): String =
+        if (currentEntries.hasNext) currentEntries.next()
+        else {
+          val h = valuesIterator.next()
+          if (h.next == null) h.name
+          else {
+            currentEntries = h.iterator.map(nv => nv.name).toList.distinct.iterator
+            currentEntries.next()
+          }
+        }
+    }
+  }


This looks pretty similar in result to HeadersHash.uniqueNamesIterator. Could we share that logic?

I factored the (lazier) logic of HeaderHash out, and made both call that.

martijnhoekstra · 2019-10-15T21:22:08Z

CI failures look spurious

mosesn

I think this looks pretty much done except we need to work out this concurrentmodification thing. LGTM otherwise!

finagle-base-http/src/main/scala/com/twitter/finagle/http/headers/JTreeMapBackedHeaderMap.scala

mosesn · 2019-10-17T15:18:19Z

finagle-base-http/src/main/scala/com/twitter/finagle/http/headers/JTreeMapBackedHeaderMap.scala

+
+  /**
+   * Underlying headers eagerly copied to an array, without synchronizing
+   * on the underlying collection.


maybe add a note that since it's not synchronized, it must be synchronized by the calling method? I think it might be safe to always synchronize here, since java synchronized does reentrancy pretty cheaply (I think)

I added the note -- I also think that re-entrance is pretty cheap, but I wasn't entirely sure, and it's called only a handful of times, and only private[this] I'm happy to change it if you see those trade-offs differently.

mosesn · 2019-10-17T15:23:03Z

finagle-base-http/src/main/scala/com/twitter/finagle/http/headers/JTreeMapBackedHeaderMap.scala

+      )
+    }
+
+  override def foreach[U](f: ((String, String)) => U): Unit = 


I was talking to @bryce-anderson and we think this might open us to ConcurrentModificationException issues, since we don't synchronize while iterating, and it iterates over the underlying map directly. it seems like we might have a few options here:

instead of TreeMap use a ConcurrentSkipListMap

synchronize foreach on underlying

remove the optimization, instead synchronize and copy the iterator when we call foreach

The risk of synchronizing foreach is that we're going to lock on someone else's lambda, but it might be worth just doing it for now, since it will still let us use your optimization at very little cost in the common case (single-threaded access). On the other hand, it would be pretty cool to use a ConcurrentSkipListMap 😈. how do you feel about it?

Now I wish I never removed the abstraction to swap out the underlying Map implementation.

But let's take a step back. How often do people really concurrently iterate over headers and also mutate the header map and don't synchronize that access because they don't care whether or not the modified data should or shouldn't be part of the iteration? Is that a use-case you want to facilitate in the first place?

Thinking about those questions, my conclusion is that throwing a ConcurrentModificationException that makes the user aware of the situation/race condition/potential bug is better than synchronizing and not being deterministic in what happens first. If the user really wants that for some unforeseen, unspoken and unspeakable reason, they can always still externally synchronize (maybe with a ReadWriteLock or something).

I think in terms of common usage patterns, that's the fastest and most reasonable thing to do too. Even if a ConcurrentSkipListMap would be pretty cool.

If we do want to get overboard and fancy, it should be validated with benchmarks that reflect real-world usage patterns, including the real-world concurrent patterns, because otherwise we're not really measuring anything close to reality.

I think the good outcome is that they get a ConcurrentModificationException–a bad outcome might be an infinite loop or worse. See https://ivoanjo.me/blog/2018/07/21/writing-to-a-java-treemap-concurrently-can-lead-to-an-infinite-loop-during-reads/

I think correctness needs to be our #1 concern. I agree with you that it's extremely unlikely–that's one of the reasons I'm willing to just wrap it in a synchronized block and call it a day.

I'm not sure I understand your concern about not being deterministic about what happens first–that's always going to be the case with concurrent use of a data structure. My concern is that HeaderMap used to be threadsafe, so people may have been already using it assuming it was threadsafe, and then one day we merge this in and their application breaks in an extremely subtle way. If this was a new abstraction, I would be open to considering, "HeaderMap is not threadsafe, all concurrent access must be synchronized by the user" but I don't think that's the right way forward given the circumstances.

My bet is the same as yours, that it's extremely unusual to access it concurrently, so I don't think we need to benchmark it. But I do think that we should protect against it. Locking is pretty cheap when there's no concurrent access, so I don't think it should be that painful to just lock it.

Jikes! I didn't realize the concurrent case could lead to non-termination. That makes not synchronizing a non-starter, and the below just an aside for academic curiosity.

The concern about concurrent iteration and modification (specifically those two) is that the concept of iteration of all headers and concurrently modifying those headers makes no sense -- either you want those in or out, but you always care, or you wouldn't iteratate them. You must synchronise externally for the operation to be meaningful and correct.

That's not always the case for two concurrent modifications that will never modify the same key, or concurrently getting a key, and modifying some keys that you know to be different to the keys you're concurrently getting.

I've run into this non-termination before... It was a nightmare, especially considering it was happening on a Netty thread.

Normally I'd agree that we'd prefer a ConcurrentModificationException and we've tried that in the past but many users are doing the wrong thing in benign places (think reusing a request to do warmup requests or something) and it just wasn't worth the hassle to introduce the behavior change.

I think the solution you've already committed of simply synchronizing the foreach call is fine since you're right: this shouldn't be happening very often, if at all. 🤞

vkostyukov

This looks pretty great @martijnhoekstra! Any chance you can attach benchmark results to the PR description?

finagle-base-http/src/main/scala/com/twitter/finagle/http/headers/Header.scala

finagle-base-http/src/main/scala/com/twitter/finagle/http/headers/JTreeMapBackedHeaderMap.scala

martijnhoekstra · 2019-10-17T17:43:06Z

This looks pretty great @martijnhoekstra! Any chance you can attach benchmark results to the PR description?

Shall I re-run after I do the last round of feedback (and we know what to do with potentially concurrent modification and iteration?)

martijnhoekstra · 2019-10-17T18:26:35Z

quick ( -i 5 -wi 3 -r 1 -w 5) benchmark run:

[info] Benchmark                                          Mode  Cnt     Score     Error  Units
[info] HashBackedHeaderMapBenchmark.create                avgt    5    58.571 ▒  18.232  ns/op
[info] HashBackedHeaderMapBenchmark.createAndAdd          avgt    5   148.290 ▒  28.821  ns/op
[info] HashBackedHeaderMapBenchmark.createAndAdd24        avgt    5  3435.482 ▒ 594.920  ns/op
[info] HashBackedHeaderMapBenchmark.getRandom             avgt    5    28.593 ▒   5.335  ns/op
[info] HashBackedHeaderMapBenchmark.getRealistic          avgt    5    26.842 ▒   2.421  ns/op
[info] HashBackedHeaderMapBenchmark.iterate               avgt    5   265.780 ▒  56.199  ns/op
[info] HashBackedHeaderMapBenchmark.iterateKeys           avgt    5   142.899 ▒  19.002  ns/op
[info] HashBackedHeaderMapBenchmark.iterateRepeatedKeys   avgt    5   109.857 ▒  48.143  ns/op
[info] HashBackedHeaderMapBenchmark.iterateValues         avgt    5   196.082 ▒ 113.488  ns/op
[info] HashBackedHeaderMapBenchmark.keySet                avgt    5     5.904 ▒   1.852  ns/op
[info] JTMapBackedHeaderMapBenchmark.create               avgt    5    15.710 ▒   3.428  ns/op
[info] JTMapBackedHeaderMapBenchmark.createAndAdd         avgt    5    78.716 ▒  18.136  ns/op
[info] JTMapBackedHeaderMapBenchmark.createAndAdd24       avgt    5  2242.704 ▒ 450.955  ns/op
[info] JTMapBackedHeaderMapBenchmark.getRandom            avgt    5    41.988 ▒   6.247  ns/op
[info] JTMapBackedHeaderMapBenchmark.getRealistic         avgt    5    41.815 ▒  24.553  ns/op
[info] JTMapBackedHeaderMapBenchmark.iterate              avgt    5   164.154 ▒  64.958  ns/op
[info] JTMapBackedHeaderMapBenchmark.iterateKeys          avgt    5   141.076 ▒  34.300  ns/op
[info] JTMapBackedHeaderMapBenchmark.iterateRepeatedKeys  avgt    5   110.051 ▒  32.983  ns/op
[info] JTMapBackedHeaderMapBenchmark.iterateValues        avgt    5   353.827 ▒  40.716  ns/op
[info] JTMapBackedHeaderMapBenchmark.keySet               avgt    5     6.344 ▒   2.265  ns/op

mosesn

this is really great, thanks so much @martijnhoekstra! really excited to start merging this in.

mosesn · 2019-11-04T02:34:17Z

@martijnhoekstra the code for this was merged in 193c603, but we cut it back over to the old impl for now, so that we can do some performance tests before switching over entirely. I hope you're partially unblocked for now

mosesn · 2019-11-07T22:32:41Z

@martijnhoekstra we found in someone else's test suite a use case where someone calls something like headerMap.foreach { case (k, v) => headerMap.add(k, ....) } which of course triggers a ConcurrentModificationException . . . so we're going to have to copy the iterator anyway. :/ Sorry about that, we'll be merging it in soon though!

martijnhoekstra · 2019-11-08T08:57:34Z

https://xkcd.com/1172/

enbnt · 2019-11-25T22:40:23Z

This branch was merged 193c603#diff-9cea8690cac77728b4a871d2c3eb222c. Just circling back now to close out the PR.

Thank you, @martijnhoekstra!

martijnhoekstra changed the title ~~add MapBackedHeaderMap~~ finagle-base-http: add MapBackedHeaderMap Oct 4, 2019

martijnhoekstra force-pushed the mapBackedHeaderMap branch from da81380 to e1ba3f7 Compare October 4, 2019 08:34

martijnhoekstra mentioned this pull request Oct 4, 2019

base-http: -> 2.13 #797

Closed

martijnhoekstra force-pushed the mapBackedHeaderMap branch 3 times, most recently from b0ebd3f to 60e8df5 Compare October 7, 2019 10:57

bryce-anderson reviewed Oct 7, 2019

View reviewed changes

martijnhoekstra force-pushed the mapBackedHeaderMap branch from 60e8df5 to 91ce8d3 Compare October 9, 2019 08:27

martijnhoekstra force-pushed the mapBackedHeaderMap branch 2 times, most recently from d5d7a03 to 7dae34c Compare October 10, 2019 08:05

Martijn Hoekstra added 3 commits October 10, 2019 12:39

add MapBackedHeaderMap

7c009f6

update benchmarks

00280d9

java map backed versions

2f1d9d3

Martijn Hoekstra added 2 commits October 10, 2019 12:42

remove unused HeaderMap implementations

801ac60

address PR feedback

69864fd

martijnhoekstra force-pushed the mapBackedHeaderMap branch from 7dae34c to 69864fd Compare October 10, 2019 11:39

bryce-anderson reviewed Oct 15, 2019

View reviewed changes

address PR feedback

c141122

bryce-anderson approved these changes Oct 15, 2019

View reviewed changes

centralize unique named iterator

90efed3

martijnhoekstra force-pushed the mapBackedHeaderMap branch from 0d8d5ba to 90efed3 Compare October 15, 2019 19:07

mosesn reviewed Oct 17, 2019

View reviewed changes

vkostyukov reviewed Oct 17, 2019

View reviewed changes

finagle-base-http/src/main/scala/com/twitter/finagle/http/headers/Header.scala Outdated Show resolved Hide resolved

finagle-base-http/src/main/scala/com/twitter/finagle/http/headers/JTreeMapBackedHeaderMap.scala Outdated Show resolved Hide resolved

Martijn Hoekstra added 2 commits October 17, 2019 20:29

address PR feedback

f1d0fc1

synchronize on iteration

de48cd9

mosesn approved these changes Oct 18, 2019

View reviewed changes

mosesn added the Ship It label Oct 18, 2019

enbnt closed this Nov 25, 2019

finagle-base-http: add MapBackedHeaderMap #805

finagle-base-http: add MapBackedHeaderMap #805

Conversation

martijnhoekstra commented Oct 4, 2019

martijnhoekstra commented Oct 4, 2019

martijnhoekstra commented Oct 4, 2019

martijnhoekstra commented Oct 4, 2019

mosesn commented Oct 4, 2019

mosesn commented Oct 4, 2019

martijnhoekstra commented Oct 4, 2019

mosesn commented Oct 4, 2019

martijnhoekstra commented Oct 4, 2019

bryce-anderson commented Oct 4, 2019

martijnhoekstra commented Oct 4, 2019

bryce-anderson commented Oct 4, 2019 via email

martijnhoekstra commented Oct 4, 2019

martijnhoekstra commented Oct 7, 2019 • edited

codecov-io commented Oct 7, 2019

Codecov Report

bryce-anderson left a comment

Choose a reason for hiding this comment

bryce-anderson commented Oct 8, 2019

martijnhoekstra commented Oct 8, 2019

martijnhoekstra commented Oct 9, 2019

martijnhoekstra commented Oct 11, 2019

bryce-anderson 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnhoekstra Oct 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryce-anderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnhoekstra commented Oct 15, 2019

mosesn 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

vkostyukov left a comment

Choose a reason for hiding this comment

martijnhoekstra commented Oct 17, 2019

martijnhoekstra commented Oct 17, 2019

mosesn left a comment

Choose a reason for hiding this comment

mosesn commented Nov 4, 2019 • edited

mosesn commented Nov 7, 2019

martijnhoekstra commented Nov 8, 2019

enbnt commented Nov 25, 2019

martijnhoekstra commented Oct 7, 2019 •

edited

martijnhoekstra Oct 15, 2019 •

edited

mosesn commented Nov 4, 2019 •

edited