HeapStatData with a distinct allocation mechanism for RawStatData #3710

jmarantz · 2018-06-24T17:07:55Z

Description:

This PR sets the stage for a memory-friendly stat-allocator for use with hot-restart disabled.

hot-restart stats are still in RawStatData, non-hot-restart stats are in HeapStatData. Previously they were in HeapRawStatData which derived from RawStatData, whereas HeapStatData is an alternate impl, not derived from RawStatData.
allocators are now responsible for truncation, and require StatsOptions to be passed on construction
IsolatedStore now truncates at 1M chars, rather than special-casing no-truncation. This is now tested.
some tests moved around as there is no allocator for RawStatData in stats_impl.cc anymore; it's
in hot_restart_impl.cc
factored out a mocked testing stats-allocator from 3 different tests into a common one, changing
it from being multi-inherited with the testing framework to being instantiated. This was needed because stat-allocators now require StatOptions in their constructor.

This is another step toward addressing #3585

Risk Level: Medium
Testing: //test/...
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Joshua Marantz <jmarantz@google.com>

…em in the .h Also simplifies the prometheus tests in admin_test.cc for readability, which seemed natural to me as I removed the references to CounterImpl and GaugeImpl. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

@ambuc

…cator, factoring out the truncation to the base class. Committed for mainly for @ambuc's consideration. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123 · 2018-06-26T16:15:39Z

@dnoe please take a first pass on this

jmarantz · 2018-06-26T16:45:44Z

@mattklein123 @dnoe I mainly issued this to share the ideas here with @ambuc -- current thinking is we want his #3629 to go in first, I think.

stale · 2018-07-03T16:49:16Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2018-07-10T17:12:06Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

jmarantz · 2018-07-18T16:43:09Z

does pinging this re-open the PR? Now that #3629 is in I want to resolve conflicts & get this rolling.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2018-07-19T10:43:14Z

I think this is ready for a look, but note that I may have regressed the behavior of IsolatedStore. See comments at the relevant line.

jmarantz · 2018-07-19T10:44:28Z

source/common/stats/stats_impl.h

+  // https://github.com/envoyproxy/envoy/pull/3629, which removed the shadowing
+  // of option size in a static.
+
+  /* explicit */ IsolatedStoreImpl(/*Stats::StatsOptions& stats_options*/)


Note behavior regression here:

prior to Refactor Stats::RawStatData into a StatsOptions struct #3629 : truncated the same as other stats due to shared static max length

after Refactor Stats::RawStatData into a StatsOptions struct #3629 : not truncated at all (AFAICT)

with this PR : truncated to default length (127 chars).

If someone (@ambuc @mattklein123 @mrice32) can share the desired behavior (not truncated at all vs truncated with the other stats) I will fix.

I think this comes down to the question of whether or not we expect an IsolatedStore to have stats which are interoperable with other stats. My instinct is no, due to its isolated nature -- in which case this behavior is not something anyone relies upon. Thoughts?

Per discussion, I force the truncation at 1M bytes, so effectively we are not truncating in this PR.

jmarantz · 2018-07-19T10:46:01Z

test/common/stats/stats_impl_test.cc

@@ -478,40 +478,26 @@ TEST(TagProducerTest, CheckConstructor) {
      "No regex specified for tag specifier and no default regex for name: 'test_extractor'");
 }

