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

fix possible unconstrained memory growth in modules/libjsc #891

Merged
merged 4 commits into from Nov 2, 2016

Conversation

Projects
None yet
5 participants
@grondo
Copy link
Contributor

grondo commented Nov 2, 2016

When libjsc was converted over to use the new hierarchical directory scheme under lwj., the mapping between jobid and kvs path was memoized in an internal hash. At the time, no management of entries in this hash was also added, so the current code has the potential for unbounded memory growth when an application using libjsc is long running or managing many jobs.

This PR changes the internal hash of jobid to kvs path mappings into an LRU cache. The cache, for now, has a max size of 256. If needed, this could be made configurable or adjustable later.

The simple LRU cache implementation is added to libutil in case it can be used elsewhere.

grondo added some commits Nov 2, 2016

libutil: initial simplified LRU cache implementation
Add simple LRU cache implemenation to libutil.
build: add lru_cache to libutil
Build and include lru_cache.c in libutil.

@grondo grondo added the review label Nov 2, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 2, 2016

Coverage Status

Coverage increased (+0.08%) to 75.857% when pulling 6556e00 on grondo:jsc-lru-cache into ef819d7 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 2, 2016

Current coverage is 72.20% (diff: 90.24%)

Merging #891 into master will increase coverage by 0.03%

@@             master       #891   diff @@
==========================================
  Files           156        157     +1   
  Lines         26934      27007    +73   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          19438      19501    +63   
- Misses         7496       7506    +10   
  Partials          0          0          
Diff Coverage File Path
•••••••• 87% src/modules/libjsc/jstatctl.c
••••••••• 90% new src/common/libutil/lru_cache.c

Powered by Codecov. Last update ef819d7...eaed541

@garlick
Copy link
Member

garlick left a comment

One tiny typo and everything else looks great to me. Nice touch doing a general lru (with tests!). That may be useful down the line.

lru_cache_t *lru;

lru = lru_cache_create (5);
ok (lru != NULL, "lru_cache_create (10)");

This comment has been minimized.

@garlick

garlick Nov 2, 2016

Member

minor: the argument should be 5 not 10 (maybe that should be a const or something?)

This comment has been minimized.

@grondo

grondo Nov 2, 2016

Author Contributor

Oh, yeah good catch and suggestion thanks!

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Nov 2, 2016

Thanks @garlick, pushed a fixup commit addressing your suggestion -- sorry I was a lazy in test writing.

If that looks ok I'll squash it.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 2, 2016

Not at all, this looks great. Yes squash and I think it's ready.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Nov 2, 2016

Nice work! My only thought was whether we want to make 256 LRU-size configurable. But I couldn't think of jobid access patterns that can significantly benefit from more than 256 entries in the LRU... So this is good as is from what I can tell.

My gut feeling is same as @garlick. There would be other areas where caching with configurable policies and parameters would be useful... Along the similar line, I'm also thinking the red black trees etc that I have been looking at can find general use later on...

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Nov 2, 2016

@dongahn, thanks. I wasn't sure how/where to add the configuration for max lru cache size, so I punted for now, though I didn't look too hard.

grondo added some commits Nov 2, 2016

test: add unit test for lru_cache_t
Add unit tests for simple LRU cache implementation
modules/libjsc: use LRU cache to store kvs paths
Instead of indefinitely growing hash of job id-to-kvs-path
mapping, switch to an LRU cache with 256 entries. This should
allow fast operation on up to 256 jobs simultaneously, without
a problem of unbounded memory growth for long running applications
using libjsc on many jobs.

@grondo grondo force-pushed the grondo:jsc-lru-cache branch from 5d54a2e to eaed541 Nov 2, 2016

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Nov 2, 2016

Ok, squashed and force pushed.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 2, 2016

Coverage Status

Coverage increased (+0.02%) to 75.801% when pulling 5d54a2e on grondo:jsc-lru-cache into ef819d7 on flux-framework:master.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Nov 2, 2016

@grondo: yeah... if we were to do it, we would probably need to add a init or new function to the JSC API and have its user pass the parameter to it. Not worthy it.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 2, 2016

Coverage Status

Coverage increased (+0.03%) to 75.808% when pulling eaed541 on grondo:jsc-lru-cache into ef819d7 on flux-framework:master.

@garlick garlick merged commit a06cf00 into flux-framework:master Nov 2, 2016

4 checks passed

codecov/patch 90.24% of diff hit (target 72.16%)
Details
codecov/project 72.20% (+0.03%) compared to ef819d7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 75.808%
Details

@garlick garlick removed the review label Nov 2, 2016

@grondo grondo deleted the grondo:jsc-lru-cache branch Nov 3, 2016

@grondo grondo referenced this pull request Nov 28, 2016

Closed

Create 0.6.0 release notes #916

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.