From 2f365e5a4b1721c43edc03fe9071008d3e81aaea Mon Sep 17 00:00:00 2001 From: olly Date: Mon, 10 Dec 2018 20:07:42 +0000 Subject: [PATCH] Don't write CachedContentIndex to disk on key removal Issue: #5136 PiperOrigin-RevId: 224857629 --- RELEASENOTES.md | 12 ++++-- .../exoplayer2/upstream/cache/Cache.java | 2 +- .../upstream/cache/CachedContentIndex.java | 39 ++++++++++++++++++- .../upstream/cache/SimpleCache.java | 34 +++++++--------- .../upstream/cache/CacheDataSourceTest.java | 6 +-- .../upstream/cache/SimpleCacheTest.java | 35 +++++++++++++++++ 6 files changed, 97 insertions(+), 31 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index ad2956cb4d..337ff54ee8 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -12,9 +12,15 @@ selections together. * Do not retry failed loads whose error is `FileNotFoundException`. * Prevent Cea608Decoder from generating Subtitles with null Cues list. -* Caching: Cache data with unknown length by default. The previous flag to opt - in to this behavior (`DataSpec.FLAG_ALLOW_CACHING_UNKNOWN_LENGTH`) has been - replaced with an opt out flag (`DataSpec.FLAG_DONT_CACHE_IF_LENGTH_UNKNOWN`). +* Offline: + * Speed up removal of segmented downloads + ([#5136](https://github.com/google/ExoPlayer/issues/5136)). +* Caching: + * Improve performance of `SimpleCache`. + * Cache data with unknown length by default. The previous flag to opt in to + this behavior (`DataSpec.FLAG_ALLOW_CACHING_UNKNOWN_LENGTH`) has been + replaced with an opt out flag + (`DataSpec.FLAG_DONT_CACHE_IF_LENGTH_UNKNOWN`). * Disable post processing on Nvidia devices, as it breaks decode-only frame skippping. diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/Cache.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/Cache.java index a769e9acac..b5b5dc64e6 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/Cache.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/Cache.java @@ -82,7 +82,7 @@ public interface Cache { * Releases the cache. This method must be called when the cache is no longer required. The cache * must not be used after calling this method. */ - void release() throws CacheException; + void release(); /** * Registers a listener to listen for changes to a given key. diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CachedContentIndex.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CachedContentIndex.java index 19160c73d4..a744917230 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CachedContentIndex.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CachedContentIndex.java @@ -17,6 +17,7 @@ package com.google.android.exoplayer2.upstream.cache; import android.support.annotation.VisibleForTesting; import android.util.SparseArray; +import android.util.SparseBooleanArray; import com.google.android.exoplayer2.upstream.cache.Cache.CacheException; import com.google.android.exoplayer2.util.Assertions; import com.google.android.exoplayer2.util.AtomicFile; @@ -42,6 +43,7 @@ import javax.crypto.CipherOutputStream; import javax.crypto.NoSuchPaddingException; import javax.crypto.spec.IvParameterSpec; import javax.crypto.spec.SecretKeySpec; +import org.checkerframework.checker.nullness.compatqual.NullableType; /** Maintains the index of cached content. */ /* package */ class CachedContentIndex { @@ -53,7 +55,30 @@ import javax.crypto.spec.SecretKeySpec; private static final int FLAG_ENCRYPTED_INDEX = 1; private final HashMap keyToContent; - private final SparseArray idToKey; + /** + * Maps assigned ids to their corresponding keys. Also contains (id -> null) entries for ids that + * have been removed from the index since it was last stored. This prevents reuse of these ids, + * which is necessary to avoid clashes that could otherwise occur as a result of the sequence: + * + *

[1] (key1, id1) is removed from the in-memory index ... the index is not stored to disk ... + * [2] id1 is reused for a different key2 ... the index is not stored to disk ... [3] A file for + * key2 is partially written using a path corresponding to id1 ... the process is killed before + * the index is stored to disk ... [4] The index is read from disk, causing the partially written + * file to be incorrectly associated to key1 + * + *

