Make sampler counts faster, and also binned by level #252

jcorbin · 2017-01-24T20:10:24Z

Towards #176:

benchmark the prior counters
implement a faster and bounded memory counters
add leveled keying

mention-bot · 2017-01-24T20:10:25Z

@jcorbin, thanks for your PR! By analyzing the history of the files in this pull request, we identified @akshayjshah, @prashantv and @creiht to be potential reviewers.

akshayjshah · 2017-01-25T01:53:05Z

zapcore/sampler.go

+func (c *counters) Reset(key string) {
+	c.Lock()
+	count := c.counts[key]
+	c.Unlock()


My fault - I think that this should be an RLock/RUnlock, right? (We're getting rid of this counters implementation anyways, but it's possible that this'll affect some of the benchmarks.)

akshayjshah · 2017-01-25T21:00:15Z

zapcore/sampler.go

+type bucket2 struct {
+	sync.Mutex
+	counts [_counters2PerLock]uint64
+}


Now that we're not keeping track of the unhashed messages, what's the benefit of using counts + mutexes instead of a flat array of atomics?

akshayjshah · 2017-01-25T23:38:17Z

zapcore/sampler.go

+
+// xorstring converts a string into a uint64 by xoring together its
+// codepoints. It works by accumulating into a 64-bit "ring" which gets
+// rotated by the apparent "byte width" of each codepoint.


Comment's no longer accurate (in case you forget when un-WIP-ing this PR.)

akshayjshah · 2017-01-25T23:39:25Z

zapcore/sampler.go

+
+// xshrr computes a "randomly" rotated xorshift; this is the "XSH RR"
+// transformation borrowed from the PCG famiily of random generators. It
+// returns a 32-bit output from a 64-bit state.


I'm probably a reasonable stand-in for a user of this library, and I hadn't heard of "PCG random generators" until talking to you. If we're going to name-check this paper, let's include a link :)

akshayjshah · 2017-01-25T23:42:18Z

zapcore/sampler_bench_test.go

+		"conjecture",
+		"Arctomys",
+		"guapilla",
+		"Sassenach",


Perhaps overly sensitive, but let's not include vaguely racist epithets (even if they're randomly selected from the system dictionary). In my understanding, "Sassenach" is in the same ballpark as "honky" and "bouzhoi."

akshayjshah · 2017-01-25T23:44:22Z

zapcore/sampler_bench_test.go

+		"raphidiferous",
+		"pooh",
+		"Tyndallize",
+	},


Can we use this same test data to generate a much larger set of n-grams, and use that to sanity check the hash function? Just want to make sure that we don't mess up and end up with way more hash collisions than expected.

👏 for taking this idea much further than I would have. Dunno about Prashant, but I'm satisfied that XSHRR is a reasonable option.

jcorbin · 2017-02-01T22:43:28Z

WIP: updated, broke out, benchmarked, and compared distributions on hash function alternatives; the only clear win to me is taking over the implementation:

the real win is dropping the string -> []byte alloc that using core the hash/fnv would incur
both fnv, and my quick riff on pcg32-xsh-rr are about as performant, and well distributed
xshrr is marginally slower, but may be marginally "better" (not that I think it matters currently)

I'm going to continue on then and actually implement the level bucking work, and reduce locking counters2 itself.

Future directions:

a better testing corpus (that does such things as sugaredlogger-esque lines that are 99% the same, but for one number)
a count min-sketch would probably be better than a single array of hash buckets; the xshrr hash should be easy to parameterize (by re-introducing the LCG additive constant, and varying it)

Notebook: https://github.com/uber-go/zap/blob/d94151cd4490bb8014fec7d4f3e442b67dd0606e/internal/hash/hash_dist.ipynb

jcorbin · 2017-02-02T00:07:46Z

Updated:

now benchmarks at Sampler.Check
dropped the old counters locked-map
added level binning

prashantv · 2017-02-02T01:48:48Z

zapcore/sampler.go

+	return &counters{}
+}
+
+type counters [_numLevels][_countersPerLevel]uint64


if we're using these atomically, can we use atomic.Uint64 here? we already have dependencies on the atomic package.

Well:

I suspect that it will be more memory inefficient, if not actually slower, due to atomic.Uint64 being a struct with a pointer field, rather than the actual uint64 values living in this array

the only use that we're getting from go.uber.org/atomic is for the AtomicLevel type, which is already a struct, and arguably would be simpler if it used standard sync/atomic functions directly

