Add ability to set an object level TTL #209

mjlazar · 2017-12-16T14:47:26Z

I think this partially addresses #31.

…ontainsObjectForKey:]

…he into object-level-ttl-override # Conflicts: # Source/PINDiskCache.m

garrettmoon

This looks like a really useful change. Can you please look over the comments and add some unit tests to PINCacheTests.m?

garrettmoon · 2018-02-13T00:52:34Z

Source/PINCaching.h

+
+ @param object An object to store in the cache.
+ @param key A key to associate with the object. This string will be copied.
+ @param ageLimit The age limit (in seconds) to associate with the object.


Maybe mention 0 being no age limit here and in the other doc headers?

garrettmoon · 2018-02-13T01:00:32Z

Source/PINDiskCache.m

@@ -485,6 +491,11 @@ - (void)initializeDiskProperties
                _metadata[key].size = fileSize;
                byteCount += [fileSize unsignedIntegerValue];
            }
+
+            NSTimeInterval ageLimit;


Do you have any guesses as to the performance cost of this?

Separately, I believe this method returns -1 on failure? That should be handled here too?

Added code to handle the failure.

So, I've spent some time doing some analysis and trying out different performance tweaks (which is why I haven't responded to your comments yet). Since PINDiskCache.ttlCache is not known at disk initialization, we cannot skip over the code which reads the extended file system attributes from disk.

There is a performance hit that will be incurred on cache initialization and is a function of the number of files in the cache. I tested adding 1000 objects into the disk cache and then compared initialization times between the current code and the code within this pull request. On average it took ~24ms to initialize the cache with the current code. On average it took ~35ms to initialize after the code changes, which is ~11ms/44% slower.

The slowdown might not be worth it for users that do not take advantage of the object-level age limits, so I have a proposal for mitigation:

Make PINDiskCache.ttlCache (and PINDiskCache.ageLimit?) a readonly property (which will obviously break people who are currently using it).

Add a ttlCache parameter to PINDiskCache's designated initializer.

Skip over reading the extended file system attributes if ttlCache == NO.

This change will basically have zero effect on users who are not using ttlCache.

garrettmoon · 2018-02-13T01:01:22Z

Source/PINDiskCache.m

