-
Notifications
You must be signed in to change notification settings - Fork 7.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Memoizing functions with multiple arguments #2115
Comments
You can use the Dup of #680. |
There is any reason why |
@VictorQueiroz serializing isn't cheap or straight-forward so we punt on that with a |
@jdalton It's for a good reason then. Thank's for the quick reply |
@jdalton can you please provide an example of a simple resolver to do this? thanks |
@anton6 Sure thing! const example = _.memoize((...args) => args, (...args) => JSON.stringify(args))
example(1,3,2)
// => [1, 3, 2]
example(1,2,3)
// => [1, 2, 3] |
Is there a resolver that works out-of-the-box for sets of hashable arguments? That is, right now I can use a |
It not work with function argument. and sometimes, we just need |
@jdalton the original suggestion in this issue was to create a multi-dimensional cache for memoize, which is very different from serializing multiple arguments into a single key, and in many cases much more performant. To be clear: a multidimensional cache would be a Map of Map of Maps (one layer of nesting per argument). Would you consider a PR for a change like this? The resolver argument could be left in place for those who want to serialize, and the multidimensional cache would be used when the "resolver" argument is not provided. |
To add to the comments above. If you try implementing memoize on your own in vanilla js you'll understand why the may have decided to provide a Root cause
ExampleImagine you have a function that accepts three args
So your ConclusionFor this reason I guess it's up to us to define how we want hash the args, e.g. using Hope it makes sense. |
@giulioambrogi That's precisely the problem, though; it's quite difficult to come up with your own resolver in plain vanilla JS, even though the INTENT of |
I know this is an old PR, and I came here hoping there could be some such functionality, but really I am siding heavily with the maintainers in that this should not be implemented. Two reasons:
So, while I wish this was a feature, unless somebody could propose an intuitive interface and efficient implementation then I do not see how this could be a usable lodash feature. |
No one wants complex or multidimensional or recursive logic, as far as I know. I don't think anyone would argue against requiring a |
I think this feature request still makes a lot of sense. The multidimensional map mentioned in the description by @willpiers and in this comment by @ahfarmer would definitely work well for multi argument functions. I created here for me this crude implementation: export default function memoize(func) {
const cache = new Map();
const memoized = function (...args) {
let innerCache = cache;
for (let i = 0; i < func.length - 1; i++) {
const key = args[i];
if (!innerCache.has(key)) {
innerCache.set(key, new Map());
}
innerCache = innerCache.get(key);
}
const key = args[args.length - 1];
if (innerCache.has(key)) {
return innerCache.get(key);
}
const result = func(...args);
innerCache.set(key, result);
return result;
};
return memoized;
} This should work the same as A problem I noticed here is that when looping through the arguments to create the inner levels of maps I was unsure whether to use |
I've been using the suggestion from @shermam but i've modified it to support functions that can be passed a varying number of arguments like variadic functions export default function memoize(func) {
const cache = new Map();
const memoized = function (...args) {
let innerCache = cache;
// first layer of the map is the arguments length
// if two calls have different number of arguments
// then they cannot be the same call
if (!innerCache.has(args.length)) {
innerCache.set(args.length, new Map());
}
innerCache = innerCache.get(args.length);
// using args.length because func.length is 0 for variadic functions
for (let i = 0; i < args.length - 1; i++) {
const key = args[i];
if (!innerCache.has(key)) {
innerCache.set(key, new Map());
}
innerCache = innerCache.get(key);
}
const key = args[args.length - 1];
if (innerCache.has(key)) {
return innerCache.get(key);
}
const result = func(...args);
innerCache.set(key, result);
return result;
};
return memoized;
} Here's how I've been visualizing the data structure (cache). const fn = memoize((...args) => { });
fn(1, 2, 3)
fn(1, 2)
|
Memoizing based on all function arguments should definitely be the standard case. A (pure) function maps inputs to an output. You cannot memoize the output based on only a part of the inputs. This implies non-determinism! We just debugged our code for 3 days with 4 senior developers in our company and removing This (and some other strange behaviors with utility functions in lodash) discourage me to use this library any further. |
The memoize function should throw by default if supplied with multiple arg function and no resolver. This is trivial to implement and the utility as it is now, can only be categorized as a potential foot gun. |
Could this or some variant of this change be applied to make the utility safe? |
The docs mention that, "By default, the first argument provided to the memoized function is used as the map cache key"
It would be great if the cache were multi-dimensional so
_.memoize
worked well out of the box with longer functions. i.e. functions with more arguments.If there is a simple way to do this already, we can close. If this sounds useful I can look into submitting a PR
The text was updated successfully, but these errors were encountered: