[Feat] simplified relationship state #4882

runspired · 2017-03-20T19:39:56Z

Simplified Relationship State

Original Commits https://github.com/runspired/data/tree/original/simple-relationship-state this PR has since been flattened for rebase to continue tracking master.

Description

This is a backwards compatible revisiting of Ember-Data's relationship state objects with the intent of simplifying data flows, improving our ability to reason about the relationship layer, and further optimize relationship management in the future.

How to Review

Each commit in this PR is a discrete step along the path of this refactor, and (with I think one exception) left ember-data in a working state with tests passing. To follow the logic, follow the PRs.

Summary

Creates a new ImplicitRelationship class which absorbed most of the previous OrderedSet behavior of Relationship
Converts the Relationship class to largely acting as a shared interface that BelongsToRelationship HasManyRelationship and ImplicitRelationship must implement.
Simplifies the state management of BelongsToRelationship and HasManyRelationship, each now has a currentState and canonicalState property. For belongs-to these are InternalModels while for has-many these are arrays of InternalModels.
Moves management of currentState for the HasManyRelationship out of ManyArray and into the relationship class. This brings ManyArray much closer to being a simple proxy Array.
Cleans up some test code encountered during this process
Cleans up some use of record => internalModel
Uses UniqueArray to make a few performance improvements to places where we were blindly concat-ing arrays leading to duplicate work before.
More intelligently iterates over relationships in Store._setupRelationships

Motivations

Make relationship state simpler to manage and easier reason about.
Enable better optimizations within the relationship layer

Performance Characteristics

With this refactor we

Allocate fewer objects (especially far fewer OrderedSets which are only used now for ImplicitRelationships).
Produce many fewer closures when iterating
No longer move back and forth between OrderedSet and Array
use strict assignment and === for managing BelongsToRelationship state
no longer duplicate diffing work for each state change within each relationship class layer as well as within ManyArray
broadcast fewer duplicate or unnecessary state changes

These changes were compared against #master to validate performance changes we expect to see. The test used the localhost:4200/api/complexes?included=foo%2Cbaz&limit=34 endpoint which returns 34 primary records and 204 included records of 3 total model types.

Before

After

Results of This PR against LinkedIn Feed

krisselden · 2017-03-21T01:08:00Z

addon/-private/system/unique-array.js

+export default class UniqueArray {
+  constructor(key) {
+    this.key = key;
+    this.seen = Object.create(null);


consider not initializing seen until array is > say 20 items.

This seems awfully similar to Ember.OrderedSet We should just tweak that, rather then adding something similar. Note: Ember basically provides OrderedSet and friends only for ember-data.

I believe we can improve the performance of (small N) ordered set by taking the approach outlined above by @krisselden, it performs quite well for large N.

We should also expose list as a public attribute on it.

krisselden · 2017-03-21T01:12:27Z

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

+            array.push(relationship.currentState, relationship.canonicalState);
+            return;
+          case 'has-many':
+            additions = [].concat(relationship.currentState, relationship.canonicalState);


do 2 pushes instead of concat

I was debating this, any particular reason why?

krisselden · 2017-03-21T01:14:18Z

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

+  destroy() {
+    if (!this.inverseKey) { return; }
+
+    let allMembers =


can we not copy to an array just to iterate over?

Seeing as this doesn't unique the sets either it's doubly inefficient.

(also fwiw this is the legacy path that everything used to do, now only implicit does it, I haven't spent much time optimizing implicit's code yet)

krisselden · 2017-03-21T01:16:20Z

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

+
+  addInternalModels(internalModels, idx) {
+    heimdall.increment(addInternalModels);
+    internalModels.forEach(internalModel => {


if this is a hot path consider avoiding forEach

krisselden · 2017-03-21T01:17:41Z

addon/-private/system/snapshot.js


    if (hasData) {
      results = [];
-      members.forEach((member) => {
+      currentState.forEach((member) => {


if hot path, avoid forEach.

runspired · 2017-03-21T01:23:50Z

testem.js

@@ -8,7 +8,7 @@ module.exports = {
    "PhantomJS"
  ],
  "launch_in_dev": [
-    "PhantomJS",
+  //  "PhantomJS",


this crept in, should remove it.

pangratz · 2017-03-22T17:52:20Z

addon/-private/system/store.js

    }

-    pushed.length = 0;
+    this._pushedInternalModels = Object.create(null);


Do we need to reset _hasPushedInternalModels as well? As far as I can tell, otherwise the _setupRelationships is only invoked once in the first runloop and never afterwards?

Thanks for the catch! We do need to. I have a fix for this in a different branch where I was rebasing and will bring it back over to here.

igorT · 2017-03-24T19:59:24Z

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

@@ -18,4 +18,9 @@ export default class Relationship {
    this.hasLoaded = false;
    this.meta = null;
  }
+
+  destroy() {
+    this.currentState = null;


Check if this is still there

igorT · 2017-03-24T20:01:57Z

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


 export default class BelongsToRelationship extends Relationship {
+  constructor(store, internalModel, inverseKey, relationshipMeta) {
+    super(store, internalModel, inverseKey, relationshipMeta);
+    this.kind = 'belongs-to';


I think we camelCase this

igorT · 2017-03-24T20:06:47Z

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

-          newRecord._implicitRelationships[this.inverseKeyForImplicit] = new ImplicitRelationship(this.store, newRecord, this.key,  { options: {} });
-        }
-        newRecord._implicitRelationships[this.inverseKeyForImplicit].addRecord(this.record);
+    // TODO implicit-legacy @runspired is this needed?


Leftover comment

igorT · 2017-03-24T20:10:06Z

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

-      return record;
-    });
+    return this.store.findBelongsTo(this.internalModel, this.link, this.relationshipMeta)
+      .then((internalModel) => {


Can you keep existing indentation please

igorT · 2017-03-24T20:11:07Z

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

 export default class Relationship {
  constructor(store, internalModel, inverseKey, relationshipMeta) {
    let asyncOptionValue = relationshipMeta.options.async;

+    this.rel_id = REL_ID++;


Lets leave a comment as to why we are doing this

igorT · 2017-03-24T20:13:49Z

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

@@ -31,6 +31,28 @@ const {

 const assign = Ember.assign || Ember.merge;

+class UniqueArray {


Can you document this please, and maybe take it out in a separate file?

igorT · 2017-03-24T20:15:30Z

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

+    this.items = [];
+  }
+
+  push(...additions) {


Can we make this more like a util to make it clear it's not a stateful array

tchak · 2017-04-07T05:35:02Z

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

+      if (this.inverseKey) {
+        this.removeRecordFromInverse(internalModel);
+      } else {
+        if (internalModel._implicitRelationships[this.inverseKeyForImplicit]) {


} else if { ?

tchak · 2017-04-07T05:48:47Z

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

+
+      if (this.inverseKey) {
+        this.removeCanonicalRecordFromInverse(internalModel);
+      } else {


} else if {

stefanpenner · 2017-04-12T04:49:50Z

addon/-private/system/many-array.js

@@ -129,8 +127,7 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, {
    */
    this.relationship = this.relationship || null;

-    this.currentState = [];
-    this.flushCanonical(false);
+    // this.flushCanonical(false);


stefanpenner · 2017-04-12T05:08:04Z

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

-    this.canonicalState = null;
+    this.kind = 'belongs-to';
+  }
+  get inverseRecord() {


likely can be removed

stefanpenner · 2017-04-12T05:08:07Z

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

+    return this.currentState;
+  }
+
+  set inverseRecord(v) {


likely can be removed

stefanpenner · 2017-04-12T05:11:15Z

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

+      if (this.inverseKey) {
+        this.removeRecordFromInverse(internalModel);
+      } else {
+        if (internalModel._implicitRelationships[this.inverseKeyForImplicit]) {


stefanpenner · 2017-04-12T05:11:34Z

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

+
+      if (this.inverseKey) {
+        this.removeCanonicalRecordFromInverse(internalModel);
+      } else {


stefanpenner · 2017-04-12T05:21:30Z

addon/-private/system/relationships/state/has-many.js

+      return;
+    }
+
+    this.removeCanonicalRecordFromOwn(internalModel);


187's index can be reused here, allowing removeCanonicalRecordFromOwn to avoid the re-scan.

stefanpenner · 2017-04-12T05:28:48Z

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

+
+  addInternalModels(internalModels, idx) {
+    heimdall.increment(addInternalModels);
+    internalModels.forEach(internalModel => {


stefanpenner · 2017-04-12T05:29:16Z

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

+    heimdall.increment(newRelationship);
+
+    this.members = new OrderedSet();
+    this.canonicalMembers = new OrderedSet();


we should create this lazily?

stefanpenner · 2017-04-12T05:38:00Z

addon/-private/system/store.js

-    if (this._pushedInternalModels.push(internalModel, data) !== 2) {
-      return;
-    }
+    let hash = this._pushedInternalModels;


i believe this can be restored, but during _setupRelationships you should keep a cache of relationshipsByName._key.list by modelName around to avoid the lookups bellow.

stefanpenner · 2017-04-12T05:43:18Z

addon/-private/system/unique-array.js

+export default class UniqueArray {
+  constructor(key) {
+    this.key = key;
+    this.seen = Object.create(null);


This seems awfully similar to Ember.OrderedSet We should just tweak that, rather then adding something similar. Note: Ember basically provides OrderedSet and friends only for ember-data.

I believe we can improve the performance of (small N) ordered set by taking the approach outlined above by @krisselden, it performs quite well for large N.

We should also expose list as a public attribute on it.

runspired · 2017-04-20T18:07:47Z

hrm, that wasn't failing locally. Might have missed something in the commit.

stefanpenner · 2017-04-24T21:51:17Z

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

@@ -429,17 +429,30 @@ export default class InternalModel {
    to or has many.
  */
  _directlyRelatedInternalModels() {
-    let array = [];
-    this.type.eachRelationship((key, relationship) => {
+    let uniqueSet = new OrderedSet();


all sets are unique, lets just name this variable set

the name is more so signify the intentional purpose of the set (to generate a unique set), can change if still desired.

best to change it. uniqueSet is confusing (like hashMapWithKeysAndValues)

stefanpenner · 2017-04-24T21:52:37Z

addon/-private/system/ordered-set.js

+
+  return this.size;
+};
+
 OrderedSet.prototype.addWithIndex = function(obj, idx) {
  let guid = guidFor(obj);
  let presenceSet = this.presenceSet;


This is unfortunately private API that will change. Ember-data shouldn't be reaching into private state.. hmmm

I think the only thing is to add this to ember's, and have ember's lazily create presenceSet for old versions of ED.

This part is pre-existing, but I agree. I feel we really just need to build an OrderedSet with good perf and tailored to ember-data, given that it exists inside of Ember mostly for ember-data to begin with.

extend implicit from relationship base relationships off of implicit remove dependency on members from belongs-to relationship remove dependence on members from has-many remove belongs-to dependency on super into implicit decouple belongsTo optimize _directlyRelatedInternalModels remove duplication from Relationship base class cleanup currentState entanglement and begin prepping has-many for decoupling from implicit as a base class test cleanup decouple has-many from implicit remove unneeded code paths from implicit relationships fix(has-many): ensure ManyArray is instantiated lazily optimize setupRelationships clean up has-many noise

…at will need some benchmarking perhaps

…tionships that builds a cache as it goes instead of upfront

stefanpenner · 2017-04-25T18:52:48Z

rebasing

stefanpenner · 2017-04-26T00:26:30Z

addon/-private/system/store.js

-      // This will convert relationships specified as IDs into DS.Model instances
-      // (possibly unloaded) and also create the data structures used to track
-      // relationships.
+    for (let i = 0; i < pushed.length; i += 2) {


this produces quite sketchy ES5 output, lots of nested functions to handle the let TDZ

_setupRelationships: function _setupRelationships() { var pushed = this._pushedInternalModels; var store = this; // Cache the inverse maps for each modelClass that we visit during this // payload push. In the common case where we are pushing many more // instances than types we want to minimize the cost of looking up the // inverse map and the overhead of Ember.get adds up. var modelNameToInverseMap = Object.create(null); var modelNameToRelationshipsMap = Object.create(null); var _loop2 = function _loop2(i) { var internalModel = pushed[i]; var data = pushed[i + 1]; var modelName = internalModel.modelName; var relationshipsByName = modelNameToRelationshipsMap[modelName]; if (relationshipsByName === undefined) { relationshipsByName = modelNameToRelationshipsMap[modelName] = get$3(internalModel.modelClass, 'relationshipsByName'); } var relationshipNames = relationshipsByName._keys.list; var relationships = internalModel._relationships; var _loop3 = function _loop3(keyIndex) { var relationshipName = relationshipNames[keyIndex]; var relationshipData = data.relationships[relationshipName]; if (relationshipData !== undefined) { var relationshipRequiresNotification = relationships.has(relationshipName) || isInverseRelationshipInitialized(store, internalModel, data, relationshipName, modelNameToInverseMap); if (relationshipRequiresNotification) { relationships.get(relationshipName).push(relationshipData); } // in debug, assert payload validity eagerly } }; for (var keyIndex = 0; keyIndex < relationshipNames.length; keyIndex++) { _loop3(keyIndex); } }; for (var i = 0; i < pushed.length; i += 2) { _loop2(i); } pushed.length = 0; }

stefanpenner · 2017-04-26T06:14:28Z

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

@@ -324,7 +324,7 @@ export default class RelationshipPayloads {
      return;
    }

-    if (Array.isArray(data)) {
+    if (data.length) { // dirty-check for "is-array"


whats wrong with Array.isArray ?

I'd recommend to stick to Array.isArray, which gives you predictable performance, unless you now for sure that data is always an array.

stefanpenner · 2017-04-26T06:15:23Z

addon/-private/system/ordered-set.js

@@ -16,6 +16,18 @@ OrderedSet.prototype = Object.create(EmberOrderedSet.prototype);
 OrderedSet.prototype.constructor = OrderedSet;
 OrderedSet.prototype._super$constructor = EmberOrderedSet;

+OrderedSet.prototype.pushMany = function pushMany(additions) {


stefanpenner · 2017-04-26T06:16:19Z

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

+    this.currentState = this.canonicalState = internalModel;
+    this.setupInverseRelationship(internalModel, true);
+
+    // this.flushCanonicalLater();


should these be removed?

stefanpenner · 2017-04-26T06:22:24Z

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

+    }
+
+    //TODO(Igor) make this less abysmally slow
+    this._currentState = this.canonicalState.copy();


should we use the currentState setter?

it doesn't matter here in theory but I did mean to change this to use it once I added the setter.

bmeurer · 2017-04-26T07:27:02Z

addon/-private/system/store.js

+          }
+
+          // in debug, assert payload validity eagerly
+          runInDebug(() => {


If you'd strip the runInDebug before running Babel on it, then Babel might be able to avoid the closures.

runspired · 2018-03-16T20:45:38Z

We should still do this, but with #5344 coming and other work including many cleanup commits and lazy-relationships landing over the past year, this has diverged too far from Master. A clean reboot will come soon :)

krisselden reviewed Mar 21, 2017

View reviewed changes

runspired commented Mar 21, 2017

View reviewed changes

pangratz reviewed Mar 22, 2017

View reviewed changes

igorT reviewed Mar 24, 2017

View reviewed changes

runspired mentioned this pull request Apr 5, 2017

Revert "Ember.warn when ManyArray.objectAt() is underfined" #4914

Merged

tchak reviewed Apr 8, 2017

View reviewed changes

stefanpenner suggested changes Apr 12, 2017

View reviewed changes

runspired force-pushed the feat/simple-relationship-state branch 2 times, most recently from eec7047 to d9eb9f1 Compare April 20, 2017 16:21

runspired mentioned this pull request Apr 22, 2017

[SUPER WIP] Relationship State Overhaul #4726

Closed

6 tasks

stefanpenner suggested changes Apr 24, 2017

View reviewed changes

runspired added 7 commits April 25, 2017 11:52

base UniqueArray over OrderedSet

ecd7bf0

addressess all feedback sans rejiggering of setupRelationships, as th…

de43562

…at will need some benchmarking perhaps

pr cleanup

5ce660f

BENCH this: experiment with returning to a flat array for pushed rela…

67ef45a

…tionships that builds a cache as it goes instead of upfront

remove UniqueArray in favor of OrderedSet

4d0ff66

fix

ba182ca

update lock

5abad65

stefanpenner force-pushed the feat/simple-relationship-state branch from fda4dca to 5abad65 Compare April 25, 2017 18:54

rename uniqueSet -> set (all sets are unique)

0cb52d9

stefanpenner suggested changes Apr 26, 2017

View reviewed changes

bmeurer reviewed Apr 26, 2017

View reviewed changes

runspired closed this Mar 16, 2018

runspired added the Relationship Refactor label Oct 8, 2019

runspired added this to the Public Graph API milestone Sep 11, 2023

runspired removed the Relationship Refactor label Sep 11, 2023

		@@ -31,6 +31,28 @@ const {

		const assign = Ember.assign \|\| Ember.merge;

		class UniqueArray {

[Feat] simplified relationship state #4882

[Feat] simplified relationship state #4882

Conversation

runspired commented Mar 20, 2017 • edited Loading

Simplified Relationship State

Description

How to Review

Summary

Motivations

Performance Characteristics

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

pangratz Mar 22, 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

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

runspired commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanpenner Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanpenner commented Apr 25, 2017

stefanpenner Apr 26, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runspired commented Mar 16, 2018

runspired commented Mar 20, 2017 •

edited

Loading

pangratz Mar 22, 2017 •

edited

Loading

stefanpenner Apr 24, 2017 •

edited

Loading

stefanpenner Apr 26, 2017 •

edited

Loading