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 TTL cache capability #63

Merged
merged 3 commits into from
Feb 8, 2016

Conversation

benasher44
Copy link
Contributor

In #32, I wanted to add what basically amounts to the ability to make the caches behave like a TTL cache. I've done this with tests, which I think clearly illustrate the behavior I was going for here. I'm pretty open to discussion on this, and I've made sure that this is opt-in, so existing behavior of the caches should be unaffected.

@garrettmoon
Copy link
Collaborator

Neat! I'll try and look at this soon (next week by the latest). My greatest concern about this is making sure there are no performance impacts if this new feature is disable.

@benasher44
Copy link
Contributor Author

@garrettmoon cool thanks! I think it should be alright performance-wise. In all cases, I check !self->_ttlCache before starting the TTL cache behavior, so performance impact should be minimal

the cache will behave as if the object does not exist

*/
@property (assign, getter=isTTLCache) BOOL ttlCache;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you make this nonatomic and also create a getter which grabs the ivar in self.lock?

@@ -1052,6 +1059,16 @@ - (void)setAgeLimit:(NSTimeInterval)ageLimit
});
}

- (BOOL)isTTLCache {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need setTTLCache too? Otherwise they'll be set outside of the lock.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It definitely does. I'm not sure why I missed that.

@garrettmoon
Copy link
Collaborator

Should ttl cache be exposed on PINCache too? Or only PINMemoryCache and PINDiskCache?

@benasher44
Copy link
Contributor Author

I can't think of why you would want to use this feature on either only PINMemoryCache or PINDiskCache, but I'd say the same thing about ageLimit (also not exposed in PINCache.h), which ttlCache depends on (ttlCache = YES doesn't mean anything if ageLimit is 0). There's also some added complexity if you go that route. Let's say you do the following:

cache.ttlCache = YES;
cache.diskCache.ttlCache = NO;
NSLog(@"%@", cache.ttlCache ? @"YES" : @"NO");

What would you expect cache.ttlCache to print in this scenario? I think you avoid running into any bugs related to this situation, if you require clients to access ttlCache via either of the sub-caches.

@garrettmoon
Copy link
Collaborator

@benasher44 you've convinced me :)

garrettmoon added a commit that referenced this pull request Feb 8, 2016
@garrettmoon garrettmoon merged commit c7eb866 into pinterest:master Feb 8, 2016
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.

2 participants