framework: add treeSelect (a new type of memoized selector) #20547

samouri · 2017-12-06T02:15:23Z

summary

Selectors are a vital part of any redux app. We have a powerful tool for creating cached selectors already called createSelector. createSelector though has a fatal flaw which reselect also has. Namely that it only stores a single dependent-->cache mapping at a time. This means that a too-common case will have disastrous cache hit rates and hurt performance.

reselect solves this by creating a cached selector per react component instance.
if we wanted that to translate to createSelector We would need to make a createSelectorCreator or change the current implementation of createSelector.

I've already created a PR trying to modify createSelector, but we decided that be too dangerous because so much of Calypso depends on it. Instead I've opted to make a new function in this PR with a slightly altered API that I think will help developers fall into the pit of success.

proposed api

treeSelect( getDependents, selector ):

getDependents: A function which maps over state and returns all of the relevant parts of state the selector needs. You should be creating an array whose elements are all the return of other selectors -- no computations allowed here. Returned dependents values must also all be objects because in the implementation they act as keys into a WeakMap.
selector: A function which takes in the same args as getDependents with one catch. Instead of being passed state as its first arg, it is given the results of getDependents. This forces you to declare all of your state-dependencies.

example

const getDependents = state => [ posts: state.posts ];
const selector = ( [ posts ], siteID ) => filter( posts, { siteId } );

const getSitePosts = treeSelect( selector, getDependents)

what is good about this approach?

By restricting the selector function so that it doesn't have access to state, it forces a selector to think critically about what it depends on. It also ensures that we only do the work of retrieving dependents once instead of twice like was usually done in createSelector.
By using a WeakMap, we can garbage collect as soon as a state subtrees disappear.
By using a dependency tree, we can efficiently cache on very granular dependencies with an O(dependencies.length) retrieval time.

References

README
three prs already made to solve the same issue:

matticbot · 2017-12-06T02:15:24Z

Test live: https://calypso.live/?branch=add/framework/create-cached-selector

samouri · 2017-12-11T23:55:04Z

~~should the the dependency map convert into a map when a primitive value is added (LazyWeakMap) that way a selector may depend on a non-object value that is in state?~~

Update: added support for non-object keys via MixedMap. Its possible we could slightly optimize for specific situations in which getDependents returns a smattering of objects and keys by sorting the dependents so that objects are first when using them as keys to the cache. I don't think it'd be helpful enough to warrant implementing though.

dmsnell · 2017-12-18T23:37:24Z

client/lib/create-cached-selector/index.js

