[BEAM-1211] Make Coders more efficient by removing unnecessary nesting #1680

dhalperi · 2016-12-21T23:55:30Z

See BEAM-469 for more information about why this is
correct.

dhalperi · 2016-12-21T23:55:56Z

This is a first prototype to see if we should generalize the discussion we had on BEAM-469 about NullableCoder

dhalperi · 2016-12-21T23:57:15Z

I'm hoping all the ITs will pass. Because nothing should depend on the value coder being force-nested, I think

asfbot · 2016-12-22T00:18:21Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6200/

Build result: FAILURE

[...truncated 16953 lines...] ^/home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/runners/direct-java/src/main/java/org/apache/beam/runners/direct/Clock.java:27: error: reference not found * Returns the current time as an {@link Instant}. ^Command line was: /usr/local/asfpackages/java/jdk1.8.0_102/jre/../bin/javadoc @options @packagesRefer to the generated Javadoc files in '/home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/runners/direct-java/target/apidocs' dir. at org.apache.maven.plugin.javadoc.AbstractJavadocMojo.executeJavadocCommandLine(AbstractJavadocMojo.java:5163) at org.apache.maven.plugin.javadoc.AbstractJavadocMojo.executeReport(AbstractJavadocMojo.java:2075) at org.apache.maven.plugin.javadoc.JavadocJar.execute(JavadocJar.java:188) ... 33 more2016-12-22T00:18:15.351 [ERROR] 2016-12-22T00:18:15.351 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2016-12-22T00:18:15.351 [ERROR] 2016-12-22T00:18:15.351 [ERROR] For more information about the errors and possible solutions, please read the following articles:2016-12-22T00:18:15.351 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException2016-12-22T00:18:15.351 [ERROR] 2016-12-22T00:18:15.351 [ERROR] After correcting the problems, you can resume the build with the command2016-12-22T00:18:15.351 [ERROR] mvn -rf :beam-runners-direct-javachannel stoppedSetting status of 621e825 to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6200/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install
--none--

dhalperi · 2016-12-22T01:10:52Z

retest this please

(failure caused by snappy, which is transient thing we've seen for unknown reasons)

asfbot · 2016-12-22T01:38:29Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6206/
--none--

asfbot · 2016-12-22T04:15:04Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6211/

Failed Tests: 2

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-sdks-java-core: 2

--none--

lukecwik · 2016-12-22T17:56:07Z

There are some wire format tests that ensure the encoding matches a predetermined value that is hardcoded.

dhalperi · 2016-12-22T18:13:12Z

(the "stash work" commit is meant to be ignored). KVCoder's tests didn't break because they used a valuecoder that isn't context-sensitive.

See [BEAM-469] for more information about why this is correct.

dhalperi

Partial review of my own PR :)

dhalperi · 2016-12-22T23:41:33Z

...e-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/internal/IsmFormat.java

-          VarIntCoder.of().decode(inStream, context),
-          VarLongCoder.of().decode(inStream, context),
+          VarIntCoder.of().decode(inStream, context.nested()),
+          VarLongCoder.of().decode(inStream, context.nested()),
          VarLongCoder.of().decode(inStream, context));


this code was broken, but I think none of the coders are context-sensitive.

dhalperi · 2016-12-22T23:43:54Z

sdks/java/core/src/main/java/org/apache/beam/sdk/coders/MapCoder.java

+        valueCoder.registerByteSizeObserver(entry.getValue(), observer, context.nested());
+      } else {
+        valueCoder.registerByteSizeObserver(entry.getValue(), observer, context);
+      }


Relatively complex changes. But as single-element maps are probably likely to be used in global combining, suspect it should be worth it.

Use the pattern I showed above about how to use an iterator to get special treatment for last value without needing to check whether its the last value more than once.

dhalperi · 2016-12-22T23:44:49Z

sdks/java/core/src/main/java/org/apache/beam/sdk/testing/ValueInSingleWindow.java

