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

Added a simple FIFO cache to defaultMemoize, with a cacheSize option #238

Closed
wants to merge 1 commit into from

Conversation

ndbroadbent
Copy link

@ndbroadbent ndbroadbent commented Mar 29, 2017

I was using memoize from lodash to create some unbounded caches, which is fine for certain parts of my code. I also know about re-reselect, but that solves a slightly different set of problems.

There are a few cases in my app where I really, really want to have a specific cache size, so that it would store the last 2 or 3 results. One example is where I am using selectors for both old and new props, and comparing the results. Another example is where I have a boolean prop that frequently switches between true and false.

So I've added a cacheSize option to defaultMemoize. If cacheSize is one, then we fall back to the original defaultMemoize code (renamed to memoizeOne.)

If cacheSize is more than one, then we store the args and results in an array. We start appending results to the end of the array and move backwards, until it wraps around to the end and starts overwriting the oldest result. It uses a lastIndex pointer to keep track of the most recent results, so the search always runs from newest to oldest.

I added benchmark tests, and they look fine on my machine. Here's the output after I added a temporary for loop to run them each 5 times:

✓ basic selector cache hit performance (0) (52ms)
✓ selector cacheSize=2 cache hit performance(0) (47ms)
✓ selector cache hit performance for state changes but shallowly equal selector args(0) (197ms)
✓ selector cacheSize=2 cache hit performance for state changes but shallowly equal selector args(0) (222ms)
✓ basic selector cache hit performance (1) (45ms)
✓ selector cacheSize=2 cache hit performance(1) (45ms)
✓ selector cache hit performance for state changes but shallowly equal selector args(1) (440ms)
✓ selector cacheSize=2 cache hit performance for state changes but shallowly equal selector args(1) (225ms)
✓ basic selector cache hit performance (2) (45ms)
✓ selector cacheSize=2 cache hit performance(2) (49ms)
✓ selector cache hit performance for state changes but shallowly equal selector args(2) (209ms)
✓ selector cacheSize=2 cache hit performance for state changes but shallowly equal selector args(2) (226ms)
✓ basic selector cache hit performance (3) (45ms)
✓ selector cacheSize=2 cache hit performance(3) (44ms)
✓ selector cache hit performance for state changes but shallowly equal selector args(3) (213ms)
✓ selector cacheSize=2 cache hit performance for state changes but shallowly equal selector args(3) (221ms)
✓ basic selector cache hit performance (4) (43ms)
✓ selector cacheSize=2 cache hit performance(4) (42ms)
✓ selector cache hit performance for state changes but shallowly equal selector args(4) (201ms)
✓ selector cacheSize=2 cache hit performance for state changes but shallowly equal selector args(4) (218ms)

P.S. I should also mention that I took some inspiration (and tests) from #210. They implemented a LRU cache, but I don't think the performance hit is worth it, and I don't think it was implemented in a very performant way.

@coveralls
Copy link

coveralls commented Mar 29, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 17c8845 on ndbroadbent:memoizeCacheSize into fb8dd53 on reactjs:master.

@codecov-io
Copy link

codecov-io commented Mar 29, 2017

Codecov Report

Merging #238 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #238   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          15     24    +9     
=====================================
+ Hits           15     24    +9
Impacted Files Coverage Δ
src/index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb8dd53...6721a43. Read the comment docs.

@ndbroadbent ndbroadbent force-pushed the memoizeCacheSize branch 2 times, most recently from 93f66e5 to 99645fb Compare March 29, 2017 15:09
@coveralls
Copy link

coveralls commented Mar 29, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 99645fb on ndbroadbent:memoizeCacheSize into fb8dd53 on reactjs:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 99645fb on ndbroadbent:memoizeCacheSize into fb8dd53 on reactjs:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 99645fb on ndbroadbent:memoizeCacheSize into fb8dd53 on reactjs:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 99645fb on ndbroadbent:memoizeCacheSize into fb8dd53 on reactjs:master.

@coveralls
Copy link

coveralls commented Mar 29, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 99645fb on ndbroadbent:memoizeCacheSize into fb8dd53 on reactjs:master.

@coveralls
Copy link

coveralls commented Mar 29, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 8837c03 on ndbroadbent:memoizeCacheSize into fb8dd53 on reactjs:master.

@coveralls
Copy link

coveralls commented Mar 29, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling e0860b2 on ndbroadbent:memoizeCacheSize into fb8dd53 on reactjs:master.