-// Check consistency of internal stat representation
-TEST(RawStatDataTest, Consistency) {


Note: moved to hot_restart_impl_test.cc, which allocates RawStatData*. stats_impl.cc no longer provides an allocator that allocates RawStatData*.

This test -- still in hot_restart_impl_test.cc -- no longer covers truncation. That's covered in a separate new test in thread_local_store_test.cc, which is solely responsible for truncation.

This test's sole purpose is to make sure that the bit-pattern of a shared-memory stat allocation remains unchanged across releases, unless we also update SharedMemory::VERSION.

jmarantz · 2018-07-19T10:48:23Z

test/test_common/utility.h

@@ -371,6 +374,27 @@ class TestAllocator : public RawStatDataAllocator {
  std::unordered_map<std::string, CSmartPtr<RawStatData, freeAdapter>> stats_;
 };

+class MockedTestAllocator : public RawStatDataAllocator {


Note: this mocked allocator was previously repeated 3x in the codebase, all as part of multiple inheritance with a test class. What motivated the refactor in this PR, though, was the need to pass a StatsOptions& to the constructor, which is annoying to do for a test-class.

If desired I could split that out as a separate PR.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123

LGTM from a skim, found two small things.

mattklein123 · 2018-07-27T21:48:35Z

source/common/stats/stats_impl.h

- * in a shared memory block without internal pointers.
- */
-class RawStatDataAllocator : public StatDataAllocator {
+// Partially implements a StaDataAllocator, leaving alloc & free for subclasses.


typo StaDataAllocator

mattklein123 · 2018-07-27T21:49:16Z

source/common/stats/stats_impl.h

-  RawStatData* alloc(const std::string& name) override;
-  void free(RawStatData& data) override;
+  explicit HeapStatDataAllocator(const StatsOptions& options) : StatDataAllocatorImpl(options) {}
+  // HeapStatDataAllocator() {}


Signed-off-by: Joshua Marantz <jmarantz@google.com>

…inheritance for StatData. Signed-off-by: Joshua Marantz <jmarantz@google.com>

mrice32 · 2018-07-27T23:56:34Z

source/common/stats/stats_impl.cc


+HeapStatData* HeapStatDataAllocator::alloc(const std::string& name) {


I mentioned this earlier, but the comment got buried under some other changes. I'm relatively confident that adding truncation in HeapStatDataAllocator is a user-facing change. Previous to this change we did not truncate stats in non hot restart mode. This behavior change should be documented in the release notes. I'm also not sure it makes sense to change this behavior if we ultimately do not want non hot restart stats to be truncated as is suggested in #3965 (since I believe that this is the existing behavior). I could be wrong about the behavior changing, but if so, please point out where they are currently being truncated on master.

Reading again I think you are right; it's pretty subtle though. In current master, we truncate when making a RawStatData in RawStatData::truncateAndInit(), but asymmetrically, we truncate at the call-site in ThreadLocalStoreImpl::safeMakeStat() only when calling the fallback. OK let me fix that; it's a little cleaner to just do this right.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2018-07-29T14:52:19Z

I restored the previous functionality, and added explicit tests for it. I made all stat-name truncation happen only in ThreadLocalStore, and added a Stat interface for determining whether an allocator requires truncation.

This changed a bunch of things -- including some changes I had previously made reverting back to the state they were in 'master'. But some interface changes, such as std::string --> absl::string_view and making the mock test allocator a separate class -- means the file-count is still 19. I think it's easiest to review this as changes from master, rather than reviewing the incremental commits.

…ge, rather than asserting, as thi is easier to test in release mode. Signed-off-by: Joshua Marantz <jmarantz@google.com>

htuch

Still looks good, but a bunch of minor comments on this pass. We really should just merge this and continue iterations in the next PR :)

htuch · 2018-07-30T02:40:17Z

source/common/stats/stats_impl.h

+   * @param stats_options The stat optinos, which must live longer than the allocator.
+   */
+  explicit StatDataAllocatorImpl(/*const StatsOptions& stats_options*/) {}
+  //: stats_options_(stats_options) {}


Nit: remove comment

htuch · 2018-07-30T02:40:25Z

source/common/stats/stats_impl.h

 public:
+  /**
+   * @param stats_options The stat optinos, which must live longer than the allocator.


Nit: s/optinos/options/

htuch · 2018-07-30T02:40:49Z

source/common/stats/stats_impl.h

+  /**
+   * @param stats_options The stat optinos, which must live longer than the allocator.
+   */
+  explicit StatDataAllocatorImpl(/*const StatsOptions& stats_options*/) {}


Nit: empty arg default constructor and explicit..

htuch · 2018-07-30T02:43:12Z

source/common/stats/thread_local_store.cc

+      name = absl::string_view(name.data(), max_length);
+    }
+  }
+  return name;


Random thought that is unrelated to this PR (existing issue). How confident are we that two truncated stats won't conflict, e.g. if I truncated a.b.c.d and a.b.c.e to 5 characters, we get overlap.. it seems this is double bad, since we're both truncating and conflating..

Yeah...in the past if I wanted to truncate to 'n' I'd actually truncate to n-k, and use a k-character base64 hash of the untruncated string to disambiguate. That would still be pretty awful for stats as you'd have no idea which one you were looking at. Basically, truncation is bad and should be avoided.

htuch · 2018-07-30T02:44:16Z

source/common/stats/thread_local_store.cc

@@ -178,18 +198,13 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat(
  if (!central_ref) {
    std::vector<Tag> tags;
    std::string tag_extracted_name = parent_.getTagsForName(name, tags);


Can you add a comment explaining why it's not necessary to use the truncated name for tag extraction?

htuch · 2018-07-30T02:46:25Z

test/test_common/utility.h

+
+class MockedTestAllocator : public RawStatDataAllocator {
+public:
+  MockedTestAllocator(const StatsOptions& stats_options) : alloc_(stats_options) {


Nit: constructor/destructor for mocks should live in .cc. See https://github.com/google/googlemock/blob/master/googlemock/docs/v1_6/CookBook.md#making-the-compilation-faster

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123

LGTM. Changes in this code/area make my eyes bleed so just need to pull the trigger. Few small things.

mattklein123 · 2018-07-30T18:54:03Z

source/common/stats/stats_impl.h

+  /**
+   * @param stats_options The stat options, which must live longer than the allocator.
+   */
+  StatDataAllocatorImpl() {}


nit: Is this default constructor needed? Doc comment seems out of date also.

oops; not needed; in an earlier rev I needed a 1-arg constructor and removing the arg, I forgot to remove the whole thing.

mattklein123 · 2018-07-30T18:55:12Z

source/common/stats/thread_local_store.cc

+
+    // Note that the heap-allocator does not truncate itself; we have to
+    // truncate here if we are using heap-allocation as a fallback due to an
+    // exahusted shared-memroy block


typo memroy

mattklein123 · 2018-07-30T18:56:27Z

source/server/hot_restart_impl.cc

-    key.remove_suffix(key.size() - options_.statsOptions().maxNameLength());
+  // The name is truncated in ThreadLocalStore before this is called.
+  size_t max_length = options_.statsOptions().maxNameLength();
+  if (name.length() > max_length) {


Can this happen then? Should this just be an assert? The comment is a bit confusing.

It should not happen in production. It can happen in a unit test. It could be an ASSERT, but then it can't be tested. It could be a RELEASE_ASSERT, and be tested with a DEATH_TEST.

I'm happy to go with whatever is the best practice of those for the project.

I'll try to clarify the comment.

I would probably just do a normal ASSERT as this seems like a programming error. If you feel strongly that it should be a RELEASE_ASSERT w/ a test that's fine also.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123

🤞 😉

@mrice32 can you approve the most recent changes? Thank you for powering through this review!

htuch

LGTM pending @mrice32 final pass.

mrice32

This is great! Thanks. A few small comments. I think that this code has gotten more complicated as we've added flexibility, and I think it could definitely use one or two PRs of work on simplification.

mrice32 · 2018-07-31T15:00:06Z

source/server/hot_restart_impl.h

  void free(Stats::RawStatData& data) override;
+  bool requiresBoundedStatNameSize() const override { return true; }


Why is this overridden here and in RawStatDataAllocator? Also why even have the RawStatDataAllocator intermediate class?

Good catch - I didn't need this extra impl of requiresBoundedStatNameSize here, but I do need RawStatDataAllocator as it is used as a base class for test allocators as well as this.

mrice32 · 2018-07-31T15:03:17Z

source/common/stats/stats_impl.cc

@@ -319,47 +320,28 @@ TagProducerImpl::addDefaultExtractors(const envoy::config::metrics::v2::StatsCon
  return names;
 }

-void HeapRawStatDataAllocator::free(RawStatData& data) {
+// TODO(jmarantz): move this below HeapStatDataAllocator::alloc.


? Seems like a strange TODO?

Normally it makes sense to group alloc/free together of a single class, but these got separated in a prior PR; leaving this here now for easier diffs. Will fix later in a follow-up.

I see. It seemed so trivial, I was a little confused as to why you didn't just do it now. Easier review is a good reason. That makes sense.

mrice32 · 2018-07-31T15:08:20Z

source/common/stats/stats_impl.cc


+HeapStatData* HeapStatDataAllocator::alloc(absl::string_view name) {
+  // Do not truncate here for non-hot-restart case; truncate at the call-site in


This is probably a weird nit, but I think this comment is adding confusion. If we want to delegate truncation to the callsite, I think we can just make the comment (or something similar) since the truncation logic isn't really relevant here:

// Any expected truncation of name is done at the callsite. No truncation is // required to use this allocator.

mrice32 · 2018-07-31T15:11:40Z

source/common/stats/stats_impl.h


-  // An unordered set of RawStatData pointers which keys off the key()
+  // TODO(jmarantz): See https://github.com/envoyproxy/envoy/pull/3927 which can


Sounds like more of a TODO for @ambuc :)

In my experience, TODO's are generally signed by the author of the TODO, not necessarily by the person who is expected to do it.

Let's leave a comment linking to issue #3585 which will address this in #3927, hopefully.

mrice32 · 2018-07-31T15:15:21Z

source/common/stats/thread_local_store.cc

+absl::string_view ThreadLocalStoreImpl::truncateStatNameIfNeeded(absl::string_view name) {
+  // If the main allocator requires stat name truncation, warn and truncate, before
+  // attempting to allocate.
+  if (alloc_.requiresBoundedStatNameSize()) {


nit: as I mentioned in person, I think the encapsulation would be cleaner if the allocators were constructed with their truncation settings and then they could handle any necessary truncation themselves (the fallback heap allocator would be set up to truncate, while the non-hot restart heap allocator would be set up to not truncate). Here, it seems like we're half delegating that responsibility by giving them a method to communicate whether they require truncation, but not giving them the responsibility to read the settings and do that truncation within alloc().

However, I think we should just try to get this PR in as is rather than making more interface changes, so this seems fine for now.

Yeah I think that's a follow-up change.

mrice32 · 2018-07-31T15:21:33Z

source/common/stats/stats_impl.h

+  HeapStatData* alloc(absl::string_view name) override;
+  void free(HeapStatData& data) override;
+
+  // StatsDataAllocator


nit typo: StatDataAllocator

This typo seems to still be here :)

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mrice32

LGTM other than the typos

mrice32 · 2018-07-31T18:09:09Z

source/common/stats/stats_impl.h

+
+class RawStatDataAllocator : public StatDataAllocatorImpl<RawStatData> {
+public:
+  // StatsDataAllocator


nit: typo here, too: StatDataAllocator

mrice32 · 2018-07-31T18:09:47Z

source/common/stats/stats_impl.h

+  HeapStatData* alloc(absl::string_view name) override;
+  void free(HeapStatData& data) override;
+
+  // StatsDataAllocator


This typo seems to still be here :)

…sure... Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123

OK let's do this. Agree with @mrice32 that this code has become incredibly complicated with the different permutations of heap/hot-restart, size limits, etc. If there is anything we can do in followups to help with readability that would be awesome.

jmarantz added 11 commits June 22, 2018 14:56

templatize RawStatDataAllocator -- not functional yet.

a7f5585

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'refactor-admin-stats' into stats-heap-divest

be3cfee

Signed-off-by: Joshua Marantz <jmarantz@google.com>

more templatization hacks.

dc1f5a5

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into stats-heap-divest

27f1d9e

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Introduce a pure heap-based variant of RawStatsData.

ee8cf0b

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into refactor-admin-stats

29741c2

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'refactor-admin-stats' into stats-heap-divest

3513338

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into stats-heap-divest

7204822

Signed-off-by: Joshua Marantz <jmarantz@google.com>

experimental commit that merges the two variants of Heap*StatDataAllo…

6c09c7c

…cator, factoring out the truncation to the base class. Committed for mainly for @ambuc's consideration. Signed-off-by: Joshua Marantz <jmarantz@google.com>

remove superfluous blank linke.

d427401

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123 assigned dnoe Jun 26, 2018

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 3, 2018

stale bot closed this Jul 10, 2018

htuch reopened this Jul 18, 2018

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 18, 2018

jmarantz added 4 commits July 18, 2018 13:08

Merge branch 'master' into stats-heap-divest

e21dd1d

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Checkpointing all tests build (2 fail)

343eae5

Signed-off-by: Joshua Marantz <jmarantz@google.com>

all tests pass.

d809c61

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Formatting and other cleanup.

5d3e916

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz changed the title ~~WIP stats: Create HeapStatData with a distinct allocation mechanism for RawStatData~~ HeapStatData with a distinct allocation mechanism for RawStatData Jul 19, 2018

jmarantz commented Jul 19, 2018

View reviewed changes

Test that truncation actually occurred in Consistency test.

c79f73d

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123 reviewed Jul 27, 2018

View reviewed changes

Fix typos

ccb29e1

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz dismissed htuch’s stale review via ccb29e1 July 27, 2018 22:03

Add more commentary about why templates are used rather than virtual …

9a757c2

…inheritance for StatData. Signed-off-by: Joshua Marantz <jmarantz@google.com>

mrice32 reviewed Jul 27, 2018

View reviewed changes

jmarantz added 3 commits July 28, 2018 16:23

Move all truncation to thread_local_store.cc.

7f84547

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Move truncation test to thread_local_store_test.cc, and simplify code.

7d15bf8

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Cleanup and restore truncation-message log test.

5f82615

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Return nullptr when HotRestartImpl::alloc is asked for a name too lar…

8dbaff2

…ge, rather than asserting, as thi is easier to test in release mode. Signed-off-by: Joshua Marantz <jmarantz@google.com>

htuch reviewed Jul 30, 2018

View reviewed changes

style fixes & typos

700d540

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123 reviewed Jul 30, 2018

View reviewed changes

jmarantz added 2 commits July 30, 2018 15:51

Address review comments, style issues, etc.

319462b

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Use ASSERT for length-check when allocating stats in hot-restart.

c0f9499

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123 previously approved these changes Jul 31, 2018

View reviewed changes

mattklein123 self-assigned this Jul 31, 2018

htuch previously approved these changes Jul 31, 2018

View reviewed changes

mrice32 reviewed Jul 31, 2018

View reviewed changes

Address review nits.

1b89d7f

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz dismissed stale reviews from htuch and mattklein123 via 1b89d7f July 31, 2018 16:59

mrice32 approved these changes Jul 31, 2018

View reviewed changes

mrice32 previously approved these changes Jul 31, 2018

View reviewed changes

Correct spelling of StatDataAllocator -- this time it will stick for …

08fe789

…sure... Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz dismissed mrice32’s stale review via 08fe789 July 31, 2018 18:28

mattklein123 approved these changes Jul 31, 2018

View reviewed changes

mattklein123 merged commit d0f10fa into envoyproxy:master Jul 31, 2018

jmarantz deleted the stats-heap-divest branch July 31, 2018 20:03

jmarantz mentioned this pull request Jul 31, 2018

Re-order functions in stats_impl to group classes together #4004

Merged


		HeapStatData* HeapStatDataAllocator::alloc(const std::string& name) {

		void free(Stats::RawStatData& data) override;
		bool requiresBoundedStatNameSize() const override { return true; }


		HeapStatData* HeapStatDataAllocator::alloc(absl::string_view name) {
		// Do not truncate here for non-hot-restart case; truncate at the call-site in


		// An unordered set of RawStatData pointers which keys off the key()
		// TODO(jmarantz): See https://github.com/envoyproxy/envoy/pull/3927 which can

HeapStatData with a distinct allocation mechanism for RawStatData #3710

HeapStatData with a distinct allocation mechanism for RawStatData #3710

Conversation

jmarantz commented Jun 24, 2018 • edited Loading

mattklein123 commented Jun 26, 2018

jmarantz commented Jun 26, 2018

stale bot commented Jul 3, 2018

stale bot commented Jul 10, 2018

jmarantz commented Jul 18, 2018

jmarantz commented Jul 19, 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

jmarantz Jul 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 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

mrice32 Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz commented Jul 29, 2018 • edited Loading

htuch 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

mattklein123 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

mattklein123 left a comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

mrice32 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

mrice32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

jmarantz commented Jun 24, 2018 •

edited

Loading

jmarantz Jul 29, 2018 •

edited

Loading

mrice32 Jul 27, 2018 •

edited

Loading

jmarantz commented Jul 29, 2018 •

edited

Loading