@@ -109,7 +109,7 @@ public void encode(ValueInSingleWindow<T> windowedElem, OutputStream outStream,
      T value = valueCoder.decode(inStream, nestedContext);
      Instant timestamp = InstantCoder.of().decode(inStream, nestedContext);
      BoundedWindow window = windowCoder.decode(inStream, nestedContext);
-      PaneInfo pane = PaneInfo.PaneInfoCoder.INSTANCE.decode(inStream, nestedContext);
+      PaneInfo pane = PaneInfo.PaneInfoCoder.INSTANCE.decode(inStream, context);


Note I only updated decode -- encode already had the changes. Suspect this was not a bug in practice b/c the PaneInfoCoder is not context-sensitive.

Anytime we have a case where we know that all but one of the coders are not context sensitive, we should always place the context sensitive coder to be last.

Done here by putting valueCoder last.

dhalperi · 2016-12-22T23:45:22Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/CombineFns.java

        coders.get(i).encode(value[i], outStream, nestedContext);
      }
+      coders.get(codersCount - 1).encode(value[codersCount - 1], outStream, context);


broken in the case of length == 0. verify that this is impossible or fix.

rewrote code to avoid the issue.

dhalperi · 2016-12-22T23:46:03Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/join/CoGbkResult.java

        tagListCoder(unionTag).encode(value.valueMap.get(unionTag), outStream, Context.NESTED);
      }
+      tagListCoder(lastTag).encode(value.valueMap.get(lastTag), outStream, context);


broken if size == 0. verify this is impossible or fix.

rewrote code to avoid the issue.

dhalperi · 2016-12-22T23:47:14Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/WindowedValue.java

@@ -671,7 +671,7 @@ public void encode(WindowedValue<T> windowedElem,
      Instant timestamp = InstantCoder.of().decode(inStream, nestedContext);
      Collection<? extends BoundedWindow> windows =
          windowsCoder.decode(inStream, nestedContext);
-      PaneInfo pane = PaneInfoCoder.INSTANCE.decode(inStream, nestedContext);
+      PaneInfo pane = PaneInfoCoder.INSTANCE.decode(inStream, context);


fixing the same "bug" as in the the other use of PaneInfoCoder. Likely not context-sensitive.

Updated this coder to put value last, and fixed the registerByteSizeObserver that I missed the last time around.

dhalperi · 2016-12-22T23:47:47Z

sdks/java/core/src/test/java/org/apache/beam/sdk/coders/BigDecimalCoderTest.java

+              + "AAAAAA",
+          "AAn_______-wAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+              + "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+              + "AAAAAA");


verifying that we save a small number of bytes.

dhalperi · 2016-12-22T23:48:15Z

sdks/java/core/src/test/java/org/apache/beam/sdk/coders/MapCoderTest.java

@@ -85,7 +85,7 @@ public void testEncodingId() throws Exception {
   */
  private static final List<String> TEST_ENCODINGS = Arrays.asList(
      "AAAAAA",
-      "AAAAAv____8PA2ZvbwEFaGVsbG8");
+      "AAAAAv____8PA2ZvbwFoZWxsbw");


verifying that we can save a small number of bytes.

dhalperi · 2016-12-22T23:48:38Z

sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/CombineTest.java

