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

Memory usage and other issues for engine cache #127

Open
jsdalton opened this issue Nov 7, 2012 · 1 comment
Open

Memory usage and other issues for engine cache #127

jsdalton opened this issue Nov 7, 2012 · 1 comment

Comments

@jsdalton
Copy link

jsdalton commented Nov 7, 2012

Have you guys given any consideration to how the cache module used by the Couchdb engine uses memory? Without a cache eviction strategy, it would appear that this cache will continue to grow indefinitely.

An in-memory LRU cache would be a great solution for the problem (if I'm not mistaken, that's how CradleDB handles it.) This library has a straightforward implementation with only minor API differences to your cache: https://github.com/isaacs/node-lru-cache/ and could pretty easily be substituted.

Barring a rewrite of the cache library, I'd at least like to propose that you guys use dependency injection to set up the cache on the engines, e.g. in the engine constructor, change:

this.cache = new resourceful.Cache();

to:

this.cache = config.cache || new resourceful.Cache();

Which would allow a user to implement their own solution to the above issues. One would still be boxed in by the requirement for a synchronous cache API, but presumably that's a separate issue.

@jsdalton
Copy link
Author

jsdalton commented Nov 7, 2012

As a slightly separate issue, the tests in cache-test.js appear to be a bit wonky.

The implication in the vows is that on get() and find() the values should be retrieved from the cache if they exist there. The tests aren't showing this however, they are just showing a superficial match of properties. (An easy way to demonstrate the tests aren't doing what they are supposed to is to clear the cache in the first batch of tests or not clear it in the second. The tests pass either way.)

The behavior specified is also a bit confusing. Not sure why you'd want to return cached values for get() and find(). If you run more than one server instance those values could get out of whack if they are updated via another instance. But maybe I'm misunderstanding what these tests are trying to test for.

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

No branches or pull requests

1 participant