Skip to content
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

Multiple inputs & results #12

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

TheLastGimbus
Copy link

Hi there!

I was looking for something like @functools.lru_cache from Python in Dart, and came across this library. But it's currently remebering only one result - kinda useless in my opinion 😕

So I'm proposing a solution to write down results with a HashMap

Currently I added only memo1, but if you like it, I will extend it to all the others - probably nested HashMap<A, HashMap<...>> will be used for that

I also added an example code to demonstrate how it makes things way faster 🚀

Closes #10

@TheLastGimbus
Copy link
Author

Hey @Aetet @brianegan, is this repo alive or something? I can make a fork/re-make of this myself...

@Aetet
Copy link
Contributor

Aetet commented Feb 21, 2021

Hey.
Currently I'm not maintaining this repo. Some guys from Wrike company should.

@DisDis @Chudesnov @bunopus

lib/memoize.dart Outdated
A prevArg;
R prevResult;
bool isInitial = true;
HashMap<A, R> argsToOutput = HashMap();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about limit storage capacity? For example second argument (with default value) to memo1.

Otherwise, in some cases, we will waste a lot of memory.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you mean to limit maximum cache? Yeah, that would be useful

Tho i'm not sure how to do this - if you just pop() one item from a HashMap in dart, does it do it in some order or just random?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so generally - you like/want my change in general sense? If so, I can implement rest of the features - just wasn't sure if I even should

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I like your changes:)

@missvalentinep
Copy link

missvalentinep commented Apr 7, 2021

@TheLastGimbus a gentle ping :)

@TheLastGimbus
Copy link
Author

TheLastGimbus commented Aug 22, 2021

HIiiiiii!

I know it took some time, but here it is! I updated branch with master, and implemented rest of methods!

I also fixed (I hope that's the right word 😅) test - they were designed that function re-executes every time something changes - please take a look at 84c2854

@sinegovsky-ivan I don't have any good idea for limiting the capacity tho 😕 It's a HashMap, so we can't just pop the first one (to make some space) because it's un-ordered... maybe we can store not only the value but also some N counter which by we would decide?

Func1<A, R> memo1<A, R>(Func1<A, R> func, {int? maxSize}) {
  final argsToOutput = HashMap<List, R>(
    equals: (a, b) => _listEquals(a, b),
    hashCode: (l) => _listHashCode(l),
  );
  // External list to keep track of cache order
  final hashes = <int>[];

  return ((A aA) {
    final cached = argsToOutput[[aA]];
    if (cached != null) {
      return cached;
    } else {
      final res = func(aA);
      argsToOutput[[aA]] = res;
      // Skip the whole thing if user doesn't use it
      if (maxSize != null) {
        hashes.add(_listHashCode([aA]));
        if (argsToOutput.length > maxSize) {
          // Pop the first (oldest) one
          // I'm not sure how efficient this is :/
          argsToOutput.removeWhere((k, _) => _listHashCode(k) == hashes[0]);
          hashes.removeAt(0);
        }
      }
      return res;
    }
  });
}

Something like this?

Edit: I actually wrote simple test for the above:

test('should remove old cache when maxSize set', () {
  var count = 0;
  var func = memo1((int a) => ++count, maxSize: 2);
  expect(func(1), 1);
  expect(func(1), 1);
  expect(func(2), 2);
  expect(func(2), 2);
  expect(func(1), 1);
  expect(func(3), 3);
  // At this point, first cache element should be removed
  expect(func(1), 4);
});

and it passes!

...oh, and also - I didn't know what to do with imemoN functions - they compare by identical, and I'm not sure will this work in HashMap - but I can try...

@missvalentinep
Copy link

@TheLastGimbus hey! I come to you with many thoughts about this PR:

  1. Creating a hash map for every memo is not very efficient (since we'll just increase memory usage for people who're already using these functions) and adding an "if" condition ("create hashmap only if maxSize was provided") would make the code hard to read. So I propose creating a separate set of functions (just like imemo), called memom1, memom2... (as in "memoMany") that will have this functionality you want to add.
  2. As for _listEquals and _listHashCode, I'd propose you use quiver package and listEqual and hashObjects methods from there
  3. For storing cache I believe you should use LinkedHashMap, as it remembers the order in which the keys were inserted. So it will allow you to do something like this:
if (argsToOutput.length == maxSize) {
  argsToOutput.remove(argsToOutput.keys.first);
}
  1. Since we're creating a separate set of functions, the maxSize param should be mandatory.
  2. [Optional] I'd personally rename argsToOutput to cachedResults/cache and maxSize to cacheLimit

@om-ha
Copy link

om-ha commented Oct 24, 2021

Great project and PR!

Thanks for your review @missvalentinep.

Thanks @TheLastGimbus for your PR, would love to see this merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memoize more than one result
5 participants