From 1f89c1d98d45aa7422e2508646821d4323610ce3 Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Thu, 7 Jun 2018 10:28:39 -0700 Subject: [PATCH 1/6] 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. --- Source/PINDiskCache.m | 23 ++++++++++++++++++----- Source/PINMemoryCache.m | 17 ++++++++++++++--- 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/Source/PINDiskCache.m b/Source/PINDiskCache.m index 66dd2011..83ee5fc3 100644 --- a/Source/PINDiskCache.m +++ b/Source/PINDiskCache.m @@ -785,13 +785,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 != _ageLimit) { + shouldReschedule = NO; + } + [self unlock]; + + if (shouldReschedule) { + [self.operationQueue scheduleOperation:^{ + [self trimToAgeLimitRecursively]; + } withPriority:PINOperationQueuePriorityLow]; + } }); } @@ -1506,7 +1517,9 @@ - (void)setAgeLimit:(NSTimeInterval)ageLimit self->_ageLimit = ageLimit; [self unlock]; - [self trimToAgeLimitRecursively]; + [self.operationQueue scheduleOperation:^{ + [self trimToAgeLimitRecursively]; + } withPriority:PINOperationQueuePriorityLow]; } withPriority:PINOperationQueuePriorityHigh]; } diff --git a/Source/PINMemoryCache.m b/Source/PINMemoryCache.m index 335d78f6..9ee0eac1 100644 --- a/Source/PINMemoryCache.m +++ b/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 != _ageLimit) { + shouldReschedule = NO; + } + [self unlock]; + + if (shouldReschedule) { + [self.operationQueue scheduleOperation:^{ + [self trimToAgeLimitRecursively]; + } withPriority:PINOperationQueuePriorityHigh]; + } }); } From 9200a550980368b448570568fd656388b122afcb Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Thu, 7 Jun 2018 10:36:11 -0700 Subject: [PATCH 2/6] Update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 01d6656f..227da83e 100644 --- a/CHANGELOG.md +++ b/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 From f59f682785e390e48a409e27aff70acf521bc487 Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Thu, 7 Jun 2018 10:43:25 -0700 Subject: [PATCH 3/6] Explicitely retain self :/ --- Source/PINDiskCache.m | 2 +- Source/PINMemoryCache.m | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Source/PINDiskCache.m b/Source/PINDiskCache.m index 83ee5fc3..1ae1f5e1 100644 --- a/Source/PINDiskCache.m +++ b/Source/PINDiskCache.m @@ -793,7 +793,7 @@ - (void)trimToAgeLimitRecursively // rescheduled (another dispatch_after was issued) and should cancel. BOOL shouldReschedule = YES; [self lock]; - if (ageLimit != _ageLimit) { + if (ageLimit != self->_ageLimit) { shouldReschedule = NO; } [self unlock]; diff --git a/Source/PINMemoryCache.m b/Source/PINMemoryCache.m index 9ee0eac1..a1ad33fa 100644 --- a/Source/PINMemoryCache.m +++ b/Source/PINMemoryCache.m @@ -302,7 +302,7 @@ - (void)trimToAgeLimitRecursively // rescheduled (another dispatch_after was issued) and should cancel. BOOL shouldReschedule = YES; [self lock]; - if (ageLimit != _ageLimit) { + if (ageLimit != self->_ageLimit) { shouldReschedule = NO; } [self unlock]; From 53d4dad8342ebd38554edf892272093f040cf329 Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Thu, 7 Jun 2018 11:49:36 -0700 Subject: [PATCH 4/6] Should make tests more reliable --- Tests/PINDiskCache+PINCacheTests.m | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/Tests/PINDiskCache+PINCacheTests.m b/Tests/PINDiskCache+PINCacheTests.m index 6d54a4ec..01db890c 100644 --- a/Tests/PINDiskCache+PINCacheTests.m +++ b/Tests/PINDiskCache+PINCacheTests.m @@ -1,23 +1,21 @@ #import "PINDiskCache+PINCacheTests.h" -@interface PINDiskCache () -- (void)setTtlCache:(BOOL)ttlCache; +@interface PINDiskCache () { + BOOL _ttlCache; +} + +- (void)lock; +- (void)unlock; + @end @implementation PINDiskCache (PINCacheTests) - (void)setTtlCacheSync:(BOOL)ttlCache { - [self setTtlCache:ttlCache]; - - // 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 object) { - dispatch_group_leave(group); - }]; - dispatch_group_wait(group, DISPATCH_TIME_FOREVER); + [self lock]; + self->_ttlCache = ttlCache; + [self unlock]; } @end From 66a9cc929d618de8af9aa2a7c24e320dff7b4982 Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Thu, 7 Jun 2018 12:00:58 -0700 Subject: [PATCH 5/6] Fix wait until metadata is known in tests --- Tests/PINCacheTests.m | 4 ++-- Tests/PINDiskCache+PINCacheTests.h | 5 +++++ Tests/PINDiskCache+PINCacheTests.m | 7 +++++++ 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/Tests/PINCacheTests.m b/Tests/PINCacheTests.m index e9a839e4..d03bc22e 100644 --- a/Tests/PINCacheTests.m +++ b/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); diff --git a/Tests/PINDiskCache+PINCacheTests.h b/Tests/PINDiskCache+PINCacheTests.h index 3fb5f557..ce487335 100644 --- a/Tests/PINDiskCache+PINCacheTests.h +++ b/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 diff --git a/Tests/PINDiskCache+PINCacheTests.m b/Tests/PINDiskCache+PINCacheTests.m index 01db890c..3535d43b 100644 --- a/Tests/PINDiskCache+PINCacheTests.m +++ b/Tests/PINDiskCache+PINCacheTests.m @@ -5,6 +5,7 @@ @interface PINDiskCache () { } - (void)lock; +- (void)lockAndWaitForKnownState; - (void)unlock; @end @@ -18,4 +19,10 @@ - (void)setTtlCacheSync:(BOOL)ttlCache [self unlock]; } +- (void)waitForKnownState +{ + [self lockAndWaitForKnownState]; + [self unlock]; +} + @end From 26215c5db3db400973c00afdf8d6689586da6335 Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Fri, 8 Jun 2018 10:04:30 -0700 Subject: [PATCH 6/6] Set internal modification immediately. --- Source/PINDiskCache.m | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/Source/PINDiskCache.m b/Source/PINDiskCache.m index 1ae1f5e1..ba475845 100644 --- a/Source/PINDiskCache.m +++ b/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; } @@ -1070,8 +1063,10 @@ - (id)objectForKeyedSubscript:(NSString *)key } [self lock]; } - if (object) + if (object) { + _metadata[key].lastModifiedDate = now; [self asynchronouslySetFileModificationDate:now forURL:fileURL]; + } } [self unlock]; @@ -1101,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 {