Repository navigation
Add memoization by instance - #185
Conversation
There was a problem hiding this comment.
I don't know how to get the model's idAttribute, but this should reference whatever that is instead of just ".id".
I also don't know if it's safe to peer into the query result in this particular way?
There was a problem hiding this comment.
Currently this memoizer still just behaves the same way as it did before, only it examines the keys of accessedModels since it's not an Array anymore. This memoizer ignores the stored list of accessed instances.
The shape of the invalidator map needs updating to accommodate instances, and the actual comparison function needs to compare at the instance level instead of at the table level. I'm not sure what that would look like.
There was a problem hiding this comment.
hey, do you still need some help on this one, i can dig into it and propose some directions if you'd like.
There was a problem hiding this comment.
Yes! Let me know what you find <3
There was a problem hiding this comment.
something like this?
export function eqCheck(a, b) {
return a === b;
}
function instancesEqual(invalidatorMap, state) {
return values(invalidatorMap).every(instanceEqualityChecker => instanceEqualityChecker(state));
}
export function memoize(func, equalityCheck = eqCheck, orm) {
let lastOrmState = null;
let lastResult = null;
let lastArgs = null;
const instanceEqualityCheckerByModelName = {};
return (...args) => {
const [dbState, ...otherArgs] = args;
const argsAreEqual = lastArgs && otherArgs.every(
(value, index) => equalityCheck(value, lastArgs[index]),
);
if (!argsAreEqual) {
const dbIsEqual = lastOrmState === dbState ||
instancesEqual(instanceEqualityCheckerByModelName, dbState);
if (!dbIsEqual) {
const session = orm.session(dbState);
const newArgs = [session, ...otherArgs];
const result = func(...newArgs);
// If a selector has control flow branching, different
// input arguments might result in a different set of
// accessed models. On each run, we check if any new
// models are accessed and add their invalidator functions.
Object.keys(session.accessedModels).forEach((modelName) => {
if (!instanceEqualityCheckerByModelName.hasOwnProperty(modelName)) {
instanceEqualityCheckerByModelName[modelName] = nextState => {
if (lastOrmState === null) {
return false;
}
const lastModelState = lastOrmState[modelName];
const nextModelState = nextState[modelName];
if (lastModelState !== nextModelState) {
return session.
accessedModels[modelName].
every(id => lastModelState.itemsById[id] === nextModelState.itemsById[id]);
} else {
return true;
}
};
}
});
lastResult = result;
lastOrmState = dbState;
lastArgs = otherArgs;
}
}
return lastResult;
};
}
There was a problem hiding this comment.
If we save the model data reference instead of true to session.accessedModels, can't we just make a === comparison against the new state?
There was a problem hiding this comment.
Ah, I see! 👍 But I would argue that those are still early function returns if you only look at the returned function from memoize.
What do you think about checking the equality for all args not only otherArgs, that way we would respect changes to the ormStates as well?
return (...args) => {
const [nextOrmState, ...otherArgs] = args;
if (argsAreEqual(lastArgs, args)) {
return lastResult;
}
const session = orm.session(nextOrmState);
if (accessedModelInstancesAreEqual(session.accessedModels, nextOrmState)) {
return lastResult;
}
lastResult = func(...[session, ...otherArgs]);
lastOrmState = nextOrmState;
lastArgs = args;
return lastResult;
};There was a problem hiding this comment.
I just realized, that with instanceEqualityCheckerByModelName[modelName] = invalidatorFunction we basically introduced a global accessedModel store. Before that was kind of implicit since we did only memoize the tables.
import every from 'lodash/every';
import merge from 'lodash/merge';
function defaultEqualityCheck(a, b) { a === b };
function const argsAreEqual(a, b) { b.every((value, index) => equalityCheck(value, a[index])) }
function accessedModelInstancesAreEqual(accessedModels, lastOrmState, nextOrmState) {
every(accessedModels, (accessedModelsForModelName, modelName) => {
const nextModelState = nextOrmState[modelName];
if (lastOrmState === null) {
return false;
}
const lastModelState = lastOrmState[modelName];
if (lastModelState === nextModelState) {
return true;
}
return
Object
.keys(accessedModelsForModelName)
.every(id => lastModelState.itemsById(id) === nextModelState.itemsById[id]);
});
}
export function memoize(func, equalityCheck = defaultEqualityCheck, orm) {
let lastOrmState = null;
let lastArgs = null;
let lastResult = null;
let allAccessedModels = {};
return (...args) => {
const [nextOrmState, ...otherArgs] = args;
if (lastArgs && argsAreEqual(lastArgs, args)) {
return lastResult;
}
if (accessedModelInstancesAreEqual(allAccessedModels, lastOrmState, nextOrmState)) {
return lastResult;
}
const session = orm.session(nextOrmState);
lastOrmState = nextOrmState;
lastArgs = args;
lastResult = func(...[session, ...otherArgs]);
allAccessedModels = allAccessedModels.merge(session.accessedModels);
return lastResult;
};
}Though we could persist that global accessedModel state in the memoize function I would rather persist it within the session itself? This makes the comment on the PR about the higher order component reseting accessedModels after render completion viable again..
@B-Reif is it ok if I hop into this with you (adding commits). It get's kind of hard keeping track of all the "meta" thoughts on on those inline comments... :P
There was a problem hiding this comment.
What do you think about checking the equality for all args not only otherArgs, that way we would respect changes to the ormStates as well?
👍
looks cleaner
There was a problem hiding this comment.
@tiii I added you to collab on my fork. Anyone else who'd like to commit there, feel free to reach out to me :>
There was a problem hiding this comment.
@B-Reif I'd love to be able to commit on your fork 🙂
@tiii Caching all arguments breaks the redux integration test at line 57 because db.getEmptyState() creates a new object every time. No idea if this change in API is welcome or not, but I think the memoization should still work even when the root reference changes. We already compare accessed model instances, this should be enough.
lastModelState.itemsById(id) should probably be lastModelState.itemsById[id].
allAccessedModels.merge(session.accessedModels); throws an error, this method does not exist (you may have intended something like merge(allAccessedModels, session.accessedModels);). The object spread syntax may actually be enough here, as we do not actually want to merge any IDs, but we want to replace the map of accessed IDs for each model - during each call we only want to check if any of the instances that were accessed during the previous function call have changed. The previous map can thus be removed without consequences.
Further, I also very strongly favor early returns. We cannot split up the initial check into two if statements though, as this turns the boolean AND into a boolean OR. What do you think of this version? Also added some comments for clarity. Please correct me if I misunderstood something 😀
const defaultEqualityCheck = (a, b) => a === b;
const argsAreEqual = (lastArgs, nextArgs, equalityCheck) => (
nextArgs.every((arg, index) =>
equalityCheck(arg, lastArgs[index])
)
);
const accessedModelInstancesAreEqual = (accessedModelInstances, lastOrmState, nextOrmState) => (
every(accessedModelInstances, (accessedInstances, modelName) => {
const nextModelState = nextOrmState[modelName];
const lastModelState = lastOrmState[modelName];
if (lastModelState === nextModelState) {
return true;
}
const accessedIds = Object.keys(accessedInstances);
return accessedIds.every(id =>
lastModelState.itemsById[id] ===
nextModelState.itemsById[id]
);
})
);
export function memoize(func, argEqualityCheck = defaultEqualityCheck, orm) {
/* cached result of previous function call */
let lastResult = null;
/* cached arguments to previous function call (excluding ORM state) */
let lastArgs = null;
/* cached previous ORM state for the sake of comparing */
let lastOrmState = null;
/**
* cached map of which model instances have been accessed
* during previous function call (contains only IDs of accessed instances)
* format (e.g.): { Book: { 1: true, 3: true } }
*/
let lastAccessedModelInstances = {};
return (...args) => {
const [nextOrmState, ...otherArgs] = args;
if (lastArgs &&
lastOrmState &&
argsAreEqual(lastArgs, otherArgs, argEqualityCheck) &&
accessedModelInstancesAreEqual(lastAccessedModelInstances, lastOrmState, nextOrmState)) {
/**
* the instances that were accessed as well as
* the arguments that were passed to func the previous time that
* func was called have not changed
*/
return lastResult;
}
/* previous result is no longer valid, update cached values */
const session = orm.session(nextOrmState);
lastOrmState = nextOrmState;
lastArgs = otherArgs;
lastResult = func(...[session, ...otherArgs]);
lastAccessedModelInstances = {
...lastAccessedModelInstances,
...session.accessedModels,
};
return lastResult;
};
}
There was a problem hiding this comment.
This reduce strikes me as a little clunky; I mostly wanted to retain a similar structure to the function as it used to be, but we could probably just examine the getDataForModel results instead of combing through this.sessionBoundModels.
There was a problem hiding this comment.
It looks like this was actually broken? markAccessed expects a modelName but this was passing in the whole model.
|
Would love to see this improvement ❤️ |
There was a problem hiding this comment.
food for thought:
- Make modelIds optional. If not provided, grab all the IDs from the model and use those.
- This would make this change backwards compatible in case people called markAccessed from within their apps
There was a problem hiding this comment.
what's the best way to grab all of the ids from the model? this is my first guess:
idsFromModelName(modelName) {
const model = this.sessionBoundModels
.filter(model => model.modelName === modelName);
return model.all().toRefArray().map(r => r.id);
}There was a problem hiding this comment.
Well they also get assigned to this (see here), so you could do:
idsFromModelName(modelName) {
const modelClass = this[modelName];
return modelClass.all().toRefArray().map(r => r[modelClass.idAttribute]);
}i think...
There was a problem hiding this comment.
Actually I don't think this would even work in this context since toRefArray starts a query itself, which would in turn run markAccessed itself.
There was a problem hiding this comment.
oh, interesting, didn't know that about toRefArray...
There was a problem hiding this comment.
My recommendation would be to start this off as a hash, not an array. Could be just a hash of IDs => true/false.
Making an array is probably better for memory, but to @plandem's point, if we have 10k+ instances in a model, imagine that the @connect function is doing a state compare on those 10000 objects, it has to literally loop through each component, and run a search for the instance IDs in the array of 10000 ids and then has to go through the other 10000 components on the page and do the same thing. This will be slow.
You might say, well we won't have 10000 ids in the array because it is only for changed objects...well that is true unless you want to invalidate the whole model, and you add every single ID into the accessed array.
For this reason, I say, go with the object lookup approach first. The users of your website will thank you... :)
|
Just pushed a commit with a few of the fixes from above. Out of curiosity about the performance impact, I ran Before changes: 11.912s, 10.555s, and 10.362s (avg 10.94s) EDIT: The CI build failed on some many-to-many relations taking too long :( from the failing build: @plandem What's your take on this level of performance? Any ideas? |
|
Over application lifetime, we keep adding up models to our session.accessedModels object and therefore incrementally checking models that did not change for changes over time. I was thinking if, for redux, we could flush the session.accessedModels after each application render, since our potential changes are then rendered to the dom and only changes to models that occured after the last render need to be checked for equality again. By providing an API to reset session.accessedModels and a higher order component that's placed early in the react component tree that calls that reset function in componentDidUpdate() we could keep checking the number of accessedModels that need to be checked for equality to a minimum. What are your thoughts on this? Am I missing a situation, where this may lead up to missing changes? Otherwise this sure would be a big performance improvement over app lifetime. |
There was a problem hiding this comment.
I guess we should also test if models are marked as accessed if they are resolved by the query function?
There was a problem hiding this comment.
Ah, I see! 👍 But I would argue that those are still early function returns if you only look at the returned function from memoize.
What do you think about checking the equality for all args not only otherArgs, that way we would respect changes to the ormStates as well?
return (...args) => {
const [nextOrmState, ...otherArgs] = args;
if (argsAreEqual(lastArgs, args)) {
return lastResult;
}
const session = orm.session(nextOrmState);
if (accessedModelInstancesAreEqual(session.accessedModels, nextOrmState)) {
return lastResult;
}
lastResult = func(...[session, ...otherArgs]);
lastOrmState = nextOrmState;
lastArgs = args;
return lastResult;
};
|
Ok, now I see, that i completely misunderstood the problem here.. Just ignore my last comment :) |
|
Turns out #185 (comment) might be relevant after all... |
|
a simple question: |
|
Currently the comparison happens in the memoize function within redux-orm. Right now, this function just checks if the previous state's table is Reference checking each instance is substantially more performant than deep-comparing all of the properties for each instance (as shouldComponentUpdate would be doing), along with the other benefits of memoization like caching computed data. |
|
I made some changes to implement the For example, some selector that looks for We could just add a check like I also feel like we should add a |
There was a problem hiding this comment.
Since our accessedModels are nested by table, doesn't this change overwrite previously accessed models for the same table?
There was a problem hiding this comment.
Yeah, this should be merged more deeply than this.
|
I just refactored the existing code along with @Aryk's proposal, so all props go to him 👍 I feel like the model.items equality check is the way to go. If the selector only uses withId it should run pretty fast, and in the end - since nothing changed - the references are still the same and react shouldn't rerender or at least not rerender to the DOM (or it's equivalent). I guess we could also store the accessedModels per selector function and check if only withId is called, but that seems utterly complex and pretty much overkill. |
|
I just realized we already memoize per selector. So we could also track the used query methods to find out if only withId is used. I hope I find time to provide some test cases / documentation soon.. |
|
This implementation is still broken for ‘filter’ and ‘exclude’ queries, since they might hit new instances on each change. EDIT: Additionally, a 'withId' query is actually just a shorthand for creating a 'filter' query. A basic 'withId' lookup might look like: |
|
To be honest, I'm not sure if there's a good strategy to apply across all possible query types. Some query types, like I think what I'd like to do is:
These changes would allow developers to take responsibility of memoization, and give developers the information they need to make meaningful memoization functions. But it would also leave the weak table-level default for most developers. And storing the instance ids in the map leaves queries (slightly) slower for everyone. These changes would also retain most backward compatibility, excepting The alternative would be to come up with a default memoization strategy that attempts to cover all possible use cases. This strategy would have to track which queries were used and their arguments to divine what level of the state it should be comparing at. |
|
@B-Reif when you said "This implementation is still broken for ‘filter’ and ‘exclude’ queries, since they might hit new instances on each change." Are you saying that potentially new records could be added to the tables and hence the memoized result would not pick up the new records? Wasn't that handled before by just adding the model to accessed Models? Why don't we keep the old functionality in this way alongside the new per-id model. Maybe we could add in the map something like When a filter or exclude happens you could add a flag to check the IDs of the table. You could use an efficient "===" on just the IDs ( |
|
That’s not the problem; the problem is that existing instances could change such that they now pass the filter, when they previously did not. For example, suppose we have a filter query that looks for Book models with a name starting with “P”. The first run, it caches the applicable model instances. Then I add a “P” to some other instance. Our filter query then doesn’t re-run, because the change was outside the cached instances. |
|
Right, that is why I am proposing anytime you hit something (filter/exclude) that relies on "bypassed" or "skipped" records, that you put {Book: true} into the accessedModels. That is basically like old functionality. The difference is that now, because we have instance level caching, when you render an individual book with withId on a connected component, you won't actually have to rerender that entire component just because another instance got updated/add/deleted, whereas even if you have a connected component on one instance with Book ID "5", it will rerender when Book Id "6" gets updated, as it is currently today. Many times in applications, it is possible the developer might keep a list of IDs to display in a list which is not dependent on attributes from the instances themselves, so you wouldn't be using filter/excude/etc. In those cases (which are many), the list of components tied to the individual redux-orm instances would not get updated and the list itself would be displayed and rerendered very optimally which is why i feel we need to include this functionality by default. If you touch exclude/filter and maybe some other commands we would just do an entire table validation. I hope we are talking about the same stuff. :) |
|
Just curious, have there been any updates on this feature? Any ETA on when it will get into the main branch? |
|
I've stopped working on this as I no longer have time to pursue it. We can close this PR or have other people take up work on it. My solution on my app's side: I use |
|
@Aryk Created a fork and re-added the functionality for marking full tables as accessed (called it To be sure everything works, I added a few Redux integration tests. The only case where the selector still doesn't get re-executed (i.e. the cache is not invalidated) is when an update is being performed on a Model. For some reason the updated Model instance does not get a new reference - and the entire What I am still unsure about is whether or not |
|
So it turns out this feature collides with batched mutations. The only immutability currently ensured is the idea that once any table changes in any way, the This token being the same among all these calls causes immutable-ops to actually mutate the passed rows and tables in order to speed up the operations. Now, to my surprise I couldn't find any place where the library still uses immutable operations to change the state. What does this mean for instance-based selectors?
|
…hips on Travis CI
ignore order of clauses (EXCLUDE and ORDER_BY are always executed after id-based filters)
…vis CI due to high volatility
…m2m relationships
c073080 to
349e256
Compare
This reverts commit 349e256.
This PR refines redux-orm's memoization behavior to memoize by model instances rather than by the whole table. This is intended as a fix for #184 and related to discussion in #71.
This PR is INCOMPLETE don't merge it yet 😅