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

[PINDiskCache] Respect small byteLimit settings by checking object size in setObject: #198

Merged
merged 4 commits into from
Sep 21, 2017

Conversation

appleguy
Copy link
Contributor

This serves as a mechanism for some clients, like users of the Texture framework, to
effectively disable the PINDiskCache by setting a byteLimit of 1.

Before this change, the cache would transiently exceed the byte limit by writing
the file anyway, and then immediately deleting it.

This change will not affect most users of PINDiskCache, but is important for this
specific use case.

…ze in setObject:

This serves as a mechanism for some clients, like users of the Texture framework, to
effectively disable the PINDiskCache by setting a byteLimit of 1.

Before this change, the cache would transiently exceed the byte limit by writing
the file anyway, and then immediately deleting it.

This change will not affect most users of PINDiskCache, but is important for this
specific use case.
@@ -838,7 +838,7 @@ - (id)objectForKeyedSubscript:(NSString *)key
{
NSDate *now = [[NSDate alloc] init];

if (!key)
if (!key || (_dates.count == 0 && _sizes.count == 0))
Copy link
Collaborator

@garrettmoon garrettmoon Sep 20, 2017

Choose a reason for hiding this comment

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

You need a lock here both because this is not thread-safe (at least not formally) and because the disk cache may not have been initialized yet which the lockWhenCondition ensures.

@@ -877,7 +877,7 @@ - (void)testAsyncDiskInitialization

testCache = [[PINDiskCache alloc] initWithName:cacheName];
//This should not return until *after* disk cache directory has been created
[testCache objectForKey:@"some bogus key"];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This actually shouldn't be changed, it failed because the optimization above, without the lock, isn't valid. I think it would cause problems for systems requesting objects early in their lifecycle which might be critical to pull from the cache.

@appleguy
Copy link
Contributor Author

@garrettmoon thanks for the feedback, I think I understand now! I'm happy to change this further if you see other issues.

Copy link
Collaborator

@garrettmoon garrettmoon left a comment

Choose a reason for hiding this comment

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

Awesome, good optimization for small disk caches (and a convenient way to disable the disk cache).

@garrettmoon garrettmoon merged commit c8a0d67 into pinterest:master Sep 21, 2017
@appleguy appleguy deleted the 1ByteLimit branch September 21, 2017 21:00
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