From 0ed7ddecd781364c8e4eae8f7e21e8b65ef32a05 Mon Sep 17 00:00:00 2001 From: olly Date: Wed, 2 Jan 2019 16:21:09 +0000 Subject: [PATCH] Reduce the number of cache files - Increase the default cache file size to 5MB - Recommended a minimum cache file size of 2MB to discourage applications from specifying values small enough such that unreasonably large numbers of cache files are generated - Allow maxCacheFileSize=C.LENGTH_UNSET, equivalent to setting it to MAX_VALUE. This is just for API consistency with other APIs we have that accept LENGTH_UNSET Issue: #4253 PiperOrigin-RevId: 227524233 --- .../offline/DownloaderConstructorHelper.java | 3 +- .../upstream/cache/CacheDataSink.java | 41 +++++++++++++++---- .../upstream/cache/CacheDataSinkFactory.java | 1 - .../upstream/cache/CacheDataSource.java | 30 +------------- .../cache/CacheDataSourceFactory.java | 11 +---- .../upstream/cache/CacheUtilTest.java | 9 +++- 6 files changed, 45 insertions(+), 50 deletions(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloaderConstructorHelper.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloaderConstructorHelper.java index 74b918c06d3..100e1a03fe6 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloaderConstructorHelper.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloaderConstructorHelper.java @@ -23,6 +23,7 @@ import com.google.android.exoplayer2.upstream.FileDataSourceFactory; import com.google.android.exoplayer2.upstream.PriorityDataSourceFactory; import com.google.android.exoplayer2.upstream.cache.Cache; +import com.google.android.exoplayer2.upstream.cache.CacheDataSink; import com.google.android.exoplayer2.upstream.cache.CacheDataSinkFactory; import com.google.android.exoplayer2.upstream.cache.CacheDataSource; import com.google.android.exoplayer2.upstream.cache.CacheDataSourceFactory; @@ -111,7 +112,7 @@ public DownloaderConstructorHelper( DataSink.Factory writeDataSinkFactory = cacheWriteDataSinkFactory != null ? cacheWriteDataSinkFactory - : new CacheDataSinkFactory(cache, CacheDataSource.DEFAULT_MAX_CACHE_FILE_SIZE); + : new CacheDataSinkFactory(cache, CacheDataSink.DEFAULT_MAX_CACHE_FILE_SIZE); onlineCacheDataSourceFactory = new CacheDataSourceFactory( cache, diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSink.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSink.java index 3c33081b162..ccec73ab359 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSink.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSink.java @@ -20,6 +20,7 @@ import com.google.android.exoplayer2.upstream.DataSpec; import com.google.android.exoplayer2.upstream.cache.Cache.CacheException; import com.google.android.exoplayer2.util.Assertions; +import com.google.android.exoplayer2.util.Log; import com.google.android.exoplayer2.util.ReusableBufferedOutputStream; import com.google.android.exoplayer2.util.Util; import java.io.File; @@ -36,8 +37,13 @@ */ public final class CacheDataSink implements DataSink { + /** Default {@code maxCacheFileSize} recommended for caching use cases. */ + public static final long DEFAULT_MAX_CACHE_FILE_SIZE = 5 * 1024 * 1024; /** Default buffer size in bytes. */ - public static final int DEFAULT_BUFFER_SIZE = 20480; + public static final int DEFAULT_BUFFER_SIZE = 20 * 1024; + + private static final long MIN_RECOMMENDED_MAX_CACHE_FILE_SIZE = 2 * 1024 * 1024; + private static final String TAG = "CacheDataSink"; private final Cache cache; private final long maxCacheFileSize; @@ -64,12 +70,15 @@ public CacheDataSinkException(IOException cause) { } /** - * Constructs a CacheDataSink using the {@link #DEFAULT_BUFFER_SIZE}. + * Constructs an instance using {@link #DEFAULT_BUFFER_SIZE}. * * @param cache The cache into which data should be written. - * @param maxCacheFileSize The maximum size of a cache file, in bytes. If the sink is opened for a - * {@link DataSpec} whose size exceeds this value, then the data will be fragmented into - * multiple cache files. + * @param maxCacheFileSize The maximum size of a cache file, in bytes. If a request results in + * data being written whose size exceeds this value, then the data will be fragmented into + * multiple cache files. If set to {@link C#LENGTH_UNSET} then no fragmentation will occur. + * Using a small value allows for finer-grained cache eviction policies, at the cost of + * increased overhead both on the cache implementation and the file system. Values under + * {@code (2 * 1024 * 1024)} are not recommended. */ public CacheDataSink(Cache cache, long maxCacheFileSize) { this(cache, maxCacheFileSize, DEFAULT_BUFFER_SIZE); @@ -77,15 +86,29 @@ public CacheDataSink(Cache cache, long maxCacheFileSize) { /** * @param cache The cache into which data should be written. - * @param maxCacheFileSize The maximum size of a cache file, in bytes. If the sink is opened for a - * {@link DataSpec} whose size exceeds this value, then the data will be fragmented into - * multiple cache files. + * @param maxCacheFileSize The maximum size of a cache file, in bytes. If a request results in + * data being written whose size exceeds this value, then the data will be fragmented into + * multiple cache files. If set to {@link C#LENGTH_UNSET} then no fragmentation will occur. + * Using a small value allows for finer-grained cache eviction policies, at the cost of + * increased overhead both on the cache implementation and the file system. Values under + * {@code (2 * 1024 * 1024)} are not recommended. * @param bufferSize The buffer size in bytes for writing to a cache file. A zero or negative * value disables buffering. */ public CacheDataSink(Cache cache, long maxCacheFileSize, int bufferSize) { + Assertions.checkState( + maxCacheFileSize > 0 || maxCacheFileSize == C.LENGTH_UNSET, + "maxCacheFileSize must be positive or C.LENGTH_UNSET."); + if (maxCacheFileSize != C.LENGTH_UNSET + && maxCacheFileSize < MIN_RECOMMENDED_MAX_CACHE_FILE_SIZE) { + Log.w( + TAG, + "maxCacheFileSize is below the minimum recommended value of " + + MIN_RECOMMENDED_MAX_CACHE_FILE_SIZE + + ". This may cause poor cache performance."); + } this.cache = Assertions.checkNotNull(cache); - this.maxCacheFileSize = maxCacheFileSize; + this.maxCacheFileSize = maxCacheFileSize == C.LENGTH_UNSET ? Long.MAX_VALUE : maxCacheFileSize; this.bufferSize = bufferSize; syncFileDescriptor = true; } diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSinkFactory.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSinkFactory.java index 0b9ab665087..6dcb14f5fb9 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSinkFactory.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSinkFactory.java @@ -46,5 +46,4 @@ public CacheDataSinkFactory(Cache cache, long maxCacheFileSize, int bufferSize) public DataSink createDataSink() { return new CacheDataSink(cache, maxCacheFileSize, bufferSize); } - } diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSource.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSource.java index 1b4b28d67e7..579f5d05e99 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSource.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSource.java @@ -45,14 +45,6 @@ */ public final class CacheDataSource implements DataSource { - /** - * Default maximum single cache file size. - * - * @see #CacheDataSource(Cache, DataSource, int) - * @see #CacheDataSource(Cache, DataSource, int, long) - */ - public static final long DEFAULT_MAX_CACHE_FILE_SIZE = 2 * 1024 * 1024; - /** * Flags controlling the CacheDataSource's behavior. Possible flag values are {@link * #FLAG_BLOCK_ON_CACHE}, {@link #FLAG_IGNORE_CACHE_ON_ERROR} and {@link @@ -163,7 +155,7 @@ public interface EventListener { * @param upstream A {@link DataSource} for reading data not in the cache. */ public CacheDataSource(Cache cache, DataSource upstream) { - this(cache, upstream, 0, DEFAULT_MAX_CACHE_FILE_SIZE); + this(cache, upstream, /* flags= */ 0); } /** @@ -176,29 +168,11 @@ public CacheDataSource(Cache cache, DataSource upstream) { * and {@link #FLAG_IGNORE_CACHE_FOR_UNSET_LENGTH_REQUESTS}, or 0. */ public CacheDataSource(Cache cache, DataSource upstream, @Flags int flags) { - this(cache, upstream, flags, DEFAULT_MAX_CACHE_FILE_SIZE); - } - - /** - * Constructs an instance with default {@link DataSource} and {@link DataSink} instances for - * reading and writing the cache. The sink is configured to fragment data such that no single - * cache file is greater than maxCacheFileSize bytes. - * - * @param cache The cache. - * @param upstream A {@link DataSource} for reading data not in the cache. - * @param flags A combination of {@link #FLAG_BLOCK_ON_CACHE}, {@link #FLAG_IGNORE_CACHE_ON_ERROR} - * and {@link #FLAG_IGNORE_CACHE_FOR_UNSET_LENGTH_REQUESTS}, or 0. - * @param maxCacheFileSize The maximum size of a cache file, in bytes. If the cached data size - * exceeds this value, then the data will be fragmented into multiple cache files. The - * finer-grained this is the finer-grained the eviction policy can be. - */ - public CacheDataSource(Cache cache, DataSource upstream, @Flags int flags, - long maxCacheFileSize) { this( cache, upstream, new FileDataSource(), - new CacheDataSink(cache, maxCacheFileSize), + new CacheDataSink(cache, CacheDataSink.DEFAULT_MAX_CACHE_FILE_SIZE), flags, /* eventListener= */ null); } diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceFactory.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceFactory.java index d9f9470fe39..e25c3d7a4a4 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceFactory.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceFactory.java @@ -46,20 +46,11 @@ public CacheDataSourceFactory(Cache cache, DataSource.Factory upstreamFactory) { /** @see CacheDataSource#CacheDataSource(Cache, DataSource, int) */ public CacheDataSourceFactory( Cache cache, DataSource.Factory upstreamFactory, @CacheDataSource.Flags int flags) { - this(cache, upstreamFactory, flags, CacheDataSource.DEFAULT_MAX_CACHE_FILE_SIZE); - } - - /** @see CacheDataSource#CacheDataSource(Cache, DataSource, int, long) */ - public CacheDataSourceFactory( - Cache cache, - DataSource.Factory upstreamFactory, - @CacheDataSource.Flags int flags, - long maxCacheFileSize) { this( cache, upstreamFactory, new FileDataSourceFactory(), - new CacheDataSinkFactory(cache, maxCacheFileSize), + new CacheDataSinkFactory(cache, CacheDataSink.DEFAULT_MAX_CACHE_FILE_SIZE), flags, /* eventListener= */ null); } diff --git a/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheUtilTest.java b/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheUtilTest.java index 4fbe93888e4..d1bf734a98d 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheUtilTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheUtilTest.java @@ -27,6 +27,7 @@ import com.google.android.exoplayer2.testutil.FakeDataSource; import com.google.android.exoplayer2.testutil.TestUtil; import com.google.android.exoplayer2.upstream.DataSpec; +import com.google.android.exoplayer2.upstream.FileDataSource; import com.google.android.exoplayer2.upstream.cache.CacheUtil.CachingCounters; import com.google.android.exoplayer2.util.Util; import java.io.EOFException; @@ -341,7 +342,13 @@ public void testRemove() throws Exception { cache, /* cacheKeyFactory= */ null, // Set maxCacheFileSize to 10 to make sure there are multiple spans. - new CacheDataSource(cache, dataSource, 0, 10), + new CacheDataSource( + cache, + dataSource, + new FileDataSource(), + new CacheDataSink(cache, /* maxCacheFileSize= */ 10), + /* flags= */ 0, + /* eventListener= */ null), new byte[CacheUtil.DEFAULT_BUFFER_SIZE_BYTES], /* priorityTaskManager= */ null, /* priority= */ 0,