Significantly improves startup performance by asynchronously building… #203

garrettmoon · 2017-10-02T01:07:46Z

… known state on startup.

appleguy · 2017-10-02T04:18:24Z

Excited for this PR :). Let me know when it's ready for a full review — I'll try to look in depth within a day or two.

appleguy

This is a big advancement for PINCache! I'm very excited to integrate this in my team's app.

I think the most important feedback in my comments relates to the spinlock / sleep behavior. I thought about it for a bit and think the suggestion should work pretty well!

appleguy · 2017-10-03T03:59:10Z

Source/PINDiskCache.m

    PINDiskCacheSerializerBlock _serializer;
    PINDiskCacheDeserializerBlock _deserializer;

    PINDiskCacheKeyEncoderBlock _keyEncoder;
    PINDiskCacheKeyDecoderBlock _keyDecoder;
 }

+@property (assign, nonatomic) pthread_mutex_t mutex;


Nice, this should be a solid improvement!

appleguy · 2017-10-03T04:03:37Z

Source/PINDiskCache.m

-        if (date && key)
-            [_dates setObject:date forKey:key];
+        // Do not continue to hold the lock while processing files.
+        [self lock];


The comment doesn't seem to match the code - should this be re-locking or unlocking?

Suggestion that would avoid the many lock / unlock cycles:

Build up a separate NSMutableDictionary instance (easier if merged into a shared _cacheItemMetadata, but can be done with each _sizes, _dates and _knownKeys)

No locking within the loop

At the end of the loop, lock and merge into the new instances any keys and values that have been set on the _sizes / _dates / _knownKeys while loading from disk has been going on.

Overwrite the instance variables with the new collections.

This would be the only way to avoid contention in the common case of a busy cache (e.g. many images loading at startup) while loading a large disk cache (e.g. a 200MB+ cache of 10-100KB images).

Ah, the comment is supposed to mean, do not hold the lock the entire time while processing files.

The real issue around this locking is not the in memory state of ivars but disk access. PINDiskCache manages access to the disk solely through the lock. Without it, we can't prevent another operation from overwriting our file while we're trying to get its attributes.

In the future future, I really want to investigate using NSFileCoordinators instead of locking to manage disk access.

Very interesting! I wonder if we should use a separate lock for disk access and for protecting the shared state? There are probably a bunch of methods that don't need to block on file access...

I think this explains why I've seen more lock contention in this layer than I would expect from just protecting the shared state. The good news is that, unlike some hierarchical objects like nodes where multiple locks create deadlocks extremely easily, this class might be ideally structured for a clean multi-lock strategy.

appleguy · 2017-10-03T04:07:42Z

Source/PINDiskCache.m

+            _byteCount = byteCount;
+
+        if (self->_byteLimit > 0 && self->_byteCount > self->_byteLimit)
+            [self trimToSizeByDateAsync:self->_byteLimit completion:nil];


It looks like we may need a way to avoid scheduling a large number of trim operations while this is processing. It probably makes sense to have a _trimPending / scheduled variable, so that a series of setObject: calls during this time doesn't schedule a lot of them.

Since the operation queue is concurrent, it looks like there may be an issue with multiple trim operations starting in parallel too — this could be addressed by using a barrier block on the underlying queue, which GCD will ensure is run serially.

@appleguy the trim's are actually coalesced thanks to @nguyenhuy's improvement in PINOperationQueue! We could also skip trimming until we have a known state.

appleguy · 2017-10-03T04:09:41Z

Source/PINDiskCache.m

@@ -974,7 +1005,7 @@ - (void)setObject:(id <NSCoding>)object forKey:(NSString *)key fileURL:(NSURL **
        return;
    }

-    [self lock];
+    [self lockUntilWritable];


Is it necessary to block until the setup is done? If this happens, then won't the common path that triggers cache init (e.g. an image download starting from PINRemoteImage) will still block displaying the image until after the cache is fully read in?

I may be missing something, like perhaps PINRemoteImage performs the setObject: asynchronously and we won't block.

lockUntilWritable only waits until the cache directory is confirmed to exist or is created if it doesn't. lockUntilKnownState (I think that's what I called it) guarantees everything is created, iterated and populated.

appleguy · 2017-10-03T04:10:49Z

