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

Tweaks to garbage collection #90

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

joelekstrom
Copy link

  • Expose enqueueGarbageCollection
    Scheduling garbage collection by time isn't appropriate for short-lived applications. This adds an alternative API which allows you to schedule the garbage collection instantly. This fixes Scheduling of garbage collection #89

  • Expose minimumFreeDiskSpaceFraction
    Defaulting to require 10% of free disk space does not make sense for some apps. For example, on a huge disk, say 64GB, there is no reason to require 6GB free space for a 30MB cache.

@codecov-io
Copy link

codecov-io commented Mar 14, 2017

Codecov Report

Merging #90 into master will decrease coverage by 0.2%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #90      +/-   ##
==========================================
- Coverage   99.68%   99.48%   -0.21%     
==========================================
  Files          14       14              
  Lines         955     1166     +211     
==========================================
+ Hits          952     1160     +208     
- Misses          3        6       +3
Impacted Files Coverage Δ
Sources/SPTPersistentCache.m 99.44% <0%> (-0.08%) ⬇️
Sources/SPTPersistentCacheFileManager.m 100% <100%> (ø) ⬆️
Sources/SPTPersistentCacheOptions.m 100% <100%> (ø) ⬆️
Sources/SPTPersistentCacheGarbageCollector.m 97.26% <33.33%> (-2.74%) ⬇️
...ategories/NSError+SPTPersistentCacheDomainErrors.m 100% <0%> (ø) ⬆️
Sources/SPTPersistentCacheDebugUtilities.m 100% <0%> (ø) ⬆️
Sources/SPTPersistentCacheHeader.m 100% <0%> (ø) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eeef171...3660d47. Read the comment docs.

@8W9aG
Copy link
Contributor

8W9aG commented Mar 14, 2017

Would it be possible to add some unit tests? Otherwise I like this idea

Scheduling garbage collection by time isn't appropriate for
short-lived applications. This adds an alternative API which allows
you to schedule the garbage collection instantly.
Defaulting to require 10% of free disk space does not make sense for
some apps. For example, on a huge disk, say 64GB, there is no reason
to require 6GB free space for a 30MB cache.
@joelekstrom
Copy link
Author

joelekstrom commented Mar 15, 2017

Tests added! I have some trouble reproducing the failed tests from travis - they are in some unchanged tests (but may be related to me changing the default disk cache size). However, they seem kind of implementation-dependent since I tried running the tests on all similar simulators on my computer. 🤔

It is probably because the code in - (SPTPersistentCacheDiskSize)optimizedDiskSizeForCacheSize:(SPTPersistentCacheDiskSize)currentCacheSize in SPTPersistentCacheFileManager.m. If you look at line 178, it adds the current cache size to the free space of the disk. Now, if it is a large disk that is almost empty, this will result in an overflow, which should result in the tests failing like they do now.

This bug seems to have existed before, but appeared in tests now that I changed the disk sizes during testing. Suggestions for a solution?

@@ -492,21 +492,35 @@
isa = XCBuildConfiguration;
baseConfigurationReference = 9C882A801C65422700D463BB /* project.xcconfig */;
buildSettings = {
CLANG_WARN_BLOCK_CAPTURE_AUTORELEASING = YES;
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to put all of these in our xcconfig file. To keep the Xcode project clean and make it easier to maintain. See https://github.com/spotify/SPTPersistentCache/blob/master/ci/spotify_os.xcconfig

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.

Scheduling of garbage collection
6 participants