I'll benchmark to test any supposed difference, but more towards point 2, I really don't think that using go.uber.org/atomic is adding much value for zap at this point.

It's not a struct with a pointer field, the field is a value.
https://github.com/uber-go/atomic/blob/master/atomic.go#L175

The memory layout will be exactly the same, there should be no indirection either way.

The only performance difference will be because of the extra function call, which Go should be inlining but doesn't yet (functions that call other functions are not inlined). Unfortunately that will probably add a handful of nanoseconds.

We should probably have benchmarks in the atomic package itself to measure the overhead.

The atomic package only really provides a couple of things:

safety: since you can't access the underlying value directly, you have to use the atomic accessors. This avoids accidentally doing a non-atomic read of an atomic value, which causes a data race.

readability: rather than atomic.AddUint64(&c[i][j], 1), the code becomes c[i][j].Inc()

Didn't realize this would be a point for debate. I'd also strongly prefer that we use the atomic package.

prashantv · 2017-02-02T01:49:43Z

zapcore/sampler.go

+func (c *counters) Inc(lvl Level, key string) uint64 {
+	i := lvl - minLevel
+	j := c.hash(key) % _countersPerLevel
+	return atomic.AddUint64(&c[i][j], 1)


can we have a method to return the specific counter?

func (c *counters) counterFor(lvl Level, key string) *atomic.Uint64

Sure, makes sense (modulo return type per my above reservations on atomic packages)

prashantv · 2017-02-02T01:50:39Z

zapcore/sampler.go

+
+func (c *counters) Inc(lvl Level, key string) uint64 {
+	i := lvl - minLevel
+	j := c.hash(key) % _countersPerLevel


We're picking the _countersPerLevel, and we've picked a nice power of 2, so we can use bit masking here rather than a mod.

const _countersPerLevelMask = _countersPerLevel - 1 [...] j = c.hash(key) & _countersPerLevelMask

Good point, I was hoping that the go compiler knew this trick, but I've not checked or verified; will do ;-)

prashantv · 2017-02-02T01:52:01Z

zapcore/sampler.go

+}
+
+func (c *counters) hash(key string) uint32 {
+	return hash.XSHRR(key) % _countersPerLevel


We're modding here, and we're modding in Reset and Inc. I think hash should not do any hashing and just return a uint32 and the counterFor method should be the only place we do any hashing.

Assuming you mean that hash shouldn't do any modding, and counterFor should be the only place we mod.

Yup, this is a legacy hold over from prior code form, will fix. Once we have counterFor, it can just call hash.Foo directly, rather than have any one-line func (*counters) hash imo

prashantv · 2017-02-02T01:54:56Z

zapcore/sampler.go

+	Facility
+
+	tick       time.Duration
+	counts     *counters


any advantage to using a pointer here rather than just a value, and then being able to remove newCounters?

We want the counts to be shared between parent and child loggers (so that sampling imposes a predictable limit on global logging load), so a pointer makes sense to me.

Ah of course, makes sense.

prashantv · 2017-02-02T01:55:50Z

internal/hash/fnv.go

+// FNV32 computes the 32-bit FNV-1 hash of s.
+func FNV32(s string) uint32 {
+	const (
+		offset32 = 2166136261


since the constants are the same, should we make them top-level constants?

Sure, but we'll probably remove the generality of the hash module, and just stick with either XSHRR or FNV32a to taste (and probably inlined in zapcore/sampler.go.

This internal/hash was a demonstrative step for this PR, unless you'd rather we keep it?

Ahh, I see, makes sense. This analysis is great, so I'd love to keep it somehow (either in this PR as is, and we use a separate PR to merge just the hash function we want), or in the internal/hash folder.

akshayjshah · 2017-02-02T04:45:09Z

internal/hash/bench_test.go

+	"go.uber.org/zap/internal/hash"
+)
+
+// XXX limit to go 1.7+, or stop using sub-benchmarks...


At this point, I'm comfortable with a separate pre-PR to drop Go 1.6 support. 1.8 is just around the corner, so we'll then support latest release and one older.

akshayjshah · 2017-02-02T04:46:11Z

internal/hash/bench_test.go

+	rand100s4 = randCross(rand100s3, rand100, 100000)
+)
+
+var testCorpii = []struct {


Latin nit: "testCorpora"

Sure, I didn't check or verify my pluralism, I'll borg yours ;-)

Or render it into English, "corpses". Or "bodies" in the Anglish.

akshayjshah · 2017-02-02T05:05:16Z

zapcore/level.go

@@ -49,6 +49,9 @@ const (
 	PanicLevel
 	// FatalLevel logs a message, then calls os.Exit(1).
 	FatalLevel
+
+	minLevel = DebugLevel
+	maxLevel = FatalLevel


I'm not always good about remembering, but we should underscore-prefix unexported globals.

akshayjshah · 2017-02-02T05:09:10Z

zapcore/sampler.go

+}
+
+func (c *counters) hash(key string) uint32 {
+	return hash.XSHRR(key) % _countersPerLevel


Assuming you mean that hash shouldn't do any modding, and counterFor should be the only place we mod.

akshayjshah · 2017-02-02T05:12:44Z

internal/hash/xshrr.go

+
+package hash
+
+// XSHRR hashes a string using the by threading each byte of it thru the PCG32


missing word here.

akshayjshah · 2017-02-02T05:24:28Z

zapcore/sampler.go

+	Facility
+
+	tick       time.Duration
+	counts     *counters


We want the counts to be shared between parent and child loggers (so that sampling imposes a predictable limit on global logging load), so a pointer makes sense to me.

akshayjshah · 2017-02-02T05:28:07Z

zapcore/sampler_bench_test.go

+		"raphidiferous",
+		"pooh",
+		"Tyndallize",
+	},


👏 for taking this idea much further than I would have. Dunno about Prashant, but I'm satisfied that XSHRR is a reasonable option.

akshayjshah · 2017-02-02T05:28:53Z

zapcore/facility.go

-	}
-	return s.Facility.Check(ent, ce)
-}
-


