Pay go setup relationships #4821

hjdivad · 2017-02-24T01:47:16Z

The goal here is to do as little work as we can with relationship payloads until the relationship is actually accessed. In an extreme case, where many relationship payloads are pushed, but no relationships are accessed, all we want to do is stash the payload.

So what we want is

to spend less time on relationships we don't access
to not spend more time (even if we spend it later on) for relationships we do access

Testing on my early 2015 13" macbook pro I get a substantial improvement on pushing relationships that aren't accessed.

For my test case of 2800 models and 4800 relationships lazy relationships is about 2.2x as fast when no relationship is accessed.

master

pr

For my test case of 2800 models and 4800 relationships lazy relationships costs about as much as master

master

pr

stefanpenner

I've done a partial review, will do more next.

stefanpenner · 2017-03-02T22:54:24Z

addon/-private/system/relationships/relationships-payloads.js

+  */
+  _getRelationshipPayloads(modelName, relationshipName) {
+    // TODO: lightschema this
+    let modelClass = this._store._modelFor(modelName);


should this be _modelFor over store.modelFor?

It's strictly internal, so by the time we get here modelName should already be normalized. I'm fine with normalizing here (ie calling this._store.modelFor instead of this._store._modelFor) for simplicity's sake.

stefanpenner · 2017-03-02T22:56:24Z

addon/-private/system/relationships/relationships-payloads.js

+export default class RelationshipsPayloads {
+  constructor(store) {
+    this._store = store;
+    this._map = {};


thoughts on renaming this to cache with a comment of // cache for relationshipPayloads?

stefanpenner · 2017-03-02T22:59:11Z

addon/-private/system/relationships/relationships-payloads.js

+  */
+  _getRelationshipPayloads(modelName, relationshipName) {
+    // TODO: lightschema this
+    let modelClass = this._store._modelFor(modelName);


I wonder if _getRelationshipPayload should take also take relationshipsByName as an argument.

In push and unload the callers of this method invoke it in a loop and for each iteration of the loop modelClass is constant, but yet each iteration must repeatedly lookup modelClass modelClass.relationshipsByName.

stefanpenner · 2017-03-02T23:00:47Z

tests/integration/relationships/inverse-relationships-test.js

@@ -463,8 +463,8 @@ testInDebug("Inverse relationships that don't exist throw a nice error for a bel
  }, /We found no inverse relationships by the name of 'testPost' on the 'user' model/);
 });

-test("inverseFor is only called when inverse is not null", function(assert) {
-  assert.expect(2);
+test("inverseFor short-circuits when inverse is null", function(assert) {


Now that we have both inverseFor and _inverseFor should we test both of them?

Looking more closely, we don't need to introduce _inverseFor; the point was just to separate the API from the work so we can test that the expensive path is not taken, but this separation already exists between inverseFor and _findInverseFor, so I've updated the tests to assert that inverseFor short-circuits _findInverseFor rather than _inverseFor.

stefanpenner · 2017-03-02T23:02:27Z

addon/-private/system/model/internal-model.js

@@ -505,6 +505,7 @@ export default class InternalModel {
    assert("Cannot destroy an internalModel while its record is materialized", !this.record || this.record.get('isDestroyed') || this.record.get('isDestroying'));

    this.store._removeFromIdMap(this);
+    this.store._relationshipsPayloads.unload(this.modelName, this.id);


should we unify 507 and 508 into this.store._internalModelDestroyed(internalModel) that way, we don't need to leak _removeFromIdMap and _relationshipsPayload private members to internalModel

stefanpenner · 2017-03-02T23:03:29Z

addon/-private/system/model/internal-model.js

@@ -988,6 +991,8 @@ export default class InternalModel {
  */
  adapterDidCommit(data) {
    if (data) {
+      this.store._relationshipsPayloads.push(this.modelName, this.id, data.relationships);


https://github.com/emberjs/data/pull/4821/files#r104052721

stefanpenner · 2017-03-02T23:03:58Z

addon/-private/system/relationships/relationships-payloads.js

+      }
+    }
+
+  @class RelationshipsPayloads


likely should be marked as @private

stefanpenner · 2017-03-02T23:15:39Z

addon/-private/system/relationships/relationships-payloads.js

+  */
+  _inverseFor(modelName, relationshipName) {
+    // TODO: lightschema this
+    let modelClass = this._store._modelFor(modelName);


should this by modelFor?

#4821 (comment)

stefanpenner · 2017-03-02T23:20:42Z

addon/-private/system/relationships/relationships-payloads.js

+    }
+  }
+
+  _hasRecordForId(store, type, id) {


this method does not appear to rely on this, we should likely move it off the prototype and just to a module local helper function. Or, we should make it use this._store internally.

When I see methods on this I feel it suggests it needs access to this.

stefanpenner · 2017-03-02T23:41:29Z

addon/-private/system/relationships/relationships-payloads.js

+  get _lhsRelationshipIsMany() {
+    if (this.__lhsRelatinoshipIsMany === undefined) {
+      if (this._lhsModelName) {
+        let modelClass = this._store.modelFor(this._lhsModelName);


I believe both LHS and RHS modelClass's are loaded when RelationshipPayload is instantiated, should we just pass them in at construction? At that point, do we already have the relationship descriptor as well, or is that what we want lazy?

I believe both LHS and RHS modelClass's are loaded when RelationshipPayload is instantiated, should we just pass them in at construction? At that point, do we already have the relationship descriptor as well, or is that what we want lazy?

This is not necessarily the case. inverseFor does not load the inverse class. Even when we check for the inverse relationship, we won't cause the inverse class to be loaded.

This is not necessarily the case. inverseFor does not load the inverse class. Even when we check for the inverse relationship, we won't cause the inverse class to be loaded.

Okay what I said is just wrong. I check this but obviously did not do so correctly. I'll push the classes down to RelationshipPayloads

I'm just passing in the relationship meta since all we want off of it is to know whether or not this side of the relationship is a many side.

hjdivad · 2017-03-10T00:11:51Z

@stefanpenner i've responded to your review + done some additional perf tweaks. I've kept the changes separate, in c53997e if you want to look at only the changes

stefanpenner · 2017-03-10T02:12:28Z

addon/-private/system/model/model.js

-    return this._inverseFor(name, store);
-  },
-
-  _inverseFor(name, store) {
    let inverseMap = get(this, 'inverseMap');
    if (inverseMap[name]) {


should this also check for null and if it is null return null, rather then failing this branch and looking it up again on 1315 only to find null again?

stefanpenner · 2017-03-10T02:13:20Z

addon/-private/system/relationships/state/create.js

-      if (rel) {
-        relationship = relationships[key] = createRelationshipFor(internalModel, rel, internalModel.store);
-      }
+      if (!rel) { return undefined; }


should we move this return to before line 60, that way we don't bother getting the relationshipPayload at all?

stefanpenner · 2017-03-10T02:13:26Z

addon/-private/system/relationships/state/create.js

-          relationship.pushFromInverse(payload);
-        }
+      if (relationshipPayload) {
+        this.internalModel.store._backburner.join(function() {


i'm surprised this join is required, whats it all about?

This is the thing that batches canonical flushes. In master this batching happens within _push

stefanpenner · 2017-03-10T02:15:12Z

addon/-private/system/relationships/state/relationship.js

+    if (this.inverseKey) {
+      let relationships = internalModel._relationships;
+      let relationshipExisted = relationships.has(this.inverseKey);
+      let relationship = relationships.get(this.inverseKey);


should we move this line to 72, that way if the branch is not taken we don't lazily create the relationship at all.

No; this PR preserves the invariant that if a relationship is initialized, its inverse is also initialized.

We could change the invariant, but it would add complexity in the case where a relationship A gets its initial state from the payload of its inverse B, B is initialized and then mutated before A is initialized.

This is something we could do but it seems better to leave that for a separate PR

stefanpenner · 2017-03-10T02:16:33Z

tests/integration/relationships/has-many-test.js

@@ -2839,6 +2839,7 @@ test("deleted records should stay deleted", function(assert) {
    message.destroyRecord()
  });

+  self.halt=true;


stefanpenner · 2017-03-10T02:16:37Z

tests/integration/relationships/has-many-test.js

@@ -2853,7 +2854,12 @@ test("deleted records should stay deleted", function(assert) {
        }
      }]
    });
+    self.halt=false;


igorT · 2017-03-13T21:10:43Z

addon/-private/system/model/internal-model.js

@@ -518,6 +518,8 @@ export default class InternalModel {

  setupData(data) {
    heimdall.increment(setupData);
+    this.store._internalModelHasRelationshipData(this.modelName, this.id, data.relationships);


This sounds like a boolean returning method, maybe something like received or process or something would be better?

_internalModelDidGetGetRelationshipData?

igorT · 2017-03-13T21:11:58Z

addon/-private/system/model/model.js

-      return inverse;
+      let relationship = get(this, 'relationshipsByName').get(name);
+      if (!relationship) {
+        return inverseMap[name] = null;


Can we not assign & return at the same time

igorT · 2017-03-13T21:14:50Z

addon/-private/system/model/model.js

+      }
+
+      let options = relationship.options;
+      if (options && options.inverse === null) {


Can you leave a comment here please

igorT · 2017-03-13T21:17:47Z

addon/-private/system/relationships/relationships-payloads.js

+let get = Ember.get;
+
+/**
+  Manages relationship payloads for a given store.  Acts as a single source of


Can you expand on this comment to say that this is only until the relationship is initialized

igorT · 2017-03-13T21:20:43Z

addon/-private/system/relationships/relationships-payloads.js

+  get(modelName, id, relationshipName) {
+    let modelClass = this._store._modelFor(modelName);
+    let relationshipsByName = get(modelClass, 'relationshipsByName');
+    let relationshipPayloads = this._getRelationshipPayloads(modelName, relationshipName, modelClass, relationshipsByName, false);


Can you leave a comment as to why this signature is so long

igorT · 2017-03-13T21:22:25Z

addon/-private/system/relationships/relationships-payloads.js

+
+    let modelClass = this._store._modelFor(modelName);
+    let relationshipsByName = get(modelClass, 'relationshipsByName');
+    for (let key in relationshipsData) {


Is this safe to do?

ah yeah, i'll switch this to Object.keys().forEach

igorT · 2017-03-13T21:24:26Z

addon/-private/system/relationships/relationships-payloads.js

+    let modelClass = this._store._modelFor(modelName);
+    let relationshipsByName = get(modelClass, 'relationshipsByName');
+    for (let key in relationshipsData) {
+      let relationshipPayloads = this._getRelationshipPayloads(modelName, key, modelClass, relationshipsByName, true);


Maybe passing the relationship would simplify things?

unfortunately we'd need to also pass the inverse relationship, which we don't always need to compute

igorT · 2017-03-13T21:28:17Z

addon/-private/system/relationships/relationships-payloads.js

+    return this._cache[key];
+  }
+
+  _initializeRelationshipPayloads(modelName, relationshipName, modelClass, relationshipsByName) {


This method could use comments or breaking apart

igorT · 2017-03-13T21:34:24Z

addon/-private/system/relationships/relationships-payloads.js

+    this._rhsRelationshipMeta = inverseRelationshipMeta;
+
+    this._lhsPayloads = {};
+    if (modelName !== inverseModelName || relationshipName !== inverseRelationshipName) {


Can you explain the goal behind this conditional

I've added some comments in the then clause to explain, as well as expanded the comments in the else clause.

// then clause // The common case of a non-reflexive relationship, or a reflexive // relationship whose inverse is not itself // else clause // Edge case when we have a reflexive relationship to itself // eg user hasMany friends inverse friends // // In this case there aren't really two sides to the relationship, but // we set `_rhsPayloads = _lhsPayloads` to make things easier to reason // about

igorT · 2017-03-13T21:36:08Z

addon/-private/system/relationships/relationships-payloads.js

+      this._isReflexive = true;
+    }
+
+    // either canoical on push or pending & flush


This comment doesn't really tell me very much

igorT · 2017-03-13T21:36:33Z

addon/-private/system/relationships/relationships-payloads.js

+  get(modelName, id, relationshipName) {
+    this._flushPending();
+
+    if (modelName === this._lhsModelName && relationshipName === this._lhsRelationshipName) {


Can you leave a comment explaining the logic behind mapping to lhs or rhs

igorT · 2017-03-13T21:37:36Z

addon/-private/system/relationships/relationships-payloads.js

+
+    if (modelName === this._lhsModelName && relationshipName === this._lhsRelationshipName) {
+      return this._lhsPayloads[id];
+    } else {


let's add a debug check that asserts this is actually === rhsRelationshipName

igorT · 2017-03-13T21:38:42Z

addon/-private/system/relationships/relationships-payloads.js

+  unload(modelName, id, relationshipName) {
+    this._flushPending();
+
+    if (modelName === this._lhsModelName && relationshipName === this._lhsRelationshipName) {


Maybe extract into a method?

igorT · 2017-03-13T21:40:56Z

addon/-private/system/relationships/relationships-payloads.js

+  _flushPending() {
+    if (this._pendingPayloads.length === 0) { return; }
+
+    let work = this._pendingPayloads.splice(0, this._pendingPayloads.length);


can we rename this to payloadsToBeProcessed or something like that

igorT · 2017-03-13T21:48:05Z

addon/-private/system/relationships/relationships-payloads.js

+        }
+      }
+
+      if (modelName === this._lhsModelName && relationshipName === this._lhsRelationshipName) {


Can you leave a comment explaining the intent behind this blob

igorT · 2017-03-13T21:53:22Z

addon/-private/system/relationships/relationships-payloads.js

+    }
+  }
+
+  _inverseLoaded(payload) {


Seems bad that the invariant from unloading where graphs are either unloaded together or not at all, is changed here

igorT · 2017-03-13T21:54:16Z

addon/-private/system/relationships/relationships-payloads.js

+    }
+  }
+
+  _populateInverse(relationshipData, inversePayload, inversePayloads, inverseIsMany) {


When we rename the classes, can we rename this arguments as well please

k i've renamed inversePayloads to inverseIdToPayloads here and elsewhere

igorT · 2017-03-14T22:58:38Z

addon/-private/system/relationships/relationship-payloads.js

+  }
+}
+
+function hasRecordForId(store, type, id) {


In case this sticks around can we verify this is needed.

igorT · 2017-03-14T23:03:39Z

addon/-private/system/relationships/state/belongs-to.js

@@ -36,6 +36,18 @@ export default class BelongsToRelationship extends Relationship {
    this.flushCanonicalLater();
  }

+  setInitialCanonicalRecord(record) {


Can we move this to a separate PR

moved to a separate commit within this PR in case we want to revert separately

igorT · 2017-03-14T23:05:44Z

addon/-private/system/relationships/state/create.js

+      relationship = relationships[key] = createRelationshipFor(internalModel, rel, internalModel.store);
+
+      if (relationshipPayload) {
+        this.internalModel.store._backburner.join(() => {


Check if this is needed

igorT · 2017-03-14T23:11:18Z

addon/-private/system/relationships/state/relationship.js

    }
    this.flushCanonicalLater();
    this.setHasData(true);
  }

+  setupInverseRelationship(internalModel, initial) {


We don't need initial here

igorT · 2017-03-14T23:13:47Z

addon/-private/system/relationships/state/relationship.js

+      let relationships = internalModel._relationships;
+      let relationshipExisted = relationships.has(this.inverseKey);
+      let relationship = relationships.get(this.inverseKey);
+      if (relationshipExisted || this.isPolymorphic) {


Can you leave comments explaining

The laziness

why we are not lazy in the polymorphic case

igorT · 2017-03-14T23:14:04Z

addon/-private/system/relationships/state/relationship.js

+    } else {
+      let relationships = internalModel._implicitRelationships;
+      let relationship = relationships[this.inverseKeyForImplicit];
+      if (!relationship) {


The semantics here have not changed at all:

data/addon/-private/system/relationships/state/relationship.js

Lines 160 to 165 in 2edaf16

} else {

if (!record._implicitRelationships[this.inverseKeyForImplicit]) {

record._implicitRelationships[this.inverseKeyForImplicit] = new Relationship(this.store, record, this.key, { options: {} });

}

record._implicitRelationships[this.inverseKeyForImplicit].addCanonicalRecord(this.record);

}

igorT · 2017-03-14T23:18:46Z

addon/-private/system/store.js

@@ -2403,14 +2412,15 @@ Store = Service.extend({
    heimdall.increment(_setupRelationships);
    let setupToken = heimdall.start('store._setupRelationships');
    let pushed = this._pushedInternalModels;
+    let modelNameToInverseMap = Object.create(null);


Lets see if there are perf benefits to caching this across runs

igorT · 2017-03-14T23:19:10Z

addon/-private/system/store.js

@@ -2403,14 +2412,15 @@ Store = Service.extend({
    heimdall.increment(_setupRelationships);
    let setupToken = heimdall.start('store._setupRelationships');
    let pushed = this._pushedInternalModels;
+    let modelNameToInverseMap = Object.create(null);


Can you leave a comment as to why we do this

igorT · 2017-03-14T23:20:55Z

addon/-private/system/store.js

@@ -2787,19 +2797,82 @@ function _commit(adapter, store, operation, snapshot) {
  }, label);
 }

-function setupRelationships(store, internalModel, data) {
+function inverseRelationshipInitialized(store, internalModel, data, key, modelNameToInverseMap) {


lets rename this to isInverseRelationshipInitialized

igorT · 2017-03-14T23:25:01Z

addon/-private/system/store.js

+    inverseMap = modelNameToInverseMap[internalModel.modelName] = get(internalModel.type, 'inverseMap');
+  }
+  let inverseData = inverseMap[key];
+  if (inverseData === undefined) {


lets rename this to something like inverseRelationshipMetadata/something not containing the word Data

igorT · 2017-03-14T23:57:08Z

addon/-private/system/store.js

-    let relationship = internalModel._relationships.get(key);
-    relationship.push(data.relationships[key]);
-  });
+  for (let key in data.relationships) {


lets try switching this back to eachRelationship

yep this seems good; also with the setInitial work the backburner.join run in relationships/store/create seems unnecessary

hjdivad · 2017-03-16T17:44:09Z

@igorT @stefanpenner this is ready to merge.

stefanpenner · 2017-03-16T19:50:44Z

igorT · 2017-03-18T04:38:26Z

addon/-private/system/store.js

+    if (relationshipRequiresNotification) {
+      let relationshipData = data.relationships[relationshipName];
+      relationships.get(relationshipName).push(relationshipData);
+    } else {


We were gonna remove this else right?

ah you're right, LMK push this fix

- add option for expanding relationships - make eager materialize materialize included as well

The goal here is to do as little work as we can with relationship payloads until the relationship is actually accessed. In an extreme case, where many relationship payloads are pushed, but no relationships are accessed, all we want to do is stash the payload.

We don't need to go through `computeChanges`, `flushCqnonical` &c. when initializing relationships. Treating initial payloads the same as updated payloads is costly.

rlivsey · 2017-04-25T16:37:54Z

Looks like this has broken for JSONAPI payloads which contain links for hasMany associations.

When accessing a relationship in a template I now get the following error, going to before this PR was merged works fine:

Assertion Failed: You modified "meeting.attendees" twice on <minutebase@model:meeting::ember1351:772c5e6e-aceb-11e6-ae79-770161ca087d> in a single render. It was rendered in "component:mb-pages/dashboard/meeting" and modified in "component:mb-pages/dashboard/meeting". This was unreliable and slow in Ember 1.x and is no longer supported. See https://github.com/emberjs/ember.js/issues/13948 for more details.
Error
    at assert (http://minutebase.dev:4202/assets/vendor.js:21770:13)
    at assert (http://minutebase.dev:4202/assets/vendor.js:33521:34)
    at Object.assertNotRendered (http://minutebase.dev:4202/assets/vendor.js:39296:11)
    at Object.propertyDidChange (http://minutebase.dev:4202/assets/vendor.js:37912:30)
    at Class.propertyDidChange (http://minutebase.dev:4202/assets/vendor.js:51064:19)
    at Class.notifyPropertyChange (http://minutebase.dev:4202/assets/vendor.js:51078:12)
    at InternalModel.notifyPropertyChange (http://minutebase.dev:4202/assets/vendor.js:140423:21)
    at ManyRelationship.updateLink (http://minutebase.dev:4202/assets/vendor.js:147451:19)
    at ManyRelationship.push (http://minutebase.dev:4202/assets/vendor.js:147529:16)
    at Relationships.get (http://minutebase.dev:4202/assets/vendor.js:146885:24)

Looks like updateLink is notifying property change and causing Glimmer to re-evaluate the relationship and so it explodes.

Tried to make a Twiddle, but the example Twiddle appears to be broken at the moment.

stefanpenner · 2017-04-25T18:04:06Z

@rlivsey can you open a new issue so this isn't lost? (also ideally a failing test suite will make this much more actionable)

rlivsey · 2017-04-25T19:21:36Z

@stefanpenner yup, was trying to make a repro first but will open an issue once I get the kids to bed!

hjdivad requested review from stefanpenner and igorT February 24, 2017 01:47

hjdivad force-pushed the pay-go-setup-relationships branch 2 times, most recently from ac26ee6 to c6a496e Compare March 2, 2017 22:05

stefanpenner reviewed Mar 2, 2017

View reviewed changes

hjdivad force-pushed the pay-go-setup-relationships branch 3 times, most recently from cb16735 to c53997e Compare March 10, 2017 00:10

stefanpenner suggested changes Mar 10, 2017

View reviewed changes

igorT reviewed Mar 13, 2017

View reviewed changes

stefanpenner approved these changes Mar 14, 2017

View reviewed changes

hjdivad force-pushed the pay-go-setup-relationships branch 2 times, most recently from e42e839 to dd55489 Compare March 14, 2017 20:08

igorT reviewed Mar 14, 2017

View reviewed changes

hjdivad force-pushed the pay-go-setup-relationships branch 3 times, most recently from bf5db27 to 97dd39d Compare March 16, 2017 17:34

hjdivad mentioned this pull request Mar 16, 2017

Rename record[s] -> internalModel[s] #4874

Merged

igorT reviewed Mar 18, 2017

View reviewed changes

hjdivad added 4 commits March 20, 2017 13:53

strip assert with --instrument

82dca9a

Update query perf tests

f0453ea

- add option for expanding relationships - make eager materialize materialize included as well

Reify relationships lazily

a38f408

The goal here is to do as little work as we can with relationship payloads until the relationship is actually accessed. In an extreme case, where many relationship payloads are pushed, but no relationships are accessed, all we want to do is stash the payload.

Do less work on initial relationship push

f8304b2

We don't need to go through `computeChanges`, `flushCqnonical` &c. when initializing relationships. Treating initial payloads the same as updated payloads is costly.

hjdivad force-pushed the pay-go-setup-relationships branch from 97dd39d to f8304b2 Compare March 20, 2017 20:53

hjdivad merged commit 552e1d6 into master Mar 20, 2017

hjdivad deleted the pay-go-setup-relationships branch March 20, 2017 22:51

rlivsey mentioned this pull request Apr 25, 2017

hasMany association with links causes backtracking re-render error #4942

Closed

buschtoens mentioned this pull request Aug 15, 2017

Saving a record immediately after unloading a record with the same id fails #5125

Closed

	} else {
	if (!record._implicitRelationships[this.inverseKeyForImplicit]) {
	record._implicitRelationships[this.inverseKeyForImplicit] = new Relationship(this.store, record, this.key, { options: {} });
	}
	record._implicitRelationships[this.inverseKeyForImplicit].addCanonicalRecord(this.record);
	}

Pay go setup relationships #4821

Pay go setup relationships #4821

Conversation

hjdivad commented Feb 24, 2017 • edited Loading

stefanpenner 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjdivad commented Mar 10, 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

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

hjdivad commented Feb 24, 2017 •

edited

Loading