Factory cache #4704
Factory cache #4704
Conversation
|
|
||
| clear() { | ||
| if (this._records) { | ||
| let records = this._records.slice(); |
There was a problem hiding this comment.
we don't appear to purge this._records membership, should we maybe instead do.
let records = this._records;
this._records = []; // or maybe nullThat way, we release anything currently retained by this._records and don't need to copy the this._records array via slice
There was a problem hiding this comment.
I'll look into it. This is the same logic preserved from before, and it's unclear if emptying this has ramifications, I think it does (I think individual records are currently responsible for removing themselves). This is a data flow story I will untangle soon in order to also help us unlock better change tracking for record arrays.
There was a problem hiding this comment.
Our tests still pass with this in place, so I've made this change.
There was a problem hiding this comment.
@runspired sounds good, making this change should prevent a potential mem leak.
| return null; | ||
| } | ||
| return this.store._modelFor(this.modelName); | ||
| }), |
|
|
||
| export default class IdentityMap { | ||
| constructor() { | ||
| this._map = new EmptyObject(); |
There was a problem hiding this comment.
Object.create(null) may be fine, here. EmptyObject was more of a trick for mega mega hotspots to work-around a strange perf issue in older v8's.
In this case, I believe Object.create(null) will be best. (less caveats etc)
There was a problem hiding this comment.
I switched to Object.create(null) but we should probably investigate which is currently better.
|
I'm 👍 with this direction, although the title of the PR is a-bit misleading, after reading the PR i feel much better. We should be careful re: compat, but this change is worth doing! |
| /* | ||
| @private | ||
| */ | ||
| _modelFor(modelName) { |
There was a problem hiding this comment.
I'd like to see this remain a public API. modelFactoryFor should probably be removed.
There was a problem hiding this comment.
It remains public, the differentiator is that "public" APIs must always normalize the modelName while "private" APIs can assume a normalized modelName. We currently call this so often internally I wanted to avoid that unnecessary step.
There was a problem hiding this comment.
I also agree that modelFactoryFor should probably be removed.
There was a problem hiding this comment.
FTR, the reason we can't remove modelFactoryFor at the moment is because adapters use it to do a modelFor lookup without risk of erroring when a model isn't found when an unknown type is encountered in a payload (prints a warning, doesn't error).
|
@stefanpenner I believe this is also the reason for the confusion around the name. This PR creates a cache for the |
👍
Yup, in the end the name seemed fine. The root cause of the confusion, was just words a words. And prior efforts that where funky used this name, but the current one feels good.
Ya, even very cheap things happening many many times ads up. |
runspired
force-pushed
the
factory-cache
branch
2 times, most recently
from
004a8b0 to
a73dd3c
Compare
|
Overall this pr looks pretty good. Once #4664 lands I think this one should be ready to go. |
runspired
force-pushed
the
factory-cache
branch
from
a73dd3c to
29d6f14
Compare
Needs benchmarking, review, may have been too aggressive in
_modelForuse, dislike thatmodelFactoryForis public, should discuss.Also, one reason for WIP is that this builds off of #4664, may be easier to review once that lands and I rebase ;)