Source/PINDiskCache.m

+        if (self->_ttlCache) {
+            // We actually need to know the entire disk state if we're a TTL cache.
+            [self unlock];
+            [self lockUntilDiskStateKnown];


Why is that? Can't we just return nil for any objectForKey: before initialization is done, including for a TTL cache?

I think I recall there being a default TTL of 30 days. We should consider turning this off by default if it incurs a meaningful performance penalty, which blocking on cache init would be a pretty big cost if the developer hasn't specifically indicated they want TTL behavior.

TTL is off by default. It was a feature added by a community member which enforces ttl (I.e. it won't return an object even if it has it if it exceeds the TTL). There are separate time limits are probably the default you're think of but they only relate to trimming, not a guarantee something won't be returned. To be frank I'm a bit regretful we merged in this feature, it's not really in the spirit of the framework and adds a lot of complexity.

appleguy · 2017-10-03T04:13:33Z

Source/PINDiskCache.m

+        [self unlock];
+        usleep(100);
+
+        __unused int result = pthread_mutex_lock(&_mutex);


Why not call [self lock] here and in the method below?

Good point.

appleguy · 2017-10-03T04:16:57Z

Source/PINDiskCache.m

+    // spinlock if the disk isn't writable
+    while (_diskWritable == NO) {
+        [self unlock];
+        usleep(100);


Using this technique could cause some other problems, like a lot of threads to be spawned. We should be able to rely on the lock to be released in order to let the threads proceed.

How about a separate read-write mutex, just for cache init? At initialization of the class, the writer lock would be acquired by the code doing the read from disk. Any code calling -lockUntilWritable would first acquire the read lock (which could be grabbed by many threads at the same time, as long as the write lock isn't held). Secondly it would acquire the main [self lock].

This should allow all of them to wait on the initialization, without busying the main cache lock for operations that don't depend on _diskWritable.

As an alternative, I think there is a pthread version of the condition lock or a semaphore which can be signalled.

Interesting, I'll play around with this.

I think the pthread condition might be the best option. I'm guessing performance isn't any better for read / write locks and it's far less indicative of what we're doing. Need to actually figure out how to do pthread conditions though, not a lot of great docs…

…ge on an iPhone 6. Essentially just adds an in memory map of what is known to be on disk.

…@appleguy

…@appleguy :)

… known state on startup.

Adlai-Holler

This is super duper good. Async all day!

Adlai-Holler · 2017-10-04T21:19:06Z

Source/PINDiskCache.m

+                                                                 attributes:nil
+                                                                      error:&error];
+        PINDiskCacheError(error);
+        created = success;


Nit: Let's assign directly to created and remove success

Adlai-Holler · 2017-10-04T21:26:50Z

Source/PINDiskCache.m

+    if (_diskStateKnown == NO) {
+        pthread_cond_wait(&_diskStateKnownCondition, &_mutex);
+    }
+}


Two alternate names might be lockForWriting and lockAndWaitForKnownDiskState or something. lockUntil isn't quite on the money.

I like those better.

Adlai-Holler · 2017-10-04T21:31:42Z

Source/PINDiskCache.m

+                                                       includingPropertiesForKeys:keys
+                                                                          options:NSDirectoryEnumerationSkipsHiddenFiles
+                                                                            error:&error];
+    [self unlock];


What's the motivation behind this lock? cacheURL is immutable and I think it's safe to use NSFileManager from multiple threads.

You're right that it's not strictly necessary and I think we should likely audit this behavior to further improve performance. However in it's current state, all access to the filesystem is done with the lock held and I want to keep that behavior intact until we decide on a cohesive replacement strategy.

Adlai-Holler · 2017-10-04T21:35:06Z

Source/PINDiskCache.m

@@ -859,17 +895,22 @@ - (id)objectForKeyedSubscript:(NSString *)key
    NSDate *now = [[NSDate alloc] init];


Unrelated to this diff, but I just noticed we can save some work by moving this date creation down.

Adlai-Holler · 2017-10-04T21:36:40Z

Source/PINDiskCache.m

