Commit 8c47b020 by olly Committed by Oliver Woodman

Reduce number of calls to File.length()

Calls to File.length() can be O(N) where N is the number of files
in the containing folder. This is believed to be true for at least
FAT32. Repeated calls for the same file tend to be faster,
presumably due to caching in the file system, however are still
surprisingly expensive. Hence minimizing the number of calls is
preferable.

Issue: #4253
PiperOrigin-RevId: 228179921
parent be69d5b7
......@@ -141,8 +141,8 @@ public interface Cache {
* obtains the data from some other source. The returned {@link CacheSpan} serves as a lock.
* Whilst the caller holds the lock it may write data into the hole. It may split data into
* multiple files. When the caller has finished writing a file it should commit it to the cache by
* calling {@link #commitFile(File)}. When the caller has finished writing, it must release the
* lock by calling {@link #releaseHoleSpan}.
* calling {@link #commitFile(File, long)}. When the caller has finished writing, it must release
* the lock by calling {@link #releaseHoleSpan}.
*
* @param key The key of the data being requested.
* @param position The position of the data being requested.
......@@ -182,9 +182,10 @@ public interface Cache {
* CacheSpan} obtained from {@link #startReadWrite(String, long)}
*
* @param file A newly written cache file.
* @param length The length of the newly written cache file in bytes.
* @throws CacheException If an error is encountered.
*/
void commitFile(File file) throws CacheException;
void commitFile(File file, long length) throws CacheException;
/**
* Releases a {@link CacheSpan} obtained from {@link #startReadWrite(String, long)} which
......
......@@ -219,7 +219,7 @@ public final class CacheDataSink implements DataSink {
File fileToCommit = file;
file = null;
if (success) {
cache.commitFile(fileToCommit);
cache.commitFile(fileToCommit, outputStreamBytesWritten);
} else {
fileToCommit.delete();
}
......
......@@ -275,26 +275,24 @@ public final class SimpleCache implements Cache {
}
@Override
public synchronized void commitFile(File file) throws CacheException {
public synchronized void commitFile(File file, long length) throws CacheException {
Assertions.checkState(!released);
SimpleCacheSpan span = SimpleCacheSpan.createCacheEntry(file, index);
Assertions.checkState(span != null);
CachedContent cachedContent = index.get(span.key);
Assertions.checkNotNull(cachedContent);
Assertions.checkState(cachedContent.isLocked());
// If the file doesn't exist, don't add it to the in-memory representation.
if (!file.exists()) {
return;
}
// If the file has length 0, delete it and don't add it to the in-memory representation.
if (file.length() == 0) {
if (length == 0) {
file.delete();
return;
}
SimpleCacheSpan span = SimpleCacheSpan.createCacheEntry(file, length, index);
Assertions.checkState(span != null);
CachedContent cachedContent = index.get(span.key);
Assertions.checkNotNull(cachedContent);
Assertions.checkState(cachedContent.isLocked());
// Check if the span conflicts with the set content length
long length = ContentMetadata.getContentLength(cachedContent.getMetadata());
if (length != C.LENGTH_UNSET) {
Assertions.checkState((span.position + span.length) <= length);
long contentLength = ContentMetadata.getContentLength(cachedContent.getMetadata());
if (contentLength != C.LENGTH_UNSET) {
Assertions.checkState((span.position + span.length) <= contentLength);
}
addSpan(span);
index.store();
......@@ -394,7 +392,7 @@ public final class SimpleCache implements Cache {
continue;
}
SimpleCacheSpan span =
file.length() > 0 ? SimpleCacheSpan.createCacheEntry(file, index) : null;
file.length() > 0 ? SimpleCacheSpan.createCacheEntry(file, file.length(), index) : null;
if (span != null) {
addSpan(span);
} else {
......
......@@ -82,16 +82,22 @@ import java.util.regex.Pattern;
return new SimpleCacheSpan(key, position, length, C.TIME_UNSET, null);
}
/*
* Note: {@code fileLength} is equivalent to {@code file.length()}, but passing it as an explicit
* argument can reduce the number of calls to this method if the calling code already knows the
* file length. This is preferable because calling {@code file.length()} can be expensive. See:
* https://github.com/google/ExoPlayer/issues/4253#issuecomment-451593889.
*/
/**
* Creates a cache span from an underlying cache file. Upgrades the file if necessary.
*
* @param file The cache file.
* @param index Cached content index.
* @param length The length of the cache file in bytes.
* @return The span, or null if the file name is not correctly formatted, or if the id is not
* present in the content index.
*/
@Nullable
public static SimpleCacheSpan createCacheEntry(File file, CachedContentIndex index) {
public static SimpleCacheSpan createCacheEntry(File file, long length, CachedContentIndex index) {
String name = file.getName();
if (!name.endsWith(SUFFIX)) {
file = upgradeFile(file, index);
......@@ -105,11 +111,12 @@ import java.util.regex.Pattern;
if (!matcher.matches()) {
return null;
}
long length = file.length();
int id = Integer.parseInt(matcher.group(1));
String key = index.getKeyForId(id);
return key == null ? null : new SimpleCacheSpan(key, Long.parseLong(matcher.group(2)), length,
Long.parseLong(matcher.group(3)), file);
return key == null
? null
: new SimpleCacheSpan(
key, Long.parseLong(matcher.group(2)), length, Long.parseLong(matcher.group(3)), file);
}
/**
......
......@@ -100,9 +100,15 @@ public class CachedContentIndexTest {
assertThat(cachedContent1.id != cachedContent2.id).isTrue();
// add a span
int cacheFileLength = 20;
File cacheSpanFile =
SimpleCacheSpanTest.createCacheSpanFile(cacheDir, cachedContent1.id, 10, 20, 30);
SimpleCacheSpan span = SimpleCacheSpan.createCacheEntry(cacheSpanFile, index);
SimpleCacheSpanTest.createCacheSpanFile(
cacheDir,
cachedContent1.id,
/* offset= */ 10,
cacheFileLength,
/* lastAccessTimestamp= */ 30);
SimpleCacheSpan span = SimpleCacheSpan.createCacheEntry(cacheSpanFile, cacheFileLength, index);
assertThat(span).isNotNull();
cachedContent1.addSpan(span);
......@@ -274,9 +280,15 @@ public class CachedContentIndexTest {
@Test
public void testCantRemoveNotEmptyCachedContent() throws Exception {
CachedContent cachedContent = index.getOrAdd("key1");
File cacheSpanFile =
SimpleCacheSpanTest.createCacheSpanFile(cacheDir, cachedContent.id, 10, 20, 30);
SimpleCacheSpan span = SimpleCacheSpan.createCacheEntry(cacheSpanFile, index);
long cacheFileLength = 20;
File cacheFile =
SimpleCacheSpanTest.createCacheSpanFile(
cacheDir,
cachedContent.id,
/* offset= */ 10,
cacheFileLength,
/* lastAccessTimestamp= */ 30);
SimpleCacheSpan span = SimpleCacheSpan.createCacheEntry(cacheFile, cacheFileLength, index);
cachedContent.addSpan(span);
index.maybeRemove(cachedContent.key);
......
......@@ -128,7 +128,7 @@ public final class CachedRegionTrackerTest {
private CacheSpan newCacheSpan(int position, int length) throws IOException {
int id = index.assignIdForKey(CACHE_KEY);
File cacheFile = createCacheSpanFile(cacheDir, id, position, length, 0);
return SimpleCacheSpan.createCacheEntry(cacheFile, index);
return SimpleCacheSpan.createCacheEntry(cacheFile, length, index);
}
public static File createCacheSpanFile(
......
......@@ -36,8 +36,9 @@ import org.robolectric.RuntimeEnvironment;
@RunWith(RobolectricTestRunner.class)
public class SimpleCacheSpanTest {
public static File createCacheSpanFile(File cacheDir, int id, long offset, int length,
long lastAccessTimestamp) throws IOException {
public static File createCacheSpanFile(
File cacheDir, int id, long offset, long length, long lastAccessTimestamp)
throws IOException {
File cacheFile = SimpleCacheSpan.getCacheFile(cacheDir, id, offset, lastAccessTimestamp);
createTestFile(cacheFile, length);
return cacheFile;
......@@ -87,7 +88,7 @@ public class SimpleCacheSpanTest {
File v1File = createTestFile("abc%def.5.6.v1.exo"); // V1 did not escape
for (File file : cacheDir.listFiles()) {
SimpleCacheSpan cacheEntry = SimpleCacheSpan.createCacheEntry(file, index);
SimpleCacheSpan cacheEntry = SimpleCacheSpan.createCacheEntry(file, file.length(), index);
if (file.equals(wrongEscapedV2file)) {
assertThat(cacheEntry).isNull();
} else {
......@@ -112,7 +113,7 @@ public class SimpleCacheSpanTest {
HashMap<Long, Long> cachedPositions = new HashMap<>();
for (File file : files) {
SimpleCacheSpan cacheSpan = SimpleCacheSpan.createCacheEntry(file, index);
SimpleCacheSpan cacheSpan = SimpleCacheSpan.createCacheEntry(file, file.length(), index);
if (cacheSpan != null) {
assertThat(cacheSpan.key).isEqualTo(key);
cachedPositions.put(cacheSpan.position, cacheSpan.lastAccessTimestamp);
......@@ -124,7 +125,7 @@ public class SimpleCacheSpanTest {
assertThat(cachedPositions.get((long) 5)).isEqualTo(6);
}
private static void createTestFile(File file, int length) throws IOException {
private static void createTestFile(File file, long length) throws IOException {
FileOutputStream output = new FileOutputStream(file);
for (int i = 0; i < length; i++) {
output.write(i);
......@@ -141,8 +142,10 @@ public class SimpleCacheSpanTest {
private void assertCacheSpan(String key, long offset, long lastAccessTimestamp)
throws IOException {
int id = index.assignIdForKey(key);
File cacheFile = createCacheSpanFile(cacheDir, id, offset, 1, lastAccessTimestamp);
SimpleCacheSpan cacheSpan = SimpleCacheSpan.createCacheEntry(cacheFile, index);
long cacheFileLength = 1;
File cacheFile =
createCacheSpanFile(cacheDir, id, offset, cacheFileLength, lastAccessTimestamp);
SimpleCacheSpan cacheSpan = SimpleCacheSpan.createCacheEntry(cacheFile, cacheFileLength, index);
String message = cacheFile.toString();
assertWithMessage(message).that(cacheSpan).isNotNull();
assertWithMessage(message).that(cacheFile.getParentFile()).isEqualTo(cacheDir);
......@@ -156,9 +159,10 @@ public class SimpleCacheSpanTest {
private void assertNullCacheSpan(File parent, String key, long offset,
long lastAccessTimestamp) {
long cacheFileLength = 0;
File cacheFile = SimpleCacheSpan.getCacheFile(parent, index.assignIdForKey(key), offset,
lastAccessTimestamp);
CacheSpan cacheSpan = SimpleCacheSpan.createCacheEntry(cacheFile, index);
CacheSpan cacheSpan = SimpleCacheSpan.createCacheEntry(cacheFile, cacheFileLength, index);
assertWithMessage(cacheFile.toString()).that(cacheSpan).isNull();
}
......
......@@ -377,7 +377,7 @@ public class SimpleCacheTest {
} finally {
fos.close();
}
simpleCache.commitFile(file);
simpleCache.commitFile(file, length);
}
private static void assertCachedDataReadCorrect(CacheSpan cacheSpan) throws IOException {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or sign in to comment