Can we also move the unit tests for the sampling facility to sampler_test.go?

akshayjshah · 2017-02-02T05:37:08Z

Love the IPython notebook - I had no idea GitHub rendered them so well. Some small nits aside, I'm satisfied with using either an internal FNV hash or XSHRR.

jcorbin · 2017-02-02T21:33:04Z

Okay, so unless @prashantv disagrees, I'll just roll with xshrr then; I don't feel strongly, other than in theory:

the MCG constant is bigger, so probably leads to more mixing for short strings
has a final xorshift and random-rotation "hardening" step (the entire difference between an [LM]CG and a PCG btw)
parameterizes easily, if we ever need to move up to a "min-sketch"

prashantv · 2017-02-02T22:13:41Z

Thanks for the awesome IPython notebook @jcorbin!

I don't have any strong opinions on hashes, I've seen fnv32 used more, which was why I originally suggested that. Looking at the output, it seems to me like:

fnv32a has a better distribution than xshrr for this corpus (higher min, lower max, lower stddev)
fnv32a is very slightly faster

The data seems to point towards fnv32a -- do you believe that xshrr will be better in more real world log scenarios Josh?

jcorbin · 2017-02-03T01:11:27Z

Yup, the ordering of fnv32a and xshrr varies per experiment run (recall the notebook randomly creates a new shuffling of /usr/share/dict/words every time you run it); so on distribution I'd call them "competitive".

Micro perf certainly is a ding on xshrr for sure.

While the larger constant and extra hardening step help my "sleep better at night", they've not really proven to be material for the (arguably unrealistic) test corpora.

I'm more than happy to just use fnv32a for now, writing down the pcg32-xsh-rr trick for later consideration if we decide that a min-sketch is better (since I'd be more confident parameterizing pcg32-xsh-rr to yield a family of hash functions, than fnv).

jcorbin · 2017-02-03T19:43:16Z

Updated:

dropped xshrr, switch to and inlined fnv32
switched to go.uber.org/atomic
addressed other feedback

jcorbin · 2017-02-03T19:46:01Z

zapcore/sampler.go

