From 80259d00ba8fd68309f6d9063e8fd7cbd416711d Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Tue, 22 Aug 2017 10:42:26 +1000 Subject: [PATCH 1/6] More precise (and correct) logging of slow queries in debug mode. Some queries are deferred, and then forced to run by Android by invoking `getCount()`. Under these circumstances, the measured speed of the query execution is 1ms. This adds speed logging around `getCount()` in case that is the first time the query is run. --- .../org/fdroid/fdroid/data/LoggingQuery.java | 29 ++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/org/fdroid/fdroid/data/LoggingQuery.java b/app/src/main/java/org/fdroid/fdroid/data/LoggingQuery.java index bf257a0de..fcd66d202 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/LoggingQuery.java +++ b/app/src/main/java/org/fdroid/fdroid/data/LoggingQuery.java @@ -1,6 +1,7 @@ package org.fdroid.fdroid.data; import android.database.Cursor; +import android.database.CursorWrapper; import android.database.sqlite.SQLiteDatabase; import org.fdroid.fdroid.BuildConfig; @@ -61,11 +62,37 @@ final class LoggingQuery { if (queryDuration >= SLOW_QUERY_DURATION) { logSlowQuery(queryDuration); } - return cursor; + + return new LogGetCountCursorWrapper(cursor); } return db.rawQuery(query, queryArgs); } + /** + * Sometimes the query will not actually be run when invoking "query()". + * Under such circumstances, it falls to the {@link android.content.ContentProvider#query} + * method to manually invoke the {@link Cursor#getCount()} method to force query execution. + * It does so with a comment saying "Force query execution". When this happens, the call to + * query() takes 1ms, whereas the call go getCount() is the bit which takes time. + * As such, we will also track that method duration in order to potentially log slow queries. + */ + private class LogGetCountCursorWrapper extends CursorWrapper { + private LogGetCountCursorWrapper(Cursor cursor) { + super(cursor); + } + + @Override + public int getCount() { + long startTime = System.currentTimeMillis(); + int count = super.getCount(); + long queryDuration = System.currentTimeMillis() - startTime; + if (queryDuration >= SLOW_QUERY_DURATION) { + logSlowQuery(queryDuration); + } + return count; + } + } + private void execSQLInternal() { if (BuildConfig.DEBUG) { long startTime = System.currentTimeMillis(); From 620affa239941c764cd5a132bb687857315c744d Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Tue, 22 Aug 2017 12:59:25 +1000 Subject: [PATCH 2/6] Remove unneeded join onto apk which was causing performance problems. This join resulted in one row for each apk in the result set (before doing a GROUP BY), instead of one row for each apk. That is a large difference in number of rows and resulted in much more work for sqlite. Turns out this join wasn't required. --- app/src/main/java/org/fdroid/fdroid/data/AppProvider.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/data/AppProvider.java b/app/src/main/java/org/fdroid/fdroid/data/AppProvider.java index 5d8851fe1..3180d57d2 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/AppProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/AppProvider.java @@ -217,7 +217,6 @@ public class AppProvider extends FDroidProvider { protected String getRequiredTables() { final String pkg = PackageTable.NAME; final String app = getTableName(); - final String apk = getApkTableName(); final String repo = RepoTable.NAME; final String cat = CategoryTable.NAME; final String catJoin = getCatJoinTableName(); @@ -226,8 +225,7 @@ public class AppProvider extends FDroidProvider { " JOIN " + app + " ON (" + app + "." + Cols.PACKAGE_ID + " = " + pkg + "." + PackageTable.Cols.ROW_ID + ") " + " JOIN " + repo + " ON (" + app + "." + Cols.REPO_ID + " = " + repo + "." + RepoTable.Cols._ID + ") " + " LEFT JOIN " + catJoin + " ON (" + app + "." + Cols.ROW_ID + " = " + catJoin + "." + CatJoinTable.Cols.APP_METADATA_ID + ") " + - " LEFT JOIN " + cat + " ON (" + cat + "." + CategoryTable.Cols.ROW_ID + " = " + catJoin + "." + CatJoinTable.Cols.CATEGORY_ID + ") " + - " LEFT JOIN " + apk + " ON (" + apk + "." + ApkTable.Cols.APP_ID + " = " + app + "." + Cols.ROW_ID + ") "; + " LEFT JOIN " + cat + " ON (" + cat + "." + CategoryTable.Cols.ROW_ID + " = " + catJoin + "." + CatJoinTable.Cols.CATEGORY_ID + ") "; } @Override From ac1dce24d29d687f8e77f8d82cf66ecd4f4958ab Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Tue, 5 Sep 2017 12:56:05 +1000 Subject: [PATCH 3/6] Don't assume all apps have a preferred signer, as media apps don't Fixes #1156. --- app/src/main/java/org/fdroid/fdroid/data/App.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/data/App.java b/app/src/main/java/org/fdroid/fdroid/data/App.java index 16e2925a1..7f91f6113 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/App.java +++ b/app/src/main/java/org/fdroid/fdroid/data/App.java @@ -1141,13 +1141,14 @@ public class App extends ValueObject implements Comparable, Parcelable { * the user to try and install versions with that signature (because thats all the OS will let * them do). */ - @NonNull + @Nullable public String getMostAppropriateSignature() { if (!TextUtils.isEmpty(installedSig)) { return installedSig; } else if (!TextUtils.isEmpty(preferredSigner)) { return preferredSigner; } - throw new IllegalStateException("Most Appropriate Signature not found!"); + + return null; } } From 595f72d5b28b93f0bc515d559b078bccf0a401db Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Tue, 12 Sep 2017 16:48:14 +0200 Subject: [PATCH 4/6] Calculate whether an app is an APK or not when updating repos. This improves performance when we need to decide whether or not apps are installed or not while scrolling through large lists. Fixes #1143. Also change Jackson tests to properly ignore App#isApk. --- .../org/fdroid/fdroid/IndexV1Updater.java | 6 +++++ .../main/java/org/fdroid/fdroid/data/App.java | 16 ++++++++++- .../java/org/fdroid/fdroid/data/DBHelper.java | 27 ++++++++++++++++++- .../java/org/fdroid/fdroid/data/Schema.java | 5 ++-- .../fdroid/updater/IndexV1UpdaterTest.java | 2 ++ 5 files changed, 52 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/IndexV1Updater.java b/app/src/main/java/org/fdroid/fdroid/IndexV1Updater.java index b199ebbb4..046c7f38c 100644 --- a/app/src/main/java/org/fdroid/fdroid/IndexV1Updater.java +++ b/app/src/main/java/org/fdroid/fdroid/IndexV1Updater.java @@ -226,6 +226,12 @@ public class IndexV1Updater extends RepoUpdater { if (apks.size() > 0) { app.preferredSigner = apks.get(0).sig; + app.isApk = true; + for (Apk apk : apks) { + if (!apk.isApk()) { + app.isApk = false; + } + } } if (appCount % 50 == 0) { diff --git a/app/src/main/java/org/fdroid/fdroid/data/App.java b/app/src/main/java/org/fdroid/fdroid/data/App.java index 7f91f6113..e621fd23b 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/App.java +++ b/app/src/main/java/org/fdroid/fdroid/data/App.java @@ -104,6 +104,8 @@ public class App extends ValueObject implements Comparable, Parcelable { @JsonIgnore @NonNull public String preferredSigner; + @JsonIgnore + public boolean isApk; @JacksonInject("repoId") public long repoId; @@ -347,6 +349,9 @@ public class App extends ValueObject implements Comparable, Parcelable { case Cols.WEAR_SCREENSHOTS: wearScreenshots = Utils.parseCommaSeparatedString(cursor.getString(i)); break; + case Cols.IS_APK: + isApk = cursor.getInt(i) == 1; + break; case Cols.InstalledApp.VERSION_CODE: installedVersionCode = cursor.getInt(i); break; @@ -854,12 +859,19 @@ public class App extends ValueObject implements Comparable, Parcelable { values.put(Cols.TV_SCREENSHOTS, Utils.serializeCommaSeparatedString(tvScreenshots)); values.put(Cols.WEAR_SCREENSHOTS, Utils.serializeCommaSeparatedString(wearScreenshots)); values.put(Cols.IS_COMPATIBLE, compatible ? 1 : 0); + values.put(Cols.IS_APK, isApk ? 1 : 0); return values; } public boolean isInstalled(Context context) { - return installedVersionCode > 0 || isMediaInstalled(context); + // First check isApk() before isMediaInstalled() because the latter is quite expensive, + // hitting the database for each apk version, then the disk to check for installed media. + return installedVersionCode > 0 || (!isApk() && isMediaInstalled(context)); + } + + private boolean isApk() { + return isApk; } public boolean isMediaInstalled(Context context) { @@ -1064,6 +1076,7 @@ public class App extends ValueObject implements Comparable, Parcelable { dest.writeStringArray(this.tenInchScreenshots); dest.writeStringArray(this.tvScreenshots); dest.writeStringArray(this.wearScreenshots); + dest.writeByte(this.isApk ? (byte) 1 : (byte) 0); dest.writeString(this.installedVersionName); dest.writeInt(this.installedVersionCode); dest.writeParcelable(this.installedApk, flags); @@ -1114,6 +1127,7 @@ public class App extends ValueObject implements Comparable, Parcelable { this.tenInchScreenshots = in.createStringArray(); this.tvScreenshots = in.createStringArray(); this.wearScreenshots = in.createStringArray(); + this.isApk = in.readByte() != 0; this.installedVersionName = in.readString(); this.installedVersionCode = in.readInt(); this.installedApk = in.readParcelable(Apk.class.getClassLoader()); diff --git a/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java b/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java index 2e604392e..7dddc07b6 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java +++ b/app/src/main/java/org/fdroid/fdroid/data/DBHelper.java @@ -151,6 +151,7 @@ class DBHelper extends SQLiteOpenHelper { + AppMetadataTable.Cols.TEN_INCH_SCREENSHOTS + " string," + AppMetadataTable.Cols.TV_SCREENSHOTS + " string," + AppMetadataTable.Cols.WEAR_SCREENSHOTS + " string," + + AppMetadataTable.Cols.IS_APK + " boolean," + "primary key(" + AppMetadataTable.Cols.PACKAGE_ID + ", " + AppMetadataTable.Cols.REPO_ID + "));"; private static final String CREATE_TABLE_APP_PREFS = "CREATE TABLE " + AppPrefsTable.NAME @@ -193,7 +194,7 @@ class DBHelper extends SQLiteOpenHelper { + InstalledAppTable.Cols.HASH + " TEXT NOT NULL" + " );"; - protected static final int DB_VERSION = 73; + protected static final int DB_VERSION = 74; private final Context context; @@ -281,6 +282,30 @@ class DBHelper extends SQLiteOpenHelper { addIntegerPrimaryKeyToInstalledApps(db, oldVersion); addPreferredSignerToApp(db, oldVersion); updatePreferredSignerIfEmpty(db, oldVersion); + addIsAppToApp(db, oldVersion); + } + + private void addIsAppToApp(SQLiteDatabase db, int oldVersion) { + if (oldVersion >= 74) { + return; + } + + if (!columnExists(db, AppMetadataTable.NAME, AppMetadataTable.Cols.IS_APK)) { + Log.i(TAG, "Figuring out whether each \"app\" is actually an app, or it represents other media."); + db.execSQL("alter table " + AppMetadataTable.NAME + " add column " + AppMetadataTable.Cols.IS_APK + " boolean;"); + + // Find all apks for which their filename DOESN'T end in ".apk", and if there is more than one, the + // corresponding app is updated to be marked as media. + String apkName = ApkTable.Cols.NAME; + String query = "UPDATE " + AppMetadataTable.NAME + " SET " + AppMetadataTable.Cols.IS_APK + " = (" + + " SELECT COUNT(*) FROM " + ApkTable.NAME + " AS apk" + + " WHERE " + + " " + ApkTable.Cols.APP_ID + " = " + AppMetadataTable.NAME + "." + AppMetadataTable.Cols.ROW_ID + + " AND SUBSTR(" + apkName + ", LENGTH(" + apkName + ") - 3) != '.apk'" + + ") = 0;"; + Log.i(TAG, query); + db.execSQL(query); + } } private void updatePreferredSignerIfEmpty(SQLiteDatabase db, int oldVersion) { diff --git a/app/src/main/java/org/fdroid/fdroid/data/Schema.java b/app/src/main/java/org/fdroid/fdroid/data/Schema.java index da3e80565..fcfdfd6a3 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/Schema.java +++ b/app/src/main/java/org/fdroid/fdroid/data/Schema.java @@ -156,6 +156,7 @@ public interface Schema { String TEN_INCH_SCREENSHOTS = "tenInchScreenshots"; String TV_SCREENSHOTS = "tvScreenshots"; String WEAR_SCREENSHOTS = "wearScreenshots"; + String IS_APK = "isApk"; interface SuggestedApk { String VERSION_NAME = "suggestedApkVersion"; @@ -195,7 +196,7 @@ public interface Schema { ANTI_FEATURES, REQUIREMENTS, ICON_URL, ICON_URL_LARGE, FEATURE_GRAPHIC, PROMO_GRAPHIC, TV_BANNER, PHONE_SCREENSHOTS, SEVEN_INCH_SCREENSHOTS, TEN_INCH_SCREENSHOTS, TV_SCREENSHOTS, WEAR_SCREENSHOTS, - PREFERRED_SIGNER, SUGGESTED_VERSION_CODE, + PREFERRED_SIGNER, SUGGESTED_VERSION_CODE, IS_APK, }; /** @@ -211,7 +212,7 @@ public interface Schema { ANTI_FEATURES, REQUIREMENTS, ICON_URL, ICON_URL_LARGE, FEATURE_GRAPHIC, PROMO_GRAPHIC, TV_BANNER, PHONE_SCREENSHOTS, SEVEN_INCH_SCREENSHOTS, TEN_INCH_SCREENSHOTS, TV_SCREENSHOTS, WEAR_SCREENSHOTS, - PREFERRED_SIGNER, SUGGESTED_VERSION_CODE, SuggestedApk.VERSION_NAME, + PREFERRED_SIGNER, SUGGESTED_VERSION_CODE, IS_APK, SuggestedApk.VERSION_NAME, InstalledApp.VERSION_CODE, InstalledApp.VERSION_NAME, InstalledApp.SIGNATURE, Package.PACKAGE_NAME, }; diff --git a/app/src/test/java/org/fdroid/fdroid/updater/IndexV1UpdaterTest.java b/app/src/test/java/org/fdroid/fdroid/updater/IndexV1UpdaterTest.java index 41df377df..c070fea20 100644 --- a/app/src/test/java/org/fdroid/fdroid/updater/IndexV1UpdaterTest.java +++ b/app/src/test/java/org/fdroid/fdroid/updater/IndexV1UpdaterTest.java @@ -304,6 +304,7 @@ public class IndexV1UpdaterTest extends FDroidProviderTest { "installedSig", "installedVersionCode", "installedVersionName", + "isApk", "preferredSigner", "prefs", "TAG", @@ -335,6 +336,7 @@ public class IndexV1UpdaterTest extends FDroidProviderTest { "hash", "hashType", "incompatibleReasons", + "isApk", "maxSdkVersion", "minSdkVersion", "nativecode", From 3a3c170781bc9fc7f15865bfce2a0644746d89ab Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Mon, 11 Sep 2017 12:48:24 +1000 Subject: [PATCH 5/6] Fix CI failures (checkstyle/pmd) --- app/src/main/java/org/fdroid/fdroid/data/LoggingQuery.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/main/java/org/fdroid/fdroid/data/LoggingQuery.java b/app/src/main/java/org/fdroid/fdroid/data/LoggingQuery.java index fcd66d202..aaa1c06ab 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/LoggingQuery.java +++ b/app/src/main/java/org/fdroid/fdroid/data/LoggingQuery.java @@ -76,7 +76,7 @@ final class LoggingQuery { * query() takes 1ms, whereas the call go getCount() is the bit which takes time. * As such, we will also track that method duration in order to potentially log slow queries. */ - private class LogGetCountCursorWrapper extends CursorWrapper { + private final class LogGetCountCursorWrapper extends CursorWrapper { private LogGetCountCursorWrapper(Cursor cursor) { super(cursor); } From 71337a49b327a33c3530b719ea009d24ff24291a Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Tue, 12 Sep 2017 08:43:17 +1000 Subject: [PATCH 6/6] Added doc comment clarifying unsigned media --- app/src/main/java/org/fdroid/fdroid/data/App.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/src/main/java/org/fdroid/fdroid/data/App.java b/app/src/main/java/org/fdroid/fdroid/data/App.java index e621fd23b..11a8ac0d0 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/App.java +++ b/app/src/main/java/org/fdroid/fdroid/data/App.java @@ -1154,6 +1154,9 @@ public class App extends ValueObject implements Comparable, Parcelable { * However, if the app is installed, then we override this and instead want to only encourage * the user to try and install versions with that signature (because thats all the OS will let * them do). + * + * Will return null for any {@link App} which represents media (instead of an apk) and thus + * doesn't have a signer. */ @Nullable public String getMostAppropriateSignature() {