@ndbroadbent ndbroadbent changed the title Added a memoize function with a configurable cache size. Added a cacheSize option to defaultMemoize, to store multiple results. Mar 29, 2017
@ndbroadbent ndbroadbent changed the title Added a cacheSize option to defaultMemoize, to store multiple results. Added a cacheSize option to defaultMemoize, to store multiple results Mar 29, 2017
@ndbroadbent ndbroadbent changed the title Added a cacheSize option to defaultMemoize, to store multiple results Added a cacheSize option to defaultMemoize, to cache multiple results Mar 29, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling efe765c on ndbroadbent:memoizeCacheSize into fb8dd53 on reactjs:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling efe765c on ndbroadbent:memoizeCacheSize into fb8dd53 on reactjs:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling efe765c on ndbroadbent:memoizeCacheSize into fb8dd53 on reactjs:master.

@coveralls
Copy link

coveralls commented Mar 29, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling efe765c on ndbroadbent:memoizeCacheSize into fb8dd53 on reactjs:master.

@ndbroadbent ndbroadbent force-pushed the memoizeCacheSize branch 2 times, most recently from f1417fd to 9babf91 Compare March 29, 2017 16:30
@coveralls
Copy link

coveralls commented Mar 29, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 9babf91 on ndbroadbent:memoizeCacheSize into fb8dd53 on reactjs:master.

@coveralls
Copy link

coveralls commented Mar 29, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling b074b0f on ndbroadbent:memoizeCacheSize into fb8dd53 on reactjs:master.

@coveralls
Copy link

coveralls commented Mar 29, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 6e97916 on ndbroadbent:memoizeCacheSize into fb8dd53 on reactjs:master.

@coveralls
Copy link

coveralls commented Mar 29, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling eae325d on ndbroadbent:memoizeCacheSize into fb8dd53 on reactjs:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling eae325d on ndbroadbent:memoizeCacheSize into fb8dd53 on reactjs:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling eae325d on ndbroadbent:memoizeCacheSize into fb8dd53 on reactjs:master.

@ndbroadbent ndbroadbent changed the title Added a cacheSize option to defaultMemoize, to cache multiple results Added a cacheSize option to defaultMemoize, to add a FIFO cache for multiple results Mar 30, 2017
@ndbroadbent ndbroadbent changed the title Added a cacheSize option to defaultMemoize, to add a FIFO cache for multiple results Added a cacheSize option to defaultMemoize, to add a simple FIFO cache for multiple results Mar 30, 2017
@coveralls
Copy link

coveralls commented Mar 30, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 6721a43 on ndbroadbent:memoizeCacheSize into fb8dd53 on reactjs:master.

@ndbroadbent
Copy link
Author

I made some changes recently:

I added a lastCacheHitIndex variable, so that any subsequent lookups do not have to loop through the equality check. If the result is not found at lastCacheHitIndex, it will begin searching from newest to oldest.

After running the performance benchmarks many times, I concluded that the new defaultMemoize performance is just as good as the original one, so there is no need to keep two versions. Also, after removing the old version, I realized that I had broken the "recomputes result after exception" and "memoizes previous result before exception" tests, because my new code was setting the args and results in the wrong order. So it's far better to just keep one version of the defaultMemoize function, to catch bugs in one place.

@ndbroadbent
Copy link
Author

Also it would be great if coveralls could only comment when there's a change

@ndbroadbent ndbroadbent changed the title Added a cacheSize option to defaultMemoize, to add a simple FIFO cache for multiple results Added a simple FIFO cache to defaultMemoize, with a cacheSize option Mar 30, 2017
@ndbroadbent
Copy link
Author

cc: @ellbee

I was wondering if someone might be able to take a look at this? I've been using this branch in my own app for the last few weeks, and it seems to be working great for me.

@b6pzeusbc54tvhw5jgpyw8pwz2x6gs

@ndbroadbent
On March 25th they added re-reselect to the README. I think they are trying to solve the cache function through re-reselect.

@ndbroadbent
Copy link
Author

Hi @b6pzeusbc54tvhw5jgpyw8pwz2x6gs - I actually use re-reselect as well, but I found that I needed both types of caching. The problem with re-reselect is that the cache is unbounded, so in my app that would cause a bad memory leak. There are some cases where it's really nice to be able to set a fixed cache of 2 or 3, and then you don't have to worry about it.

@timhwang21
Copy link

@ellbee any possibility of an update on this? I have a use case where I need a cache size of exactly two. This would be extremely handy. Thanks!

@toomuchdesign
Copy link

Ping 😃.

From version 1.0.0, re-reselect comes with a cacheObject option to provide custom caching behaviours. The library expose also 3 pre-configured cache strategies which might comes in handy in @ndbroadbent case:

  • flat
  • fifo
  • lru

Greetings!

@markerikson
Copy link
Contributor

Superseded by #513 .

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

Successfully merging this pull request may close these issues.

None yet

7 participants