+
+func (c *counters) get(lvl Level, key string) *atomic.Uint64 {
+	i := lvl - _minLevel
+	j := fnv32a(key) % _countersPerLevel


@prashantv confirmed that the 1.7 compiler is sufficiently clever here.

The modulo compiles to & 4096-1:

. . 8a701: ANDL $0xfff, AX

As seen in:

(pprof) disasm counters..get Total: 34.89s ROUTINE ======================== go.uber.org/zap/zapcore.(*counters).get 1.33s 3.81s (flat, cum) 10.92% of Total 120ms 120ms 8a6a0: GS MOVQ GS:0x8a0, CX 10ms 10ms 8a6a9: CMPQ 0x10(CX), SP . . 8a6ad: JBE 0x8a728 80ms 80ms 8a6af: SUBQ $0x28, SP 40ms 40ms 8a6b3: MOVQ BP, 0x20(SP) 10ms 10ms 8a6b8: LEAQ 0x20(SP), BP . . 8a6bd: MOVQ 0x40(SP), AX 60ms 60ms 8a6c2: MOVQ AX, 0(SP) 50ms 50ms 8a6c6: MOVQ 0x48(SP), AX 60ms 60ms 8a6cb: MOVQ AX, 0x8(SP) 70ms 70ms 8a6d0: MOVZX 0x38(SP), AX 30ms 30ms 8a6d5: INCL AX 40ms 40ms 8a6d7: MOVB AL, 0x1f(SP) 60ms 2.54s 8a6db: CALL go.uber.org/zap/zapcore.fnv32a(SB) 50ms 50ms 8a6e0: MOVL 0x10(SP), AX 240ms 240ms 8a6e4: MOVQ 0x30(SP), CX 20ms 20ms 8a6e9: TESTB AL, 0(CX) 250ms 250ms 8a6eb: MOVZX 0x1f(SP), DX . . 8a6f0: MOVSX DL, DX . . 8a6f4: CMPQ $0x6, DX . . 8a6f8: JAE 0x8a721 . . 8a6fa: SHLQ $0xf, DX 40ms 40ms 8a6fe: ADDQ CX, DX . . 8a701: ANDL $0xfff, AX . . 8a706: CMPQ $0x1000, AX . . 8a70c: JAE 0x8a721 . . 8a70e: LEAQ 0(DX)(AX*8), AX 100ms 100ms 8a712: MOVQ AX, 0x50(SP) . . 8a717: MOVQ 0x20(SP), BP . . 8a71c: ADDQ $0x28, SP . . 8a720: RET . . 8a721: CALL runtime.panicindex(SB) . . 8a726: UD2 . . 8a728: CALL runtime.morestack_noctxt(SB) . . 8a72d: JMP go.uber.org/zap/zapcore.(*counters).get(SB) . . 8a732: INT $0x3 . . 8a733: INT $0x3 . . 8a734: INT $0x3 . . 8a735: INT $0x3 . . 8a736: INT $0x3 . . 8a737: INT $0x3 . . 8a738: INT $0x3 . . 8a739: INT $0x3 . . 8a73a: INT $0x3 . . 8a73b: INT $0x3 . . 8a73c: INT $0x3 . . 8a73d: INT $0x3 . . 8a73e: INT $0x3

Thanks for confirming Josh!

akshayjshah

generally looks great, just import order and that commented-out test.

akshayjshah · 2017-02-03T20:50:18Z

zapcore/sampler_bench_test.go

+		"lithotint",
+		"bargander",
+		"opisthographical",
+		"cacography",


I'm adding "cacography" to my day-to-day vocabulary.

akshayjshah · 2017-02-03T20:52:16Z

zapcore/sampler_test.go

+	"github.com/stretchr/testify/assert"
+	"go.uber.org/zap/internal/observer"
+	"go.uber.org/zap/testutils"
+	. "go.uber.org/zap/zapcore"


nit: import ordering. assert should be in a separate group.

akshayjshah · 2017-02-03T20:56:14Z

zapcore/sampler_test.go

+
+// 		assert.Equal(t, []spy.Log(nil), sink.Logs(), "Unexpected output when sampling with Check.")
+// 	}
+// }


Can we restore this test now? We should be able to use the internal exit package to make sure that we're actually sampling messages at panic and fatal too.

jcorbin · 2017-02-03T21:31:16Z

Turns out that the comment out test is just wholly obsolete:

always panicing or fatal exiting is zap.Logger's concern
we already have test for that, e.g. TestLoggerAlwaysPanics

prashantv · 2017-02-03T23:38:17Z

zapcore/sampler.go

+
+func (c *counters) get(lvl Level, key string) *atomic.Uint64 {
+	i := lvl - _minLevel
+	j := fnv32a(key) % _countersPerLevel


Thanks for confirming Josh!

prashantv · 2017-02-03T23:38:54Z

zapcore/sampler.go

+	Facility
+
+	tick       time.Duration
+	counts     *counters


Ah of course, makes sense.

prashantv · 2017-02-03T23:42:45Z

zapcore/sampler_test.go

+
+func TestSampler(t *testing.T) {
+	for _, lvl := range []Level{DebugLevel, InfoLevel, WarnLevel, ErrorLevel, DPanicLevel} {
+		sampler, logs := fakeSampler(DebugLevel, time.Minute, 2, 3)


I don't think we have any tests that ensure that separate levels use separate counters. I was able checkout the code, make the following patch:

--- a/zapcore/sampler.go +++ b/zapcore/sampler.go @@ -48,6 +48,7 @@ func (c *counters) Reset(lvl Level, key string) { func (c *counters) get(lvl Level, key string) *atomic.Uint64 { i := lvl - _minLevel j := fnv32a(key) % _countersPerLevel + j = 0 return &c[i][j] }

And all the tests still passed. Can we add a test for this case?

prashantv · 2017-02-04T00:05:47Z

zapcore/sampler.go

+	}
+
+	if n := s.counts.Inc(ent.Level, ent.Message); n > s.first {
+		if n == s.first+1 {


Changing this s.first+1 to s.first doesn't affect any tests, which means:

we should add more test coverage

we should better define what the behaviour should be for "first". I assume "first log every second" means if we log once a second, all logs would be output. However that's not the behaviour I'm seeing:

func TestSamplerReset(t *testing.T) { sampler, logs := fakeSampler(DebugLevel, time.Second, 1, 10000) writeIter(sampler, 1, DebugLevel) time.Sleep(2 * time.Second) writeIter(sampler, 2, DebugLevel) expected := buildExpectation(DebugLevel, 1, 2) assert.Equal(t, expected, logs.All()) }

For this PR, I think we should just add a bit more test coverage on the statement (so we know exactly what behaviour we currently have), and then open an issue with the behaviour we want. The ideal behaviour may be expensive with the current implementation since we'd have to start the timer as soon as we see the first message, which is definitely not ideal.

Good eyes - this is wrong.

In practice, I don't think that the correct behavior will be so bad, since logging only the first message each second is pretty extreme. I'm planning to make the default 100 first and 1000 thereafter.

Depends on which correct behaviour we're talking about I think. We have a few options:

current option of s.first+1 which I don't think has any benefits

s.first which will start the timer as soon as we're about to start dropping and will handle my above example better.

n == 1

Let's consider another example: we're logging every 0.5 seconds, and the parameters are s.first = 2, s.tick = time.Second s.thereafter = 100 (or some high number).

Option 3 would log every message (which seems like the ideal behaviour), option 2 would drop every 3rd message, option 1 would drop every 3rd and 4th message.

Option 3 will also start a timer every s.tick for every log message, which seems really bad. I think Option 2 is a good compromise between performance and sampling.

akshayjshah · 2017-02-04T03:10:19Z

I don't think that the test is obsolete, since the other tests aren't exercising Panic and Fatal at all (though level coverage is pretty spotty). We should be able to use the internal exit package to safely ensure that we're not exiting.

akshayjshah · 2017-02-04T16:10:55Z

The more I think about his, the less I like these options. Imagine sampling the first 100 messages and every 1000 thereafter every second. If I'm only logging 5 messages per sec, options 1 & 2 are occasionally going to drop all messages for a second. That's pretty bad. I think we can get around this by leveraging the time stamp on the Entry. I'll try to play with some ideas this weekend.

prashantv · 2017-02-05T00:09:31Z

Option 3 is definitely my preferred solution, but making it efficient will require rethinking the sampling logic (using timestamps). However, I don't think that change should be made as part of this PR. The refactoring to use bins + hash is unrelated to the sampling tick logic, so we should fix that separately.

jcorbin · 2017-02-06T17:50:42Z

Updated TestSamplerCheck to cover all levels (added ObservedLogs.TakeAll in passing)

See discussion on how the current sampler is wrong: #252 (comment) Instead of using `time.AfterFunc` and resetting timers, use timestamps on the entries, and maintain a sliding window for the current "tick" with atomics to track when the counters should be reset.

See discussion on how the current sampler is wrong: #252 (comment) Instead of using `time.AfterFunc` and resetting timers, use timestamps on the entries, and maintain a sliding window for the current "tick" with atomics to track when the counters should be reset. The alternate approach is to start the timer as soon as the first message for any bucket is logged, but that would cause more timers to be started, which costs an allocation as well as requiring a global lock on the timer heap.

* break out zapcore/sampler module * Benchmark sampling * Re-implement counts table to be lockless and static memory * Use two-level count binning to separate levels

jcorbin changed the base branch from master to dev January 24, 2017 20:10

akshayjshah reviewed Jan 25, 2017

View reviewed changes

jcorbin force-pushed the jcorbin.sampler_keying branch from ed47415 to d94151c Compare February 1, 2017 22:33

jcorbin force-pushed the jcorbin.sampler_keying branch 2 times, most recently from 54b7c28 to a338d78 Compare February 2, 2017 00:06

jcorbin changed the title ~~WIP: sampler counts and keying~~ Make sampler counts faster, and also binned by level Feb 2, 2017

prashantv reviewed Feb 2, 2017

View reviewed changes

akshayjshah reviewed Feb 2, 2017

View reviewed changes

jcorbin force-pushed the jcorbin.sampler_keying branch from a338d78 to 949c73d Compare February 3, 2017 18:26

jcorbin mentioned this pull request Feb 3, 2017

[HISTORICAL] Alternate hashes for sampler keying #272

Closed

jcorbin force-pushed the jcorbin.sampler_keying branch 2 times, most recently from 30032d2 to f59ce58 Compare February 3, 2017 19:42

jcorbin commented Feb 3, 2017

View reviewed changes

jcorbin force-pushed the jcorbin.sampler_keying branch from f59ce58 to fa59069 Compare February 3, 2017 20:02

akshayjshah suggested changes Feb 3, 2017

View reviewed changes

prashantv approved these changes Feb 4, 2017

View reviewed changes

jcorbin force-pushed the jcorbin.sampler_keying branch from e8c1be0 to f729bd1 Compare February 6, 2017 17:49

Joshua T Corbin added 20 commits February 7, 2017 16:37

internal/hash: dis-own the modulo

db3df3f

internal/hash: add FNV32 for distribution comparison

016992a

Add and benchmark counters2

bbf9cc8

Drop old counters

24b27e3

sampler_bench_test: shift to benching Sampler.Check

f2b63d2

Sampler: also bin counts by level

7d04421

counters.hash: drop mod

2b328d2

sampler: switch to, and inline implementation of, fnv32a

c05861e

zapcore: sunder-{min,max}Level

9410f25

counters: factor out get method

db68a72

counters: switch to go.uber.org/atomic

f16617d

Drop internal/hash

b471eb3

zapcore: breakout sampler_test from facility_test

d37e99a

sampler_test: nit tri-grouped imports

69d1382

sampler_test: drop obsolete TODO test

b088691

ObservedLogs: add TakeAll method

b710b1a

TestSamplerCheck: test at all levels

e6023df

Simplify ObservedLogs.TakeAll

9c38d8e

TestSamplerCheck: fix spell-o

85b6ca3

Fix facility_test broken import

7202086

jcorbin force-pushed the jcorbin.sampler_keying branch from ffd4ad9 to 7202086 Compare February 8, 2017 00:37

jcorbin merged commit 10589ed into dev Feb 8, 2017

jcorbin deleted the jcorbin.sampler_keying branch February 8, 2017 01:05

akshayjshah mentioned this pull request Feb 9, 2017

Re-consider sampler bucket keying #176

Closed

prashantv mentioned this pull request Feb 13, 2017

Rewrite sampler to fix oversampling #290

Merged


		package hash

		// XSHRR hashes a string using the by threading each byte of it thru the PCG32

Make sampler counts faster, and also binned by level #252

Make sampler counts faster, and also binned by level #252

Conversation

jcorbin commented Jan 24, 2017 • edited Loading

mention-bot commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcorbin commented Feb 1, 2017 • edited Loading

jcorbin commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

akshayjshah commented Feb 2, 2017

jcorbin commented Feb 2, 2017

prashantv commented Feb 2, 2017

jcorbin commented Feb 3, 2017

jcorbin commented Feb 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshayjshah 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

jcorbin commented Feb 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshayjshah commented Feb 4, 2017

akshayjshah commented Feb 4, 2017 via email • edited by prashantv Loading

prashantv commented Feb 5, 2017

jcorbin commented Feb 6, 2017

jcorbin commented Jan 24, 2017 •

edited

Loading

jcorbin commented Feb 1, 2017 •

edited

Loading

akshayjshah commented Feb 4, 2017 via email •

edited by prashantv

Loading