Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 425ec7a9 authored by Junyu Lai's avatar Junyu Lai
Browse files

Read files in increasing timestamp order in FileRotator 

The current design of the readMatching() function reads all
files that match a given prefix, but does not guarantee
the order in which they are read. This could cause
performance problems because the caller is optimized for
inserting files in ascending timestamp order but the files
are read in a reversed order.

This change sorts the list by the timestamp in the file name,
which is more natural since the files are written based on
timestamp. Benchmarking result shows that
testReadFromRecorder_manyUids improved from 1.5s to 1.0s
compares to the worst case.

Also, this is safe because all callers of the
readMatching() function only pass a list of less than 20 files.
None of these callers rely on the order in which the
files are read.

Test: atest ConnectivityBenchmarkTests FileRotatorTest
Bug: 269409485
Change-Id: I73f10890e8f454ee98e7d92fa754d1affe598df4
parent 5c4b8708

core/java/com/android/internal/util/FileRotator.java

+17 −7
Original line number Diff line number Diff line
@@ -19,6 +19,9 @@ package com.android.internal.util; 
import android.annotation.NonNull;

import android.os.FileUtils;

import android.util.Log;

import android.util.Pair;



import libcore.io.IoUtils;



import java.io.BufferedInputStream;

import java.io.BufferedOutputStream;
@@ -28,12 +31,12 @@ import java.io.FileOutputStream; 
import java.io.IOException;

import java.io.InputStream;

import java.io.OutputStream;

import java.util.Comparator;

import java.util.Objects;

import java.util.TreeSet;

import java.util.zip.ZipEntry;

import java.util.zip.ZipOutputStream;



import libcore.io.IoUtils;



/**

 * Utility that rotates files over time, similar to {@code logrotate}. There is

 * a single "active" file, which is periodically rotated into historical files,
@@ -302,18 +305,25 @@ public class FileRotator { 
    public void readMatching(Reader reader, long matchStartMillis, long matchEndMillis)

            throws IOException {

        final FileInfo info = new FileInfo(mPrefix);

        final TreeSet<Pair<Long, String>> readSet = new TreeSet<>(

                Comparator.comparingLong(o -> o.first));

        for (String name : mBasePath.list()) {

            if (!info.parse(name)) continue;



            // read file when it overlaps

            // Add file to set when it overlaps.

            if (info.startMillis <= matchEndMillis && matchStartMillis <= info.endMillis) {

                if (LOGD) Log.d(TAG, "reading matching " + name);

                readSet.add(new Pair(info.startMillis, name));

            }

        }



        // Read files in ascending order of start timestamp.

        for (Pair<Long, String> pair : readSet) {

            final String name = pair.second;

            if (LOGD) Log.d(TAG, "reading matching " + name);

            final File file = new File(mBasePath, name);

            readFile(file, reader);

        }

    }

    }



    /**

     * Return the currently active file, which may not exist yet.

core/tests/utiltests/src/com/android/internal/util/FileRotatorTest.java

+11 −10
Original line number Diff line number Diff line
@@ -366,6 +366,16 @@ public class FileRotatorTest extends AndroidTestCase { 
        assertReadAll(rotate, "bar");

    }



    public void testReadSorted() throws Exception {

        write("rotator.1024-2048", "2");

        write("rotator.2048-4096", "3");

        write("rotator.512-1024", "1");



        final FileRotator rotate = new FileRotator(

                mBasePath, PREFIX, SECOND_IN_MILLIS, SECOND_IN_MILLIS);

        assertReadAll(rotate, "1", "2", "3");

    }



    public void testFileSystemInaccessible() throws Exception {

        File inaccessibleDir = null;

        String dirPath = getContext().getFilesDir() + File.separator + "inaccessible";
@@ -422,16 +432,7 @@ public class FileRotatorTest extends AndroidTestCase { 
        }



        public void assertRead(String... expected) {

            assertEquals(expected.length, mActual.size());



            final ArrayList<String> actualCopy = new ArrayList<String>(mActual);

            for (String value : expected) {

                if (!actualCopy.remove(value)) {

                    final String expectedString = Arrays.toString(expected);

                    final String actualString = Arrays.toString(mActual.toArray());

                    fail("expected: " + expectedString + " but was: " + actualString);

                }

            }

            assertEquals(Arrays.asList(expected), mActual);

        }

    }

}