refactor: memoize some methods

[makepanic]

implements parts of #128

[makepanic]

I've added memoization to the association classes. It might be unsafe but our tests and mirage tests work fine with it.

The issue with these methods is that they don't get an argument.
Alternatively we could extract private methods which receive the instance fields as arguments to make them safe to memoize.

[makepanic]

I've also ran a slightly larger test, which uses more mirage instances to check how it compares:

• old mirage

1416ms
1290ms
1127ms
1185ms
1172ms
1143ms
1145ms

• memoized mirage

1053ms
969ms
938ms
909ms
973ms
969ms
918ms

Seems like a constant improvement.

[samselikoff]

Great work 👍 I'm willing to change these things and if they cause bugs it means we need better test coverage. We should be able to push changes to Mirage if the tests pass.

Stylistically when I've done memoization in the past I've done it like this

get identifier() {
  if (!this._identifier) {
    this._identifier = `${camelize(this._container.inflector.singularize(this.key))}Ids`;
  }
  
  return this._identifier
}

Just curious if that would achieve the same effect as lodash's memoize function, I've never used it. Don't worry about changing anything yet just wanted to bring this up.

[makepanic]

Just curious if that would achieve the same effect as lodash's memoize function, I've never used it. Don't worry about changing anything yet just wanted to bring this up.

It's probably better in regards to complexity.
Lodash is also checking arguments and comparing them. We can also manually cache the values but I thought it would make future maintenance more complex.

[chbonser]

I can confirm that I saw a large improvement with this change set. Models which were taking ~300ms to create are now taking ~100ms.

I will note that I did have to modify db-collection.js as follows as sometimes the id is null. I did not see this with the master branch.

@@ -344,7 +344,7 @@ class DbCollection {
     @hide
    */
   _findRecord(id) {
-    id = id.toString();
+    id = id ? id.toString() : id;

[makepanic]

I will note that I did have to modify db-collection.js as follows as sometimes the id is null. I did not see this with the master branch.

That's odd.
Maybe we should memoize ourself and not let lodash do it. There might be some unexpected lodash features that play into it.

@chbonser can you maybe post a stacktrace of the _findRecord that failed on you?

I think it might be becauses some of the methods are memoizing when using this. We should extract a method to pass the this arguments in. I'll update the PR later on.

[makepanic]

@chbonser I've refactored some of the association memoized methods. Now they're native getters that call the methods by passing an argument. Can you check if your adjustment is still needed?

@samselikoff I'm not sure if the latest commit is fine as it's potentially breaking api consumers due to multiple api changes.

[chbonser]

@makepanic That did the trick.

[samselikoff]

@makepanic Yeah I mean these methods have never been documented, there's no way every method in Mirage is public. I think they are not breaking changes but we should still clearly communicate the changes.

Great job on all this work, super appreciate of you diving into this!

So using a getter that calls a memoized function is faster eh?

[makepanic]

So using a getter that calls a memoized function is faster eh?

I don't think that there's a perf difference.
It's just that a pure function is easier to memoize if its arguments are passed in. Previously it was memoized while still reading instance values. Repeated calls would've returned the initial "cached" result.

The alternative would be to keep the getX() methods and introduce _getX(argument) to allow clean memoize usages.

[ryanto]

This looks awesome, thanks so much!

I left a question and a stylistic comment. Would love to get your feedback or changes on those before merging.

[ryanto]

🎉 🎉 🎉

Nice job @makepanic