@@ -830,7 +866,7 @@ - (void)enumerateObjectsWithBlockAsync:(PINDiskCacheFileURLEnumerationBlock)bloc
 - (void)synchronouslyLockFileAccessWhileExecutingBlock:(PINCacheBlock)block
 {
    if (block) {
-        [self lock];
+        [self lockUntilWritable];
            block(self);
        [self unlock];
    }


Should we also modify containsObjectForKey:?

Good catch!

Adlai-Holler · 2017-10-04T21:40:55Z

Source/PINDiskCache.m

+- (void)dealloc
+{
+    __unused int result = pthread_mutex_destroy(&_mutex);
+    NSCAssert(result == 0, @"Failed to destroy lock in PINMemoryCache %p. Code: %d", (void *)self, result);


Add pthread_cond_destroy here

I knew there had to be something like that (but couldn't find docs)

Adlai-Holler

Bingo!

appleguy reviewed Oct 3, 2017

View reviewed changes

garrettmoon force-pushed the improveStartupPerformance branch from cdf4bd2 to c673afc Compare October 4, 2017 02:25

garrettmoon requested review from maicki and nguyenhuy October 4, 2017 20:11

garrettmoon changed the base branch from improveCacheMissPerformance to master October 4, 2017 20:49

garrettmoon added 6 commits October 4, 2017 13:51

Improves cache miss on disk cache by two orders of magnetude on avera…

2c64c76

…ge on an iPhone 6. Essentially just adds an in memory map of what is known to be on disk.

bundle up metadata as opposed to having two dicts. Thanks for the idea …

acd16d1

…@appleguy :)

Significantly improves startup performance by asynchronously building…

6567d9c

… known state on startup.

I knew writing tests was a good idea.

7fd13d7

Add entry to CHANGELOG

cdb981b

Use a condition lock instead. Thanks @appleguy!

f2ac652

garrettmoon force-pushed the improveStartupPerformance branch from 46704b2 to f2ac652 Compare October 4, 2017 20:53

Adlai-Holler reviewed Oct 4, 2017

View reviewed changes

Lot's of little fixes. Thanks @Adlai-Holler!

412476b

Adlai-Holler approved these changes Oct 4, 2017

View reviewed changes

garrettmoon merged commit 3d80c8e into master Oct 4, 2017

garrettmoon mentioned this pull request Oct 6, 2017

[Performance] -initializeDiskProperties can take >1.5s on startup. #144

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Significantly improves startup performance by asynchronously building… #203

Significantly improves startup performance by asynchronously building… #203

garrettmoon commented Oct 2, 2017

appleguy commented Oct 2, 2017

appleguy left a comment

appleguy Oct 3, 2017

appleguy Oct 3, 2017 •

edited

Loading

garrettmoon Oct 3, 2017

appleguy Oct 4, 2017

appleguy Oct 3, 2017

garrettmoon Oct 3, 2017 •

edited

Loading

appleguy Oct 3, 2017

garrettmoon Oct 3, 2017

appleguy Oct 3, 2017

garrettmoon Oct 3, 2017

appleguy Oct 3, 2017

garrettmoon Oct 3, 2017

appleguy Oct 3, 2017

garrettmoon Oct 3, 2017

garrettmoon Oct 4, 2017

Adlai-Holler left a comment

Adlai-Holler Oct 4, 2017

Adlai-Holler Oct 4, 2017

garrettmoon Oct 4, 2017

Adlai-Holler Oct 4, 2017

garrettmoon Oct 4, 2017

Adlai-Holler Oct 4, 2017

Adlai-Holler Oct 4, 2017

garrettmoon Oct 4, 2017

Adlai-Holler Oct 4, 2017

garrettmoon Oct 4, 2017

Adlai-Holler left a comment

		@@ -859,17 +895,22 @@ - (id)objectForKeyedSubscript:(NSString *)key
		NSDate *now = [[NSDate alloc] init];

Significantly improves startup performance by asynchronously building… #203

Significantly improves startup performance by asynchronously building… #203

Conversation

garrettmoon commented Oct 2, 2017

appleguy commented Oct 2, 2017

appleguy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appleguy Oct 3, 2017 • edited Loading

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 Oct 3, 2017 • edited Loading

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

Adlai-Holler 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

Adlai-Holler left a comment

Choose a reason for hiding this comment

appleguy Oct 3, 2017 •

edited

Loading

garrettmoon Oct 3, 2017 •

edited

Loading