memo icon indicating copy to clipboard operation
memo copied to clipboard

implemented compiletime {.memoized.} compat, at expense of resetCache

Open JorySchossau opened this issue 6 years ago • 3 comments

Addresses #3 . This is a proposal, but hopefully someone has a better idea. This PR moves the cache declaration into the memoization wrapper proc as a {.global.}. The reason is that doing this allows the {.memoized.} macro to declare and use the cache during compileTime or runtime, whereas previously only runtime worked. The consequence of this is that the resetCache proc does not have access to the cache anymore. This is an unfortunate trade-off. Ideally, a completely different execution path would be selected depending on how the user is invoking the memoized proc (compileTime vs runtime). Thoughts?

JorySchossau avatar Mar 17 '19 20:03 JorySchossau

Thank you for your contribution! I don't think the tradeoff here is worth, since without resetCache, the cache would grow unbounded at runtime, and that may be the cause of memory leaks. For instance, one may want to use a cache to avoid the exponential recursive expansion, but then reset the cache between different calls.

It is good that you have found a solution, so at least you can install your branch of memo for your talk. I think the best way to proceed would be to have two different declarations of memoized, like this:

when nimvm:
  proc memoized # here we use a global variable
else:
  proc memoized # here we use the existing implementation

I didn't try that, it may work or it may cause issues since macros run in the vm anyway

andreaferretti avatar Mar 18 '19 10:03 andreaferretti

I see why the resetCache is quite necessary. Good idea to try the nimvm redefinition. I tried it but both definitions are during compiletime so they conflict. Rats!

JorySchossau avatar Mar 18 '19 23:03 JorySchossau

Well, then there is the easy solution: just define two different procs memoize and memoizevm. Then we can document when to use each one

andreaferretti avatar Mar 19 '19 08:03 andreaferretti