@@ -905,14 +905,14 @@ public CountSum createAccumulator() {
      @Override
      public void encode(CountSum value, OutputStream outStream,
          Context context) throws CoderException, IOException {
-        LONG_CODER.encode(value.count, outStream, context);
+        LONG_CODER.encode(value.count, outStream, context.nested());
        DOUBLE_CODER.encode(value.sum, outStream, context);


bug the other way, but in practice LONG_CODER is not context-sensitive.

asfbot · 2016-12-23T00:14:49Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6233/
--none--

dhalperi · 2016-12-23T01:07:44Z

Filed a proper JIRA issue, ready for review.

I may make a test-improvement pass next week.

asfbot · 2016-12-23T01:23:39Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6240/

Failed Tests: 2

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-examples-java: 2

--none--

dhalperi · 2016-12-27T17:51:02Z

retest this please

asfbot · 2016-12-27T18:25:31Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6260/
--none--

lukecwik

Swapping to use the iterator pattern, other than that looks good to me.

lukecwik · 2016-12-28T17:25:48Z

sdks/java/core/src/main/java/org/apache/beam/sdk/coders/MapCoder.java

@@ -115,7 +123,12 @@ public void encode(
    Map<K, V> retval = Maps.newHashMapWithExpectedSize(size);
    for (int i = 0; i < size; ++i) {


Update to remove the if from the loop:

if (size == 0) { return empty map } for (i = 0; i < size - 1; ++i) { decode nested key and value } decode nested key decode value using context passed in

lukecwik · 2016-12-28T17:27:40Z

sdks/java/core/src/main/java/org/apache/beam/sdk/coders/MapCoder.java

+      } else {
+        valueCoder.encode(entry.getValue(), outStream, context);
+      }
+      ++i;
    }
    dataOutStream.flush();


Flushing is not needed or expected since DataOutputStream is not a buffering output stream.

lukecwik · 2016-12-28T17:30:24Z

sdks/java/core/src/main/java/org/apache/beam/sdk/coders/MapCoder.java

-    dataOutStream.writeInt(map.size());
+    int size = map.size();
+    dataOutStream.writeInt(size);
+    int i = 0;
    for (Entry<K, V> entry : map.entrySet()) {


Might be cleaner using an iterator:

write out size if size == 0; return Iterator<Map.Entry> iterator = map.entrySet().iterator(); Map.Entry<K, V> entry = iterator.next(); while (iterator.hasNext()) { encode key and value using nested context entry = iterator.next(); } encode key using nested context encode value using passed in context

lukecwik · 2016-12-28T17:35:48Z

sdks/java/core/src/main/java/org/apache/beam/sdk/coders/MapCoder.java

+        valueCoder.registerByteSizeObserver(entry.getValue(), observer, context.nested());
+      } else {
+        valueCoder.registerByteSizeObserver(entry.getValue(), observer, context);
+      }


Use the pattern I showed above about how to use an iterator to get special treatment for last value without needing to check whether its the last value more than once.

lukecwik · 2016-12-28T17:40:52Z

sdks/java/core/src/main/java/org/apache/beam/sdk/testing/ValueInSingleWindow.java

@@ -109,7 +109,7 @@ public void encode(ValueInSingleWindow<T> windowedElem, OutputStream outStream,
      T value = valueCoder.decode(inStream, nestedContext);
      Instant timestamp = InstantCoder.of().decode(inStream, nestedContext);
      BoundedWindow window = windowCoder.decode(inStream, nestedContext);
-      PaneInfo pane = PaneInfo.PaneInfoCoder.INSTANCE.decode(inStream, nestedContext);
+      PaneInfo pane = PaneInfo.PaneInfoCoder.INSTANCE.decode(inStream, context);


Anytime we have a case where we know that all but one of the coders are not context sensitive, we should always place the context sensitive coder to be last.

lukecwik · 2016-12-28T17:41:44Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/CombineFns.java

@@ -996,7 +996,11 @@ public void encode(Object[] value, OutputStream outStream, Context context)
      checkArgument(value.length == codersCount);
      Context nestedContext = context.nested();
      for (int i = 0; i < codersCount; ++i) {


Ditto on using iterator to differentiate the loop from the last element case as done in MapCoder

lukecwik · 2016-12-28T17:42:27Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/CombineFns.java

@@ -1006,7 +1010,11 @@ public void encode(Object[] value, OutputStream outStream, Context context)
      Object[] ret = new Object[codersCount];
      Context nestedContext = context.nested();
      for (int i = 0; i < codersCount; ++i) {
-        ret[i] = coders.get(i).decode(inStream, nestedContext);


Ditto on using iterator to differentiate loop from last element

lukecwik · 2016-12-28T17:42:53Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/join/CoGbkResult.java

@@ -269,7 +269,11 @@ public void encode(
        throw new CoderException("input schema does not match coder schema");
      }
      for (int unionTag = 0; unionTag < schema.size(); unionTag++) {


Use iterator to differentiate loop from last element

lukecwik · 2016-12-28T17:42:59Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/join/CoGbkResult.java

@@ -280,7 +284,11 @@ public CoGbkResult decode(
        throws CoderException, IOException {
      List<Iterable<?>> valueMap = new ArrayList<>();
      for (int unionTag = 0; unionTag < schema.size(); unionTag++) {


Use iterator to differentiate loop from last element

lukecwik · 2016-12-28T17:44:07Z

sdks/java/core/src/test/java/org/apache/beam/sdk/coders/JAXBCoderTest.java

@@ -175,7 +175,7 @@ public void encode(TestType value, OutputStream outStream, Context context)
      Context subContext = context.nested();


nit: rename subContext to nestedContext here and below

…nsitive coders last

dhalperi

PTAL

dhalperi · 2016-12-28T18:55:18Z

sdks/java/core/src/main/java/org/apache/beam/sdk/testing/ValueInSingleWindow.java

@@ -109,7 +109,7 @@ public void encode(ValueInSingleWindow<T> windowedElem, OutputStream outStream,
      T value = valueCoder.decode(inStream, nestedContext);
      Instant timestamp = InstantCoder.of().decode(inStream, nestedContext);
      BoundedWindow window = windowCoder.decode(inStream, nestedContext);
-      PaneInfo pane = PaneInfo.PaneInfoCoder.INSTANCE.decode(inStream, nestedContext);
+      PaneInfo pane = PaneInfo.PaneInfoCoder.INSTANCE.decode(inStream, context);


Done here by putting valueCoder last.

dhalperi · 2016-12-28T18:56:14Z

sdks/java/core/src/main/java/org/apache/beam/sdk/coders/MapCoder.java

+        valueCoder.registerByteSizeObserver(entry.getValue(), observer, context.nested());
+      } else {
+        valueCoder.registerByteSizeObserver(entry.getValue(), observer, context);
+      }


dhalperi · 2016-12-28T18:59:26Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/CombineFns.java

@@ -996,7 +996,11 @@ public void encode(Object[] value, OutputStream outStream, Context context)
      checkArgument(value.length == codersCount);
      Context nestedContext = context.nested();
      for (int i = 0; i < codersCount; ++i) {


dhalperi · 2016-12-28T18:59:33Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/CombineFns.java

@@ -1006,7 +1010,11 @@ public void encode(Object[] value, OutputStream outStream, Context context)
      Object[] ret = new Object[codersCount];
      Context nestedContext = context.nested();
      for (int i = 0; i < codersCount; ++i) {
-        ret[i] = coders.get(i).decode(inStream, nestedContext);


dhalperi · 2016-12-28T19:03:25Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/join/CoGbkResult.java

@@ -269,7 +269,11 @@ public void encode(
        throw new CoderException("input schema does not match coder schema");
      }
      for (int unionTag = 0; unionTag < schema.size(); unionTag++) {


dhalperi · 2016-12-28T19:03:29Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/join/CoGbkResult.java

@@ -280,7 +284,11 @@ public CoGbkResult decode(
        throws CoderException, IOException {
      List<Iterable<?>> valueMap = new ArrayList<>();
      for (int unionTag = 0; unionTag < schema.size(); unionTag++) {


dhalperi · 2016-12-28T19:06:21Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/WindowedValue.java

@@ -671,7 +671,7 @@ public void encode(WindowedValue<T> windowedElem,
      Instant timestamp = InstantCoder.of().decode(inStream, nestedContext);
      Collection<? extends BoundedWindow> windows =
          windowsCoder.decode(inStream, nestedContext);
-      PaneInfo pane = PaneInfoCoder.INSTANCE.decode(inStream, nestedContext);
+      PaneInfo pane = PaneInfoCoder.INSTANCE.decode(inStream, context);


Updated this coder to put value last, and fixed the registerByteSizeObserver that I missed the last time around.

dhalperi · 2016-12-28T19:07:04Z

sdks/java/core/src/test/java/org/apache/beam/sdk/coders/JAXBCoderTest.java

@@ -175,7 +175,7 @@ public void encode(TestType value, OutputStream outStream, Context context)
      Context subContext = context.nested();


dhalperi · 2016-12-28T19:24:05Z

Which coders are cross-Fn-API and need to stay fixed? Should have a list. E.g., is WindowedValueCoder okay to change?

asfbot · 2016-12-28T19:49:21Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6297/
--none--

lukecwik · 2016-12-28T23:12:26Z

No point in worrying about binary representations right now. Someone will have to make a pass through on a coder by coder basis per SDK to make them align.

lukecwik

Minor nits
LGTM

lukecwik · 2016-12-28T23:13:29Z

sdks/java/core/src/main/java/org/apache/beam/sdk/coders/MapCoder.java

+      return;
+    }
+
+    // Since we handled size == 0 above, entry is guaranteed to exist after before and after loop


exist after before -> exist before

lukecwik · 2016-12-28T23:15:08Z

sdks/java/core/src/main/java/org/apache/beam/sdk/coders/MapCoder.java

@@ -162,15 +172,17 @@ public void registerByteSizeObserver(
      Map<K, V> map, ElementByteSizeObserver observer, Context context)
      throws Exception {
    observer.update(4L);
+    if (map.isEmpty()){


spacing around ){

lukecwik · 2016-12-28T23:16:34Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/join/CoGbkResult.java

+      if (schema.size() == 0) {
+        return;
+      }
+      int nested = schema.size() - 1;


rename nested -> lastIndex

lukecwik · 2016-12-28T23:17:40Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/CombineFns.java

-        } else {
-          coders.get(i).encode(value[i], outStream, context);
-        }
+      for (int i = 0; i < codersCount - 1; ++i) {


I like how you used the place holder variable to store the lastIndex in CoGbkResult.java, would recommend using it here and in decode

lukecwik · 2016-12-28T23:17:58Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/join/CoGbkResult.java

+      if (schema.size() == 0) {
+        return new CoGbkResult(schema, ImmutableList.<Iterable<?>>of());
+      }
+      int nested = schema.size() - 1;


rename nested -> lastIndex

dhalperi · 2016-12-28T23:31:44Z

Thanks Luke! Fixed nits and merging.

asfbot · 2016-12-29T00:02:22Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6309/
--none--

[BEAM-XXX] Make KVCoder more efficient by removing unnecessary nesting

b5b18ac

See [BEAM-469] for more information about why this is correct.

dhalperi force-pushed the efficient-nested-coders branch from 1dea97e to b5b18ac Compare December 22, 2016 23:40

dhalperi commented Dec 22, 2016

View reviewed changes

dhalperi changed the title ~~[BEAM-XXX] Make KVCoder more efficient by removing unnecessary nesting~~ [BEAM-XXX] Make Coders more efficient by removing unnecessary nesting Dec 22, 2016

fix == 0 bugs in list-based standard-coders

4e08e73

dhalperi changed the title ~~[BEAM-XXX] Make Coders more efficient by removing unnecessary nesting~~ [BEAM-1211] Make Coders more efficient by removing unnecessary nesting Dec 23, 2016

lukecwik requested changes Dec 28, 2016

View reviewed changes

Rewrite loops using fewer ifs, reorder a few coders to put context-se…

78b0e57

…nsitive coders last

dhalperi commented Dec 28, 2016

View reviewed changes

lukecwik approved these changes Dec 28, 2016

View reviewed changes

fixup

fb834cb

asfgit closed this in 84134bc Dec 28, 2016

dhalperi deleted the efficient-nested-coders branch January 20, 2017 03:39

		@@ -115,7 +123,12 @@ public void encode(
		Map<K, V> retval = Maps.newHashMapWithExpectedSize(size);
		for (int i = 0; i < size; ++i) {

		@@ -175,7 +175,7 @@ public void encode(TestType value, OutputStream outStream, Context context)
		Context subContext = context.nested();

[BEAM-1211] Make Coders more efficient by removing unnecessary nesting #1680

[BEAM-1211] Make Coders more efficient by removing unnecessary nesting #1680

Conversation

dhalperi commented Dec 21, 2016 • edited

dhalperi commented Dec 21, 2016 • edited

dhalperi commented Dec 21, 2016

asfbot commented Dec 22, 2016

Build result: FAILURE

dhalperi commented Dec 22, 2016 • edited

asfbot commented Dec 22, 2016

asfbot commented Dec 22, 2016

Failed Tests: 2

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-sdks-java-core: 2

lukecwik commented Dec 22, 2016

dhalperi commented Dec 22, 2016

dhalperi 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

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

asfbot commented Dec 23, 2016

dhalperi commented Dec 23, 2016

asfbot commented Dec 23, 2016

Failed Tests: 2

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-examples-java: 2

dhalperi commented Dec 27, 2016

asfbot commented Dec 27, 2016

lukecwik 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

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

dhalperi 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

dhalperi commented Dec 28, 2016

asfbot commented Dec 28, 2016

lukecwik commented Dec 28, 2016

lukecwik 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

dhalperi commented Dec 21, 2016 •

edited

dhalperi commented Dec 21, 2016 •

edited

dhalperi commented Dec 22, 2016 •

edited