+	constructor( init ) {
+		forEach( init, ( [ key, val ] ) => {
+			this.mapForKey( key ).set( key, val );
+		} );


did you see this on MDN

var kvArray = [['key1', 'value1'], ['key2', 'value2']]; // Use the regular Map constructor to transform a 2D key-value Array into a map var myMap = new Map(kvArray);

yep! which is why i needed to bake in some custom support for it.

this constructor takes all the values that have object keys and throws them into the weakmap. the rest go into the regular map

ah right. good good

dmsnell · 2017-12-18T23:47:37Z

client/lib/create-cached-selector/index.js

+
+	delete( k ) {
+		this.mapForKey( k ).delete( k );
+		return this;


do you anticipate chaining these calls like cache.clear().set().set() etc…?

I find that it's helpful to not return this when we are mutating the underlying object because the code itself kind of indicates based on return value if this is a side-effecting action or a pure one. moment() is a good example where it supports chaining but the underlying object mutates throughout the chain. it gives the appearance of being safe from a purity standpoint, but that can lead to mistakes on that assumption.

const then = moment( thenDate ) const nextDay = then.startOf( 'day' ).add( 1, 'day' ) const prevDay = then.startOf( 'day' ).subtract( 1, 'day' )

^^^ looks fine until you realize that prevDay is actually the start of thenDate

const then = moment( thenDate ) const nextDay = then nextDay.startOf( 'day' ).add( 1, 'day' )

^^^ wait, that's odd, something must be up - I wonder if there's implicit mutation happening…

do you anticipate chaining these calls like cache.clear().set().set() etc…?

I don't think returning this indicates purity...I don't usually assume purity unless its specifically stated.

The thing I would want most is to adhere to the WeakMap specification. Some of them return this and some don't

set --> returns this

get --> returns the value associated with the key

clear --> returns undefined

delete --> returns true/false depending on if the key was found and removed

odd. carry on.

dmsnell · 2017-12-18T23:58:43Z

client/lib/create-cached-selector/index.js

+import warn from 'lib/warn';
+
+/*
+ * A map that is Weak with objects but Strong with primitives


this is a great description

why do we have this? It strikes me as a dangerous idea, as anything that ends up in the strong map is effectively a memory leak. The point of using a WeakMap is to weakly hold the keys so that when they go out of scope elsewhere, we can GC them. If we allow some keys to end up in the strong map, they're never be GC'd.

For true primitives, that's probably ok, they don't take up much space, but then there's also not much benefit to caching them this way. For strings, it could get ugly, as they're effectively unbounded in size.

So why do we need this?

I was torn on whether or not to allow primitives.
There are perfectly valid use-cases for having a primitive as a dependent.
Imagine state looked like this:

state = { priceOfSite: 5.40, sites: [ ... ], ... }

If primitives weren't allowed, then you wouldn't be able to write a cached selector that used priceOfSite. For example:

// would be invalid without MixedMap const getTotalPrice = createCachedSelector( ({ sites, price }) =>return price * sites.length, state => ({ sites: state.sites, price: state.priceOfSite }) )

The solutions i could think of are:

restrict this cachedSelector function to to selectors that depend on objects within state, no primitives allowed.

MixedMap

selector-ception: Do something clever with primitives. If we notice that a dependent is a primitive, then create a cached function for it so that it will return the value wrapped in an object { value: 'primitive-value', objectified: true } and make sure to return a referentially equal one whenever the value hasn't changed.

@blowery we can't use primitive values as keys in WeakMaps sadly. that is, this is getting as much out of the WeakMap as we can and only falling back to classical maps when JavaScript won't let us use the WeakMap

our alternative is probably to skip caching these values

dmsnell · 2017-12-18T23:59:30Z

client/lib/create-cached-selector/index.js

+ * @return {Function}               Cached selector
+ */
+export default function createCachedSelector( { selector, getDependents } ) {
+	if ( ! isFunction( selector ) || ! isFunction( getDependents ) ) {


cough TypeScript

export default function createCachedSelector( selector: Function, getDependents: Function )

would be nice :)

dmsnell · 2017-12-19T00:00:00Z

client/lib/create-cached-selector/index.js

+
+	const cache = new MixedMap();
+
+	const cachedSelector = function( state, ...args ) {


function( { state, ...args } )

dmsnell · 2017-12-19T00:00:59Z

client/lib/create-cached-selector/index.js

+	const cachedSelector = function( state, ...args ) {
+		const dependents = getDependents( state, ...args );
+		const sortedDependentsArray = Object.keys( dependents )
+			.sort()


this is not what you think it is, it is also not stable

we discussed this on slack. the keys will always be unique strings therefore Array#sort does what we want -- to produce the same array for the same object of dependents every time

The enumeration order of object properties is guaranteed to be the same as the order in which the properties were created. So you don't need to sort the keys nearly as often as you might think. You still need to make cases like this behave correctly:

getDependents = state => cond ? { dep1, dep2 } : { dep2, dep1 };

The fact that you require a particular property order, and that the keys are not used by the createCachedSelector for anything, suggests that the data structure you're really looking for is an array, not an object:

createCachedSelector( ( [ dep1, dep2 ], ...args ) => combine( dep1, dep2, args ), ( state, ...args ) => [ dep1, dep2 ] );

Or, even better in my opinion, follow the widely used reselect function signature:

createCachedSelector( ( state, ...args ) => computeDep1, ( state, ...args ) => computeDep2, ( dep1, dep2, ...args ) => combine( dep1, dep2, args ) )

The disadvantage here is that it's hard to add a getKey function as another argument, if you ever needed it. And the code in #21507 suggests that you might need it.

The fact that you require a particular property order, and that the keys are not used by the createCachedSelector for anything, suggests that the data structure you're really looking for is an array, not an object:

They keys are used! Not in a way that is necessary, but in a way that I think makes for a nice api. The return value of getDependents becomes the first parameter to selector

So for example:

getDependents = (state, siteId) => ({ site: getSite(siteId), posts: getPosts(siteId), comments: getComments(siteId)}); selector = ({site, posts, comments}) => doThings(site, posts, comments)

All that said, I may be alone in how much I love using objects for named parameters, so I'll switch to an array that may make more sense here for all the reasons you cited:

getDependents = (state, siteId) => [getSite(siteId), getPosts(siteId), getComments(siteId)]; selector = ([site, posts, comments]) => doThings(site, posts, comments)

While the reselect api is really pleasant, I agree that we may need to sometimes use custom cache key generation which discounts that as an option. If we were rebuilding calypso from scratch we wouldn't, but in order to easily transition code that already depends on objects as selector args then we'll need it.

dmsnell · 2017-12-19T00:04:55Z

client/lib/create-cached-selector/index.js

+
+		if ( process.env.NODE_ENV !== 'production' ) {
+			if ( some( args, isObject ) ) {
+				warn( 'Do not pass complex objects as arguments to a cachedSelector' );


why is this important? we have a number of existing selectors which take objects as arguments after state

It's part of the current createSelector as well.

If an argument is an object literal then it shouldn't be used for caching because it's a new object on each call. if it is in object from state, then it should be part of getDependents

because it's a new object on each call

assumption

still, it may be part of createSelector, but what I understand is that we're saying, "if you pass any object into a selector we won't let you cache things" when it seems like it could be valid

like what if the dependents are based on a property in that object, or unrelated to them at all?

I think this might occur when we pass things like formatting info to the selectors and stuff like that where the last step in the selector is to do some specific twiddling based on the passed option bag

assumption

object literals are by definition new every time used (like the options bag you suggested).

The reason we can't use them is because there is no standard way to create a unique key for objects. for an ordered list of primitives its simple (args.join()).

If this ends up being a need then we can allow for custom cacheKey generation...but i'd rather stay away for this first iteration because YAGNI

can drop the "complex" part as even the empty object qualifies here. Maybe "You can only pass primitives as arguments to a cached selector"

dropped the word complex in the latest update.

dmsnell · 2017-12-19T00:08:54Z

client/lib/create-cached-selector/index.js

+		let currCache = cache;
+		forEach( sortedDependentsArray, dependent => {
+			if ( ! currCache.has( dependent ) ) {
+				currCache.set( dependent, new MixedMap() );


this line may mutate cache but it doesn't look like it from here. this is confusing to me and relates to my comment above in MixedMap

I think what's happening is that we are using currCache more as a cursor than as an object and we're moving that cursor through cache. if this is happening, then I think we can rearrange the code to more explicitly reflect that reality

I'd love input on how to do that! I have no idea 🤔

I like the word cursor for what I'm doing with currCache, its more descriptive

using cursor as the name would help I think.

this is really a kind of insertion on a prefix tree where the dependents are the atoms forming the nodes. things could potentially be more clear simply with the proper messaging around them…

const upsert = ( cache, value ) => { if ( ! cache.has( value ) ) { cache.set( value, new MixedMap() ) } return cache.get( value ); } // insert dependents into cache tree sortedDependentsArray.reduce( upsert, cache )

maybe even something along these lines…

@samouri did you see this last comment as it pertains to the naming of the cursor? (it eliminates it, for example)

Now I do! I think its brilliant! I'm just changing the name to insertIfAbsent but otherwise keeping the logic intact.

dmsnell · 2017-12-19T00:12:39Z

client/lib/create-cached-selector/test/index.js

+jest.mock( 'lib/warn', () => jest.fn() );
+
+describe( 'index', () => {
+	describe( '#createCachedSelector', () => {


I think that the root of this file will already appear as create-cached-selector so this two-level descent may report in the tests as create-cached-selector/index/#createCachedSelector

I created a couple of errors to show how it outputs:

What do you think?

dmsnell · 2017-12-19T00:29:28Z

client/lib/create-cached-selector/test/index.js

+			getSitePosts( reduxState, 2916284 );
+
+			expect( selector.mock.calls.length ).toBe( 1 );
+		} );


A good partner-test here might be one to verify that this gets called twice when the arguments are different. This test has a high risk of false positives on the behavior of the function.

test( 'should call selector when making non-cached calls', () => { getSitePosts( reduxState, 2916284 ) getSitePosts( reduxState, 2916284 + 1 ) expect( selector.mock.calls.length ).toBe( 2 ); } )

I think the current test named that should return the expected value of differing arguments does this.

I like your test naming better though, I'll switch

dmsnell · 2017-12-19T00:33:52Z

client/lib/create-cached-selector/test/index.js

+			getSitePosts( state, 1, [] );
+
+			expect( warn.mock.calls.length ).toBe( 3 );
+		} );


as per my earlier comment, I question the validity of this constraint

samouri · 2018-01-11T22:07:31Z

I just tested out the optimization gains that can be had by using this selector and they were stark.
The test is here: #21468

	createSelector	treeSelect
wasted renders	351	8

cc @blowery re. conversations perf boost

dmsnell · 2018-01-11T23:41:36Z

client/lib/create-cached-selector/README.md

+const getDependents = state => ( { posts: state.posts } );
+const selector = ( { posts }, siteId ) => filter( posts, { siteId } );
+
+const getSitePosts = createCachedSelector( selector, getDependents );


wouldn't it make more sense to pass getDependents first since they are logically prior to the selector?

this isn't a big deal either way, but something feels odd about this ordering

I agree with you wholeheartedly. I only picked this order to match createSelector's api.
Since we are diverging anyway, may as well diverge a bit more ;).

dmsnell · 2018-01-14T00:38:51Z

@samouri what do you need to get this out the door? I think getting it in soon will be a big boon for us

samouri · 2018-01-15T08:36:04Z

@samouri what do you need to get this out the door? I think getting it in soon will be a big boon for us

The main question I'd been pondering was how to handle primitives. For now I think its safest to disallow them completely. We may realize that we need to allow primitives at some point, but until then I'd rather explicitly not support it. I also have a gut feeling that we'll be able to work around the restriction by having selectors grab items higher up in state instead.

So the changes i've made based on feedback are:

Swapped the arguments order (@dmsnell)
Made getDependents return an array instead of an object (@jsnajdr)
Disallow primitives as dependents, and delete MixedMap (@blowery).
Renamed from createCachedSelector --> treeSelect

Two things that could happen in in future prs is allowing for custom cacheKeys and re-enabling primitives.

jsnajdr · 2018-01-15T09:02:46Z

client/lib/tree-select/index.js

+			cursor.set( key, selector( dependents, ...args ) );
+		}
+
+		return cursor.get( key );


Here you already know the return value (it's the one that you just inserted into the map), so you can avoid the extra map lookup here.

Selectors are called zillion times on every possible Redux dispatch, so even a tiny optimization like this is very likely to be detectable.

it'll require an extra line to assign the value to a variable. I'll do that

One more line for Jake, one less function call for the CPU. Looks like a good tradeoff 😜

maybe. we probably shouldn't make too many assumptions about this though due to JS engine optimization. probably less CPU work but not definitely. maybe not significant but probably not.

@dmsnell I'm pretty sure that the call to Map.prototype.get would be actually executed by the engine and that it takes more than zero CPU cycles ;)

jsnajdr · 2018-01-15T09:05:03Z

client/lib/tree-select/index.js

+ */
+function insertDependentKey( map, key, currentIndex, arr ) {
+	const NewMap = currentIndex === arr.length - 1 ? Map : WeakMap;
+	return insertIfAbsent( map, key, new NewMap() );


Create the new map only when you're really going to need it. Most of them will be thrown away.

thats a good point 👍

jsnajdr · 2018-01-15T09:05:48Z

client/lib/tree-select/index.js

+	}
+
+	map.set( key, value );
+	return map.get( key );


Just return the value without an extra map lookup.

jsnajdr · 2018-01-15T09:08:41Z

client/lib/tree-select/index.js

+ * @return {Function}               Cached selector
+ */
+export default function treeSelect( getDependents, selector ) {
+	if ( ! isFunction( getDependents ) || ! isFunction( selector ) ) {


This check could be done only in development mode.

ok, adding check

Also added a unit test for this in https://github.com/Automattic/wp-calypso/pull/20547/files#diff-e1fde4b19690dc0c18ea9cd84f272397R90

jsnajdr · 2018-01-15T10:09:51Z

client/lib/tree-select/index.js

+ * Note: Inserts WeakMaps except for the last map which will be a regular Map.
+ */
+function insertDependentKey( map, key, currentIndex, arr ) {
+	const NewMap = currentIndex === arr.length - 1 ? Map : WeakMap;


I figured out this function only after stepping through it in debugger. I was wondering why there is a classic Map for the last dependent. Its results are also objects, aren't they? And then I understood that it's all offset by one and the last Map is for string keys coming from ...args :) Some explanatory comment would be helpful here.

nailed it! I'll add in a comment explaining that

I adjusted the comment slightly. I'm having trouble coming up with a good explanation so feel free to rewrite it (or post here a better one and I'll modify it)

dmsnell · 2018-01-15T20:05:34Z

client/lib/tree-select/README.md

+# `treeSelect`
+
+This module exports a function which creates a cached state selector for use with the Redux global application state. It is a good idea to use this function over plain selectors whenever either the computation over state or React's rerenders are expensive.
+It is called `treeSelect` because it internally uses a [WeakMap](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakMap) based dependency tree in order to allow the gc to free memory without explicitly clearing the cache.


It is called treeSelect because it internally uses a [WeakMap]

That doesn't make sense. I think we're missing the logical warrant or we probably just need to remove the reference to WeakMap or add it later when it's not tied to tree

read a few more words and then I think it makes sense:

It is called treeSelect because it internally uses a WeakMap based dependency tree.

I can switch around the wording so that the word tree is emphasized more though. Something like:

it is called treeSelect because it internally uses a tree of dependencies in order to allow the gc to free memory without explicitly clearing the cache.

read a few more words and then I think it makes sense:

I read those words and the sense never came

Something like:

Removing the WeakMap mention as you did makes sense.

dmsnell · 2018-01-15T20:15:22Z

client/lib/tree-select/README.md

+For example, imagine that our state contains post objects, each of which are assigned to a particular site. Retrieving an array of posts for a specific site would require us to filter over all of the known posts. While this would normally trigger a re-render in React, we can use `treeSelect` to create a cached version of the selector which can return a referentially equal object:
+
+```js
+const getDependents = state => ( { posts: state.posts } );


while I think dependent is more proper I find myself constantly making typos in createSelector code because it uses dependant with an a

🤷‍♂️

dmsnell · 2018-01-15T20:17:35Z

client/lib/tree-select/README.md

+
+```js
+const getDependents = state => ( { posts: state.posts } );
+const selector = ( { posts }, siteId ) => filter( posts, { siteId } );


does this need to be updated to an array?

yes it does!

dmsnell · 2018-01-15T20:20:22Z

client/lib/tree-select/index.js

+		return value;
+	};
+
+	return cachedSelector;


why store it as a separate binding vs just returning the function expression?

this is the nittiest of nits :)

There is no reason any more. beforehand it was so that we could add properties to cachedSelector with cachedSelector.cache = .... Since its not necessary anymore I can get rid of it

dmsnell · 2018-01-15T20:23:41Z

client/lib/tree-select/index.js

+		// create a dependency tree for caching selector results.
+		// this is beneficial over standard memoization techniques so that we can
+		// garbage collect any values that are based on outdated dependents
+		const cursor = dependents.reduce( insertDependentKey, cache );


at this point I don't see much value in the cursor name because we're not "cursing" around anymore.

what about just calling it something like leafCache or leaf or callCache or something like that?

i like leafCache

dmsnell · 2018-01-15T20:25:23Z

client/lib/tree-select/index.js

+		// garbage collect any values that are based on outdated dependents
+		const cursor = dependents.reduce( insertDependentKey, cache );
+
+		const key = args.join();


this is something I think is still dubious. it seems like there are reasons why passing objects makes sense (like a moment time value or some kind of query object). I guess I'm fine leaving it as-is as long as we are open to adjusting as developers hit the stringable-data-type wall

this is something that getCacheKey() can actually work around in createSelector

we're specifying a kind of relational division operation to collapse "unique" invocations

I already called out that we may want to add it in a future pr:

Two things that could happen in in future prs is allowing for custom cacheKeys and re-enabling primitives.

for now, less is more

dmsnell

jsnajdr

Awesome! Thanks for working on this 🚀

jsnajdr · 2018-01-15T21:20:52Z

client/lib/tree-select/index.js

+ */
+function insertDependentKey( map, key, currentIndex, arr ) {
+	if ( map.has( key ) ) {
+		return map.get( key );


You could shave off another lookup by only calling map.get and checking for a falsy result.

samouri requested a review from spen December 6, 2017 02:15

samouri force-pushed the add/framework/create-cached-selector branch from af1584f to ad595cf Compare December 6, 2017 02:17

samouri changed the title ~~Add/framework/create cached selector~~ add createCachedSelector Dec 6, 2017

samouri changed the title ~~add createCachedSelector~~ framework: add createCachedSelector Dec 6, 2017

samouri requested review from blowery and dmsnell December 6, 2017 02:50

samouri force-pushed the add/framework/create-cached-selector branch from b032177 to 412d8d1 Compare December 6, 2017 03:05

samouri self-assigned this Dec 6, 2017

samouri changed the title ~~framework: add createCachedSelector~~ framework: add createCachedSelector (treeselect) Dec 6, 2017

samouri changed the title ~~framework: add createCachedSelector (treeselect)~~ framework: add createCachedSelector (treeSelect) Dec 6, 2017

samouri mentioned this pull request Dec 6, 2017

createSelector: allow for per-key dependents #19771

Closed

4 tasks

samouri requested a review from jsnajdr December 6, 2017 03:21

designsimply added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 11, 2017

samouri added the [Status] In Progress label Dec 11, 2017

samouri removed the [Status] In Progress label Dec 17, 2017

dmsnell reviewed Dec 18, 2017

View reviewed changes

dmsnell reviewed Dec 19, 2017

View reviewed changes

dmsnell reviewed Jan 11, 2018

View reviewed changes

samouri changed the title ~~framework: add createCachedSelector (treeSelect)~~ framework: add treeSelect (a new type of memoized selector) Jan 15, 2018

samouri added 2 commits January 15, 2018 03:10

address feedback

78a00c4

add checks for production

2039a43

samouri added 2 commits January 15, 2018 03:52

goodbye mixedmap

e4f6eb1

update docs

12ab35e

jsnajdr reviewed Jan 15, 2018

View reviewed changes

samouri added 3 commits January 15, 2018 13:36

address jarda's feedback

46720f5

adjust comment

6fedd60

improve test name

46ea23a

dmsnell mentioned this pull request Jan 15, 2018

WIP: Allow createSelector to store a cache #21547

Closed

dmsnell reviewed Jan 15, 2018

View reviewed changes

address @dmsnells points

b058d3f

dmsnell approved these changes Jan 15, 2018

View reviewed changes

jsnajdr approved these changes Jan 15, 2018

View reviewed changes

samouri merged commit 8826135 into master Jan 15, 2018

samouri deleted the add/framework/create-cached-selector branch January 16, 2018 03:31

spen mentioned this pull request Feb 2, 2018

CreateSelector: AddcachePerKey flag #20330

Closed

aduth mentioned this pull request Feb 9, 2018

Use per-dependants-set WeakMap-based cache aduth/rememo#1

Merged

sirreal removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 27, 2018

aduth mentioned this pull request Nov 28, 2018

Improve typing performance by splitting attributes in state tree. WordPress/gutenberg#12312

Merged

4 tasks


		const cache = new MixedMap();

		const cachedSelector = function( state, ...args ) {

framework: add treeSelect (a new type of memoized selector) #20547

framework: add treeSelect (a new type of memoized selector) #20547

Conversation

samouri commented Dec 6, 2017 • edited Loading

matticbot commented Dec 6, 2017

samouri commented Dec 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blowery Dec 21, 2017 • edited Loading

Choose a reason for hiding this comment

samouri Dec 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samouri Dec 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samouri Dec 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samouri Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell Dec 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

samouri commented Jan 11, 2018 • edited Loading

Choose a reason for hiding this comment

samouri Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

dmsnell commented Jan 14, 2018

samouri commented Jan 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samouri Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samouri Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samouri commented Dec 6, 2017 •

edited

Loading

samouri commented Dec 11, 2017 •

edited

Loading

dmsnell Dec 18, 2017 •

edited

Loading

blowery Dec 21, 2017 •

edited

Loading

samouri Dec 21, 2017 •

edited

Loading

samouri Dec 19, 2017 •

edited

Loading

samouri Dec 19, 2017 •

edited

Loading

samouri Dec 20, 2017 •

edited

Loading

dmsnell Dec 19, 2017 •

edited

Loading

samouri commented Jan 11, 2018 •

edited

Loading

samouri Jan 15, 2018 •

edited

Loading

samouri Jan 15, 2018 •

edited

Loading

samouri Jan 15, 2018 •

edited

Loading

dmsnell Jan 15, 2018 •

edited

Loading

samouri Jan 15, 2018 •

edited

Loading