Improve performance of age limit trimming (#224)

* Improve performance of age limit trimming

This does two things:
1. Before this patch, PINCache was naively scheduling a task to trim to age limit *recursively* every time the age limit was set. This makes it so recursive calls are canceled when it detects another recursive call has been kicked off. This is still less than ideal :/
2. Trimming (mistakenly) used to be a high priority task. Now the age limit itself is set with a high priority, but the trimming is done at a low priority.

* Update CHANGELOG

* Explicitely retain self :/

* Should make tests more reliable

* Fix wait until metadata is known in tests

* Set internal modification immediately.

Author: garrettmoon

CHANGELOG.md

@@ -5,6 +5,7 @@
 - [performance] Reduce locking churn in cleanup methods. [#212](https://github.com/pinterest/PINCache/pull/212)
 - [fix] Don't set file protection unless requested. [#220](https://github.com/pinterest/PINCache/pull/220)
 - [new] Add ability to set an object level TTL: [#209](https://github.com/pinterest/PINCache/pull/209)
+- [performance] Improve performance of age limit trimming: [#224](https://github.com/pinterest/PINCache/pull/224)
 
 ## 3.0.1 -- Beta 6
 - [fix] Add some sane limits to the disk cache: [#201]https://github.com/pinterest/PINCache/pull/201

Source/PINDiskCache.m

@@ -586,13 +586,6 @@ - (BOOL)_locked_setFileModificationDate:(NSDate *)date forURL:(NSURL *)fileURL
                                                            error:&error];
     PINDiskCacheError(error);
 
-    if (success) {
-        NSString *key = [self keyForEncodedFileURL:fileURL];
-        if (key) {
-            _metadata[key].lastModifiedDate = date;
-        }
-    }
-    
     return success;
 }
 
@@ -785,13 +778,24 @@ - (void)trimToAgeLimitRecursively
         return;
 
     NSDate *date = [[NSDate alloc] initWithTimeIntervalSinceNow:-ageLimit];
-    [self trimDiskToDate:date];
+    [self trimToDateAsync:date completion:nil];
 
     dispatch_time_t time = dispatch_time(DISPATCH_TIME_NOW, (int64_t)(ageLimit * NSEC_PER_SEC));
     dispatch_after(time, dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^(void) {
-        [self.operationQueue scheduleOperation:^{
-            [self trimToAgeLimitRecursively];
-        } withPriority:PINOperationQueuePriorityLow];
+        // Ensure that ageLimit is the same as when we were scheduled, otherwise, we've been
+        // rescheduled (another dispatch_after was issued) and should cancel.
+        BOOL shouldReschedule = YES;
+        [self lock];
+            if (ageLimit != self->_ageLimit) {
+                shouldReschedule = NO;
+            }
+        [self unlock];
+        
+        if (shouldReschedule) {
+            [self.operationQueue scheduleOperation:^{
+                [self trimToAgeLimitRecursively];
+            } withPriority:PINOperationQueuePriorityLow];
+        }
     });
 }
 
@@ -1059,8 +1063,10 @@ - (id)objectForKeyedSubscript:(NSString *)key
               }
               [self lock];
             }
-            if (object)
+            if (object) {
+                _metadata[key].lastModifiedDate = now;
                 [self asynchronouslySetFileModificationDate:now forURL:fileURL];
+            }
         }
     [self unlock];
 
@@ -1090,6 +1096,7 @@ - (NSURL *)fileURLForKey:(NSString *)key updateFileModificationDate:(BOOL)update
     [self lockForWriting];
         if (fileURL.path && [[NSFileManager defaultManager] fileExistsAtPath:fileURL.path]) {
             if (updateFileModificationDate) {
+                _metadata[key].lastModifiedDate = now;
                 [self asynchronouslySetFileModificationDate:now forURL:fileURL];
             }
         } else {
@@ -1506,7 +1513,9 @@ - (void)setAgeLimit:(NSTimeInterval)ageLimit
             self->_ageLimit = ageLimit;
         [self unlock];
 
-        [self trimToAgeLimitRecursively];
+        [self.operationQueue scheduleOperation:^{
+            [self trimToAgeLimitRecursively];
+        } withPriority:PINOperationQueuePriorityLow];
     } withPriority:PINOperationQueuePriorityHigh];
 }
 