+
+- (BOOL)_locked_setAgeLimit:(NSTimeInterval)ageLimit forURL:(NSURL *)fileURL
+{
+    if (ageLimit <= 0.0 || !fileURL) {


If ageLimit being <= 0 early returns, is there a way to clear the ageLimit on an object?

Good call. Added code here to clear the ageLimit if 0 is passed in.

garrettmoon · 2018-02-13T01:03:19Z

Source/PINDiskCache.m

+    }
+
+    NSError *error = nil;
+    if (setxattr([fileURL fileSystemRepresentation], PINDiskCacheAgeLimitAttributeName, &ageLimit, sizeof(NSTimeInterval), 0, 0) != 0) {


Is this method thread safe? The fact that it can set the global errno variable leads me to believe it may not be?

Yes it is thread safe. errno is actually a macro that expands to a function that returns the calling thread's errno value. See /usr/include/sys/errno.h.

garrettmoon · 2018-02-13T17:55:35Z

Source/PINDiskCache.m

@@ -845,8 +898,13 @@ - (BOOL)containsObjectForKey:(NSString *)key
 {
    [self lock];
        if (_metadata[key] != nil || _diskStateKnown == NO) {
+            BOOL objectExpired = NO;
+            if (self->_ttlCache && _metadata[key].date != nil) {


Should this be an || ? Or does the ageLimit option only apply if ttlCache is true? If that's the case, should we assert if someone attempts to set an ageLimit on a cache which isn't marked ttlCache?

Yes, ageLimit only applies if ttlCache is true. I added an assertion as you suggested.

I think the assert got removed in a follow up commit?

garrettmoon · 2018-02-13T17:56:33Z

Source/PINDiskCache.m

            [self unlock];
-            return ([self fileURLForKey:key updateFileModificationDate:NO] != nil);
+            return ([self fileURLForKey:key updateFileModificationDate:NO] != nil && !objectExpired);


Flip these around for performance?

garrettmoon · 2018-02-14T23:30:04Z

Source/PINDiskCache.m

-
-        if (!self->_ttlCache || self->_ageLimit <= 0 || fabs([_metadata[key].date timeIntervalSinceDate:now]) < self->_ageLimit) {
+
+        NSTimeInterval ageLimit = _metadata[key].hasAgeLimit ? _metadata[key].ageLimit : self->_ageLimit;


Same question as above.

Obsolete. hasAgeLimit has been removed.

garrettmoon · 2018-02-14T23:30:50Z

Source/PINDiskCache.m

-        if (!self->_ttlCache || self->_ageLimit <= 0 || fabs([_metadata[key].date timeIntervalSinceDate:now]) < self->_ageLimit) {
+
+        NSTimeInterval ageLimit = _metadata[key].hasAgeLimit ? _metadata[key].ageLimit : self->_ageLimit;
+        if (!self->_ttlCache || ageLimit <= 0 || fabs([_metadata[key].date timeIntervalSinceDate:now]) < ageLimit) {


Is this different behavior than above? I.e. it behaves like a ttlCache if there's an age limit or it's marked as a ttlCache here but not above?

I think this is basically saying:

If ttlCache == NO, return the object.

If ttlCache == YES and ageLimit is not set, return the file.

If ttlCache == YES, ageLimit is set and object is not expired return the object.

garrettmoon · 2018-02-14T23:33:39Z

Source/PINDiskCache.m

@@ -1025,7 +1094,9 @@ - (void)setObject:(id <NSCoding>)object forKey:(NSString *)key fileURL:(NSURL **
            if (date) {
                self->_metadata[key].date = date;
            }
-
+            if (ageLimit > 0.0) {
+                [self asynchronouslySetAgeLimit:ageLimit forURL:fileURL];


Not sure the answer to this: will there be implications to this being asynchronous? Specifically that _metadata[key].ageLimit won't be set immediately and checks against it could happen before it is?

I'm guessing the answer is no, but maybe there could be a comment here suggesting it's been thought through and why it's safe?

garrettmoon · 2018-02-14T23:35:25Z

Source/PINDiskCache.m

+
+- (void)setAgeLimit:(NSTimeInterval)ageLimit {
+    _ageLimit = ageLimit;
+    _hasAgeLimit = YES;


Would it be better to just make a getter for hasAgeLimit which checks _ageLimit > 0?

My thoughts were if ageLimit were set to anything (including zero), it would override the cache's ageLimit. I realize this is actually confusing, so I removed hasAgeLimit; the object's ageLimit must now be greater than zero in order to override the cache's ageLimit.

…e init] This will allow us to swizzle +[NSDate date] in unit tests and return an arbitrary date in order to test TTLs.

…re set

… level age limits

…t-level age limits exist

garrettmoon · 2018-02-22T22:29:44Z

Source/PINDiskCache.m

@@ -455,6 +455,7 @@ - (BOOL)_locked_createCacheDirectory
 - (void)initializeDiskProperties
 {
    NSUInteger byteCount = 0;
+    BOOL hasAtLeastOneAgeLimit = NO;


Should this be initialized to the value of isTTLCache?

The value of isTTLCache is unset at this point. See my other comment about adding this as a param to the designated initializer.

This is no longer true?

I removed the hasAtLeastOneAgeLimit var all together since the isTTLCache check is all we need.

garrettmoon · 2018-02-22T22:31:55Z

Source/PINDiskCache.m

@@ -553,15 +566,26 @@ - (void)asynchronouslySetAgeLimit:(NSTimeInterval)ageLimit forURL:(NSURL *)fileU

 - (BOOL)_locked_setAgeLimit:(NSTimeInterval)ageLimit forURL:(NSURL *)fileURL
 {
-    if (ageLimit <= 0.0 || !fileURL) {
+    if (!fileURL) {
        return NO;


I wonder if we should throw an assert here? Any valid reason to call this with a nil URL?

I wasn't sure what the implications of asserting here. -_locked_setFileModificationDate does a similar nil check/return early. WDTY?

garrettmoon · 2018-02-22T22:46:45Z

Source/PINDiskCache.m

+            }
+        }];
+
+        for (NSString *key in expiredObjectKeys) {


Could you just unlock before this loop since expiredObjectKeys is created in this method? Then you don't need to unlock and relock in each loop iteration? Hmm, I bet I can do this elsewhere…

garrettmoon · 2018-02-22T22:48:58Z

Source/PINDiskCache.m

@@ -673,7 +697,7 @@ - (void)trimDiskToDate:(NSDate *)trimDate

        for (NSString *key in keysSortedByDate) { // oldest files first
            NSDate *accessDate = _metadata[key].date;
-            if (!accessDate)
+            if (!accessDate || _metadata[key].ageLimit > 0.0)


This seems a bit confusing to me… Should trimDiskToDate do nothing to an object if an ageLimit is set? Perhaps this is correct… but trimToAgeLimitRecursively should call removeExpiredObjectsAsync? Or maybe trimToAgeLimitRecursively should be removed in favor of removeExpiredObjects?

Yeah, I agree it is sort of confusing. My thoughts here are that the trimDate that is passed in here is based on the cache-level age limit and we don't want to remove objects that have overridden the age limit.

I don't know what the correct thing to do here would be. -trimToAgeLimitRecursively calling -removeExpiredObjectsAsync would certainly be an option, but I wanted to avoid over-calling it since it has to iterate over all of the objects in the cache to determine if they are expired or not. Although, I don't think there is much harm in letting these expired objects lay dormant on the filesystem until the next initialization; the getters/iterators will treat them as if they don't exist.

Ok, that makes sense, I think if we address the issue I mention below regarding trimDiskToSizeByDate and trimToCostLimitByDate

garrettmoon · 2018-02-22T23:23:43Z

Source/PINMemoryCache.m

@@ -196,6 +200,27 @@ - (void)trimMemoryToDate:(NSDate *)trimDate
    }
 }

+- (void)removeExpiredObjects


Sorry this comment is in the wrong place (you're using dates[key] below which gets updated on access in objectForKey), but, should objectForKey: not set the access date if an ageLimit is set?

Hmm, good point. The disk cache checks if object && !self->_ttlCache before setting the access date. This logic currently exists; I wonder if that's a bug. Shall I update the memory cache to do the same check?

I'm wondering which is actually the bug…

trimDiskToSizeByDate and trimToCostLimitByDate both use last access to trim to order which objects they trim first. If ttlCache is enabled or there are age limits, objects which exceed the age limit should be trimmed first and then objects should be trimmed by last access. Does that seem right? In which case the two methods will need to be updated and the diskCache will need to remove its check of object && !self->_ttlCache…

Made the updates you suggested. PTAL.

garrettmoon · 2018-02-22T23:26:36Z

Thanks for writing the tests!

garrettmoon · 2018-02-22T23:29:13Z

Source/PINCache.m

 - (void)setObjectAsync:(id <NSCoding>)object forKey:(NSString *)key withCost:(NSUInteger)cost completion:(PINCacheObjectBlock)block
+{
+    [self setObjectAsync:object forKey:key withCost:cost ageLimit:0.0 completion:block];


Once this lands, I wonder if we could reduce the complexity of everything by changing this line to:
[self setObjectAsync:object forKey:key withCost:cost ageLimit:self.isTTLCache ? self.ageLimit : 0.0 completion:block]

Then we could get rid of any other logic that deals with isTTLCache?

I think that could certainly be done, but it would have to be in conjunction with some changes to the trim-to-date methods. Since this would essentially set object-level age limits all of the time, we could combine the current trimming logic with the new -removeExpiredObjects logic I'm proposing in this PR.

mjlazar · 2018-02-24T17:15:05Z

Warnings should be fixed now.

mjlazar · 2018-02-25T14:35:13Z

@garrettmoon In one of my previous comments, I proposed passing the ttlCache switch to the initializer(s) and making the property readonly. I just pushed a commit that implements that. PTAL; this would be a breaking change to folks that are utilizing ttlCache.

ghost · 2018-02-25T19:10:41Z

🚫 CI failed with log

mjlazar · 2018-02-26T16:08:34Z

@garrettmoon is there any chance the latest CI failure is due to a flaky test? I can't reproduce locally.

garrettmoon · 2018-02-27T01:17:37Z

@mjlazar I wasn't able to repro either so I'll re-run. I've never seen that one fail due to flakiness though…

garrettmoon · 2018-02-27T21:48:28Z

Source/PINDiskCache.h

@@ -280,6 +292,29 @@ PIN_SUBCLASSING_RESTRICTED
 */
 - (instancetype)initWithName:(nonnull NSString *)name rootPath:(nonnull NSString *)rootPath serializer:(nullable PINDiskCacheSerializerBlock)serializer deserializer:(nullable PINDiskCacheDeserializerBlock)deserializer operationQueue:(nonnull PINOperationQueue *)operationQueue __attribute__((deprecated));

+/**
+ The designated initializer allowing you to override default NSKeyedArchiver/NSKeyedUnarchiver serialization.


Nit: this is no longer the designated initializer.

Done. Removed.

garrettmoon · 2018-02-27T21:52:20Z

Source/PINDiskCache.m

@@ -455,6 +455,7 @@ - (BOOL)_locked_createCacheDirectory
 - (void)initializeDiskProperties
 {
    NSUInteger byteCount = 0;
+    BOOL hasAtLeastOneAgeLimit = NO;


This is no longer true?

garrettmoon · 2018-02-27T22:16:38Z

Source/PINDiskCache.m

+        }];
+
+        [self unlock];
+            for (NSString *key in expiredObjectKeys) {


Can this for-loop be moved outside the lock? Or the [self lock] below should be changed to [self lockForWriting]?

Yes. Moved this outside the lock.

garrettmoon · 2018-02-27T22:42:21Z

Source/PINMemoryCache.m

@@ -196,6 +200,27 @@ - (void)trimMemoryToDate:(NSDate *)trimDate
    }
 }

+- (void)removeExpiredObjects


I'm wondering which is actually the bug…

trimDiskToSizeByDate and trimToCostLimitByDate both use last access to trim to order which objects they trim first. If ttlCache is enabled or there are age limits, objects which exceed the age limit should be trimmed first and then objects should be trimmed by last access. Does that seem right? In which case the two methods will need to be updated and the diskCache will need to remove its check of object && !self->_ttlCache…

garrettmoon · 2018-02-27T22:42:53Z

Source/PINDiskCache.m

@@ -845,8 +898,13 @@ - (BOOL)containsObjectForKey:(NSString *)key
 {
    [self lock];
        if (_metadata[key] != nil || _diskStateKnown == NO) {
+            BOOL objectExpired = NO;
+            if (self->_ttlCache && _metadata[key].date != nil) {


I think the assert got removed in a follow up commit?

garrettmoon · 2018-02-27T22:46:36Z

Source/PINMemoryCache.m

+    NSDate *now = [NSDate date];
+    for (NSString *key in ageLimits) {
+        NSDate *accessDate = dates[key];
+        NSTimeInterval ageLimit = [_ageLimits[key] doubleValue] ?: self->_ageLimit;


Accessing these ivars is unsafe, gather self->_ageLimit as globalAgeLimit in the lock above and use ageLimits here?

garrettmoon · 2018-02-27T22:51:55Z

Source/PINDiskCache.m

@@ -673,7 +697,7 @@ - (void)trimDiskToDate:(NSDate *)trimDate

        for (NSString *key in keysSortedByDate) { // oldest files first
            NSDate *accessDate = _metadata[key].date;
-            if (!accessDate)
+            if (!accessDate || _metadata[key].ageLimit > 0.0)


Ok, that makes sense, I think if we address the issue I mention below regarding trimDiskToSizeByDate and trimToCostLimitByDate

Specifically, this commit does the following: - Remove designated initializer language from non-designated initializers in header documentation - Remove `hasAtLeastOneAgeLimit` logic from `-initializeDiskProperties` - Prune expired objects first in `-[PINDiskCache trimDiskToDate:]` and `-[PINMemoryCache trimToCostLimitByDate:]`. - Set file modification date regardless if TTL cache or not. - Fix unsafe ivar access in PINMemoryCache - In tests that need `ttlCache` set to YES, do so before calling `-removeAllObjects` to avoid race condition.

garrettmoon · 2018-03-27T20:44:25Z

Source/PINDiskCache.m

+        NSMutableArray<NSString *> *expiredObjectKeys = [NSMutableArray array];
+        [_metadata enumerateKeysAndObjectsUsingBlock:^(NSString * _Nonnull key, PINDiskCacheMetadata * _Nonnull obj, BOOL * _Nonnull stop) {
+            NSTimeInterval ageLimit = obj.ageLimit > 0.0 ? obj.ageLimit : self->_ageLimit;
+            NSDate *expirationDate = [obj.date dateByAddingTimeInterval:ageLimit];


Ok, so I think there's a problem: now we're always setting date. It should probably be renamed lastModifiedDate and a new property createdDate should be added? Because objects shouldn't have their expiration lengthened when they're accessed, but they should live longer, in relation to other objects, than other objects which haven't expired but were accessed later? This behavior will have to be added / updated in PINMemoryCache as well?

Good call. I modified the code using your suggestions. PTAL when you get a chance.

Specifically, this commit does the following: - In disk cache, rename 'date' -> 'lastModifiedDate'. - In in-mem cache, rename 'dates' -> 'accessDates'. - Add new 'created date' in both in-mem and disk caches. - Use 'created date' to determine object expiration in both caches. - Use 'accessDates' when trimming in-mem cache to cost limit by date. - Use 'lastModifiedDate' when trimming disk cache to size.

garrettmoon · 2018-05-04T21:13:36Z

This is looking great! One final request: would you be willing to update the tests to exercise our new creation / modification logic?

mjlazar · 2018-05-08T21:08:23Z

@garrettmoon, I added some additional tests that exercise the new logic. Let me know what you think. Thanks!

ghost · 2018-05-08T21:54:04Z

🚫 CI failed with log

garrettmoon · 2018-05-08T23:22:40Z

@mjlazar looks good to me! Looks like there's an assertion failure in one of the tests though?

mjlazar · 2018-05-09T13:16:40Z

Seems to be a race condition. I'll push a fix shortly.

mjlazar · 2018-05-09T14:17:53Z

@garrettmoon, would you mind trying the tests again? Thanks.

ghost · 2018-05-09T16:07:18Z

	1 Warning
⚠️	This is a big PR, please consider splitting it up to ease code review.

Generated by 🚫 Danger

garrettmoon

Let's do this! Thanks so much for the contribution and for addressing all my feedback!

mjlazar · 2018-05-14T13:55:08Z

Awesome! Thanks for reviewing.

mjlazar added 8 commits December 5, 2017 10:54

Add new ageLimit setters in PINCaching protocol

a08b66c

Implement ageLimit methods in PINDiskCache

3d7714e

Implement ageLimit methods in PINMemoryCache

d1c796a

Implement ageLimit methods in PINCache

f7d6d3c

Honor ageLimit (at both cache and object level) in -[PINDiskCache c…

336d401

…ontainsObjectForKey:]

Honor ageLimit (at both cache and object level) in -[PINDiskCache c…

6be8d22

…ontainsObjectForKey:]

Merge branch 'object-level-ttl-override' of github.com:mjlazar/PINCac…

c2dfcd6

…he into object-level-ttl-override # Conflicts: # Source/PINDiskCache.m

Update CHANGELOG

4b05c88

garrettmoon reviewed Feb 14, 2018

View reviewed changes

mjlazar added 16 commits February 16, 2018 11:26

Add new NSDate category for swizzling +[NSDate date] in unit tests

0d4e71d

Change memory and disk caches to use +[NDate date] instead of -[NSDat…

d4d7640

…e init] This will allow us to swizzle +[NSDate date] in unit tests and return an arbitrary date in order to test TTLs.

Add unit tests for object-level TTLs

b4b7ac2

Update header documentation regarding object-level age limits

171de47

Clear previously set object age limits if setting to zero

723c326

Add assertions to ttlCache is set to YES if object-level age limits a…

06e331c

…re set

Update header documentation to describe behavior of overridden object…

3895432

… level age limits

Remove hasAgeLimit logic

a4466b6

Misc cleanup

c762a84

Handle failure on extended attribute read

fd103ca

Make sure trim methods do not remove objects with overridden age limits

fb7694b

New methods to remove expired objects from cache

e504b0b

Remove expired objects from disk cache on initialization if any objec…

24ea58a

…t-level age limits exist

Remove expired objects from memory cache on memory warning

f539d78

Fix expiration logic

a27d755

Add unit test for removeExpiredObjects

4b383a5

garrettmoon reviewed Feb 22, 2018

View reviewed changes

mjlazar added 2 commits February 23, 2018 10:34

Remove assert in -setObject:...

662a7cb

Call async version of -removeExpiredObjects in -initializeDiskProperties

7fa42e3

Fix warnings

92e1d59

Make ttlCache a readonly property

71b7fb9

garrettmoon reviewed Feb 27, 2018

View reviewed changes

mjlazar and others added 3 commits March 17, 2018 12:13

Merge branch 'master' into object-level-ttl-override

6a34a58

Merge branch 'master' into object-level-ttl-override

a47eff1

garrettmoon reviewed Mar 27, 2018

View reviewed changes

mjlazar and others added 3 commits April 16, 2018 10:06

Fix indents.

fab908f

Merge branch 'master' into object-level-ttl-override

f7216ed

garrettmoon mentioned this pull request Apr 26, 2018

Fix mem cache TTL #222

Closed

Adding additional tests

60c83aa

Fix race condition in unit tests

2655498

Update CHANGELOG

871f503

garrettmoon approved these changes May 11, 2018

View reviewed changes

garrettmoon merged commit d886490 into pinterest:master May 11, 2018

mjlazar deleted the object-level-ttl-override branch May 14, 2018 13:55


		if (!self->_ttlCache \|\| self->_ageLimit <= 0 \|\| fabs([_metadata[key].date timeIntervalSinceDate:now]) < self->_ageLimit) {

		NSTimeInterval ageLimit = _metadata[key].hasAgeLimit ? _metadata[key].ageLimit : self->_ageLimit;

Add ability to set an object level TTL #209

Add ability to set an object level TTL #209

Conversation

mjlazar commented Dec 16, 2017

garrettmoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrettmoon commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjlazar commented Feb 24, 2018

mjlazar commented Feb 25, 2018

ghost commented Feb 25, 2018

mjlazar commented Feb 26, 2018

garrettmoon commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrettmoon commented May 4, 2018

mjlazar commented May 8, 2018

ghost commented May 8, 2018

garrettmoon commented May 8, 2018

mjlazar commented May 9, 2018

mjlazar commented May 9, 2018

ghost commented May 9, 2018

garrettmoon left a comment

Choose a reason for hiding this comment

mjlazar commented May 14, 2018