By avoiding id reuse in step [2], a new id2 will be used instead. Step [4] will then delete + * the partially written file because the index does not contain an entry for id2. + * + *

When the index is next stored (id -> null) entries are removed, making the ids eligible for + * reuse. + */ + private final SparseArray<@NullableType String> idToKey; + /** + * Tracks ids for which (id -> null) entries are present in idToKey, so that they can be removed + * efficiently when the index is next stored. + */ + private final SparseBooleanArray removedIds; + private final AtomicFile atomicFile; private final Cipher cipher; private final SecretKeySpec secretKeySpec; @@ -105,6 +130,7 @@ import javax.crypto.spec.SecretKeySpec; } keyToContent = new HashMap<>(); idToKey = new SparseArray<>(); + removedIds = new SparseBooleanArray(); atomicFile = new AtomicFile(new File(cacheDir, FILE_NAME)); } @@ -125,6 +151,12 @@ import javax.crypto.spec.SecretKeySpec; } writeFile(); changed = false; + // Make ids that were removed since the index was last stored eligible for re-use. + int removedIdCount = removedIds.size(); + for (int i = 0; i < removedIdCount; i++) { + idToKey.remove(removedIds.keyAt(i)); + } + removedIds.clear(); } /** @@ -169,8 +201,11 @@ import javax.crypto.spec.SecretKeySpec; CachedContent cachedContent = keyToContent.get(key); if (cachedContent != null && cachedContent.isEmpty() && !cachedContent.isLocked()) { keyToContent.remove(key); - idToKey.remove(cachedContent.id); changed = true; + // Keep an entry in idToKey to stop the id from being reused until the index is next stored. + idToKey.put(cachedContent.id, /* value= */ null); + // Track that the entry should be removed from idToKey when the index is next stored. + removedIds.put(cachedContent.id, /* value= */ true); } } diff --git a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/SimpleCache.java b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/SimpleCache.java index 1fd6dc63bc..a3ec3b1f05 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/SimpleCache.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/SimpleCache.java @@ -146,13 +146,16 @@ public final class SimpleCache implements Cache { } @Override - public synchronized void release() throws CacheException { + public synchronized void release() { if (released) { return; } listeners.clear(); + removeStaleSpans(); try { - removeStaleSpansAndCachedContents(); + index.store(); + } catch (CacheException e) { + Log.e(TAG, "Storing index file failed", e); } finally { unlockFolder(cacheDir); released = true; @@ -265,7 +268,7 @@ public final class SimpleCache implements Cache { if (!cacheDir.exists()) { // For some reason the cache directory doesn't exist. Make a best effort to create it. cacheDir.mkdirs(); - removeStaleSpansAndCachedContents(); + removeStaleSpans(); } evictor.onStartFile(this, key, position, maxLength); return SimpleCacheSpan.getCacheFile( @@ -311,9 +314,9 @@ public final class SimpleCache implements Cache { } @Override - public synchronized void removeSpan(CacheSpan span) throws CacheException { + public synchronized void removeSpan(CacheSpan span) { Assertions.checkState(!released); - removeSpan(span, true); + removeSpanInternal(span); } @Override @@ -379,7 +382,7 @@ public final class SimpleCache implements Cache { if (span.isCached && !span.file.exists()) { // The file has been deleted from under us. It's likely that other files will have been // deleted too, so scan the whole in-memory representation. - removeStaleSpansAndCachedContents(); + removeStaleSpans(); continue; } return span; @@ -431,27 +434,21 @@ public final class SimpleCache implements Cache { notifySpanAdded(span); } - private void removeSpan(CacheSpan span, boolean removeEmptyCachedContent) throws CacheException { + private void removeSpanInternal(CacheSpan span) { CachedContent cachedContent = index.get(span.key); if (cachedContent == null || !cachedContent.removeSpan(span)) { return; } totalSpace -= span.length; - try { - if (removeEmptyCachedContent) { - index.maybeRemove(cachedContent.key); - index.store(); - } - } finally { - notifySpanRemoved(span); - } + index.maybeRemove(cachedContent.key); + notifySpanRemoved(span); } /** * Scans all of the cached spans in the in-memory representation, removing any for which files no * longer exist. */ - private void removeStaleSpansAndCachedContents() throws CacheException { + private void removeStaleSpans() { ArrayList spansToBeRemoved = new ArrayList<>(); for (CachedContent cachedContent : index.getAll()) { for (CacheSpan span : cachedContent.getSpans()) { @@ -461,11 +458,8 @@ public final class SimpleCache implements Cache { } } for (int i = 0; i < spansToBeRemoved.size(); i++) { - // Remove span but not CachedContent to prevent multiple index.store() calls. - removeSpan(spansToBeRemoved.get(i), false); + removeSpanInternal(spansToBeRemoved.get(i)); } - index.removeEmpty(); - index.store(); } private void notifySpanRemoved(CacheSpan span) { diff --git a/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest.java b/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest.java index 9182074eb9..236f8f6641 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest.java @@ -467,11 +467,7 @@ public final class CacheDataSourceTest { NavigableSet cachedSpans = cache.getCachedSpans(defaultCacheKey); for (CacheSpan cachedSpan : cachedSpans) { if (cachedSpan.position >= halfDataLength) { - try { - cache.removeSpan(cachedSpan); - } catch (Cache.CacheException e) { - // do nothing - } + cache.removeSpan(cachedSpan); } } diff --git a/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/SimpleCacheTest.java b/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/SimpleCacheTest.java index a4e444386a..15bbb8c108 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/SimpleCacheTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/SimpleCacheTest.java @@ -47,6 +47,7 @@ import org.robolectric.RuntimeEnvironment; public class SimpleCacheTest { private static final String KEY_1 = "key1"; + private static final String KEY_2 = "key2"; private File cacheDir; @@ -152,6 +153,40 @@ public class SimpleCacheTest { assertCachedDataReadCorrect(cacheSpan2); } + @Test + public void testReloadCacheWithoutRelease() throws Exception { + SimpleCache simpleCache = getSimpleCache(); + + // Write data for KEY_1. + CacheSpan cacheSpan1 = simpleCache.startReadWrite(KEY_1, 0); + addCache(simpleCache, KEY_1, 0, 15); + simpleCache.releaseHoleSpan(cacheSpan1); + // Write and remove data for KEY_2. + CacheSpan cacheSpan2 = simpleCache.startReadWrite(KEY_2, 0); + addCache(simpleCache, KEY_2, 0, 15); + simpleCache.releaseHoleSpan(cacheSpan2); + simpleCache.removeSpan(simpleCache.getCachedSpans(KEY_2).first()); + + // Don't release the cache. This means the index file wont have been written to disk after the + // data for KEY_2 was removed. Move the cache instead, so we can reload it without failing the + // folder locking check. + File cacheDir2 = Util.createTempFile(RuntimeEnvironment.application, "ExoPlayerTest"); + cacheDir2.delete(); + cacheDir.renameTo(cacheDir2); + + // Reload the cache from its new location. + simpleCache = new SimpleCache(cacheDir2, new NoOpCacheEvictor()); + + // Read data back for KEY_1. + CacheSpan cacheSpan3 = simpleCache.startReadWrite(KEY_1, 0); + assertCachedDataReadCorrect(cacheSpan3); + + // Check the entry for KEY_2 was removed when the cache was reloaded. + assertThat(simpleCache.getCachedSpans(KEY_2)).isEmpty(); + + Util.recursiveDelete(cacheDir2); + } + @Test public void testEncryptedIndex() throws Exception { byte[] key = "Bar12345Bar12345".getBytes(C.UTF8_NAME); // 128 bit key