Source/PINMemoryCache.m

@@ -298,9 +298,20 @@ - (void)trimToAgeLimitRecursively
 
     dispatch_time_t time = dispatch_time(DISPATCH_TIME_NOW, (int64_t)(ageLimit * NSEC_PER_SEC));
     dispatch_after(time, dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^(void){
-        [self.operationQueue scheduleOperation:^{
-            [self trimToAgeLimitRecursively];
-        } withPriority:PINOperationQueuePriorityHigh];
+        // Ensure that ageLimit is the same as when we were scheduled, otherwise, we've been
+        // rescheduled (another dispatch_after was issued) and should cancel.
+        BOOL shouldReschedule = YES;
+        [self lock];
+            if (ageLimit != self->_ageLimit) {
+                shouldReschedule = NO;
+            }
+        [self unlock];
+        
+        if (shouldReschedule) {
+            [self.operationQueue scheduleOperation:^{
+                [self trimToAgeLimitRecursively];
+            } withPriority:PINOperationQueuePriorityHigh];
+        }
     });
 }
 

Tests/PINCacheTests.m

@@ -1245,8 +1245,8 @@ - (void)testDiskRehydrationOfObjectAgeLimit
     // Re-initialize the cache, this should read the age limit for the object from the extended file system attributes.
     testCache = [[PINDiskCache alloc] initWithName:cacheName];
     [testCache setTtlCacheSync:YES];
-    //This should not return until *after* disk cache directory has been created
-    [testCache setObject:@"some bogus object" forKey:@"some bogus key"];
+    
+    [testCache waitForKnownState];
     id object = testCache.metadata[key];
     id ageLimitFromDisk = [object valueForKey:@"ageLimit"];
     XCTAssertEqual([ageLimitFromDisk doubleValue], ageLimit);

Tests/PINDiskCache+PINCacheTests.h

@@ -8,4 +8,9 @@
  */
 - (void)setTtlCacheSync:(BOOL)ttlCache;
 
+/**
+ Waits until all metadata has been read off the disk
+ */
+- (void)waitForKnownState;
+
 @end

Tests/PINDiskCache+PINCacheTests.m

@@ -1,23 +1,28 @@
 #import "PINDiskCache+PINCacheTests.h"
 
-@interface PINDiskCache ()
-- (void)setTtlCache:(BOOL)ttlCache;
+@interface PINDiskCache () {
+    BOOL _ttlCache;
+}
+
+- (void)lock;
+- (void)lockAndWaitForKnownState;
+- (void)unlock;
+
 @end
 
 @implementation PINDiskCache (PINCacheTests)
 
 - (void)setTtlCacheSync:(BOOL)ttlCache
 {
-    [self setTtlCache:ttlCache];
+    [self lock];
+        self->_ttlCache = ttlCache;
+    [self unlock];
+}
 
-    // Attempt to read from the cache. This will be put on the same queue as `setTtlCache`, but at a lower priority.
-    // When the completion handler runs, we can be sure the property value has been set.
-    dispatch_group_t group = dispatch_group_create();
-    dispatch_group_enter(group);
-    [self objectForKeyAsync:@"some bogus key" completion:^(PINDiskCache *cache, NSString *key, id<NSCoding> object) {
-        dispatch_group_leave(group);
-    }];
-    dispatch_group_wait(group, DISPATCH_TIME_FOREVER);
+- (void)waitForKnownState
+{
+    [self lockAndWaitForKnownState];
+    [self unlock];
 }
 
 @end