android: Low rank group

Implement code using updateServiceGroup. Specifically create a group for
service connections that have low rank. Currently the cutoff is at
"invisible main frame" and below.

Now implementation uses own sort implementation by taking advantage that
updates happen to a single connection at a time only. NB the simplified
sort is no longer stable, and specifically pulls up connection's rank
whenever it's updated.

Algorithm for assigning importance in group is to pick a value between
importance of connection to the left and to the right (if any), and
reshuffle the entire list of there is not enough room left. There is
a bias of assigning closer to the right; this is an optimization to
reduce shuffle since by far the most common operation is moving a
connection to be the most important (ie with low importance value)
in the group.

Add a feature to enable this so we can experiment when there are
enough users.

Bug: 946216
Change-Id: I2ea4437b9b27eb205b7c2e51344f2af387938232
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1597071Reviewed-by: ssid <ssid@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#659436}

base/android/java/src/org/chromium/base/process_launcher/ChildProcessConnection.java

@@ -660,6 +660,7 @@ public class ChildProcessConnection {
         assert isRunningOnLauncherThread();
         assert !mUnbound;
         assert mWaivedBinding.isBound();
+        assert group != 0 || importanceInGroup == 0;
         if (mGroup != group || mImportanceInGroup != importanceInGroup) {
             mGroup = group;
             mImportanceInGroup = importanceInGroup;
@@ -667,6 +668,16 @@ public class ChildProcessConnection {
         }
     }
 
+    public int getGroup() {
+        assert isRunningOnLauncherThread();
+        return mGroup;
+    }
+
+    public int getImportanceInGroup() {
+        assert isRunningOnLauncherThread();
+        return mImportanceInGroup;
+    }
+
     public boolean isStrongBindingBound() {
         assert isRunningOnLauncherThread();
         return mStrongBinding.isBound();

base/android/junit/src/org/chromium/base/process_launcher/ChildProcessConnectionTest.java

@@ -467,6 +467,8 @@ public class ChildProcessConnectionTest {
         ChildProcessConnection connection = createDefaultTestConnection();
         connection.start(false /* useStrongBinding */, null /* serviceCallback */);
         connection.updateGroupImportance(1, 2);
+        assertEquals(1, connection.getGroup());
+        assertEquals(2, connection.getImportanceInGroup());
         // Expect a important, moderate and waived bindings.
         assertEquals(3, mMockConnections.size());
         // Group should be set on the wavied (last) binding.

content/browser/android/content_feature_list.cc

@@ -25,6 +25,7 @@ const base::Feature* kFeaturesExposedToJava[] = {
     &features::kBackgroundMediaRendererHasModerateBinding,
     &kEnhancedSelectionInsertionHandle,
     &features::kServiceWorkerForegroundPriority,
+    &kServiceGroupImportance,
 };
 
 const base::Feature* FindFeatureExposedToJava(const std::string& feature_name) {
@@ -44,6 +45,8 @@ const base::Feature kEnhancedSelectionInsertionHandle{
     "EnhancedSelectionInsertionHandle", base::FEATURE_ENABLED_BY_DEFAULT};
 const base::Feature kRequestUnbufferedDispatch{
     "RequestUnbufferedDispatch", base::FEATURE_ENABLED_BY_DEFAULT};
+const base::Feature kServiceGroupImportance{"ServiceGroupImportance",
+                                            base::FEATURE_DISABLED_BY_DEFAULT};
 
 static jboolean JNI_ContentFeatureList_IsEnabled(
     JNIEnv* env,

content/browser/android/content_feature_list.h

@@ -13,6 +13,7 @@ namespace android {
 // Alphabetical:
 extern const base::Feature kEnhancedSelectionInsertionHandle;
 extern const base::Feature kRequestUnbufferedDispatch;
+extern const base::Feature kServiceGroupImportance;
 
 }  // namespace android
 }  // namespace content

content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelperImpl.java

@@ -62,6 +62,9 @@ public final class ChildProcessLauncherHelperImpl {
     private static final String PRIVILEGED_SERVICES_NAME =
             "org.chromium.content.app.PrivilegedProcessService";
 
+    // Flag to check features after native is initialized.
+    private static boolean sCheckedFeatures;
+
     // A warmed-up connection to a sandboxed service.
     private static SpareChildConnection sSpareSandboxedConnection;
 
@@ -230,6 +233,14 @@ public final class ChildProcessLauncherHelperImpl {
         ChildProcessLauncherHelperImpl helper = new ChildProcessLauncherHelperImpl(nativePointer,
                 commandLine, filesToBeMapped, sandboxed, canUseWarmUpConnection, binderCallback);
         helper.start();
+
+        if (!sCheckedFeatures) {
+            sCheckedFeatures = true;
+            if (sSandboxedChildConnectionRanking != null
+                    && ContentFeatureList.isEnabled(ContentFeatureList.SERVICE_GROUP_IMPORTANCE)) {
+                sSandboxedChildConnectionRanking.enableServiceGroupImportance();
+            }
+        }
         return helper;
     }
 

content/public/android/java/src/org/chromium/content/browser/ChildProcessRanking.java

@@ -4,11 +4,11 @@
 
 package org.chromium.content.browser;
 
+import org.chromium.base.BuildConfig;
 import org.chromium.base.process_launcher.ChildProcessConnection;
 import org.chromium.content_public.browser.ChildProcessImportance;
 
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.Comparator;
 import java.util.Iterator;
 import java.util.List;
@@ -17,6 +17,15 @@ import java.util.List;
  * Ranking of ChildProcessConnections for a particular ChildConnectionAllocator.
  */
 public class ChildProcessRanking implements Iterable<ChildProcessConnection> {
+    private static final boolean ENABLE_CHECKS = BuildConfig.DCHECK_IS_ON;
+    private static final int NO_GROUP = 0;
+    private static final int LOW_RANK_GROUP = 1;
+
+    // If there is a gap in importance that's larger than 2 * FROM_RIGHT, insert connection
+    // with importance right - FROM_RIGHT rather than in the middle. Use 15 out of 31 bits
+    // so should support 2^16 connections, which should be way more than enough.
+    private static final int FROM_RIGHT = 32768;
+
     private static class ConnectionWithRank {
         public final ChildProcessConnection connection;
 
@@ -36,6 +45,16 @@ public class ChildProcessRanking implements Iterable<ChildProcessConnection> {
             this.intersectsViewport = intersectsViewport;
             this.importance = importance;
         }
+
+        // Returns true for low ranked connection that it should be in the low rank group.
+        // Note this must be kept up-to-date with RankComparator so that all shouldBeInLowRankGroup
+        // connections are sorted to the end of the list.
+        // Note being in the low rank group does not necessarily imply the connection is not
+        // important or that it only has waived binding.
+        public boolean shouldBeInLowRankGroup() {
+            boolean inViewport = visible && (frameDepth == 0 || intersectsViewport);
+            return importance == ChildProcessImportance.NORMAL && !inViewport;
+        }
     }
 
     private static class RankComparator implements Comparator<ConnectionWithRank> {
@@ -52,21 +71,13 @@ public class ChildProcessRanking implements Iterable<ChildProcessConnection> {
 
         @Override
         public int compare(ConnectionWithRank o1, ConnectionWithRank o2) {
-            // Sort null to the end.
-            if (o1 == null && o2 == null) {
-                return 0;
-            } else if (o1 == null && o2 != null) {
-                return 1;
-            } else if (o1 != null && o2 == null) {
-                return -1;
-            }
-
             assert o1 != null;
             assert o2 != null;
 
             // Ranking order:
             // * (visible and main frame) or ChildProcessImportance.IMPORTANT
             // * (visible and subframe and intersect viewport) or ChildProcessImportance.MODERATE
+            // ---- cutoff for shouldBeInLowRankGroup ----
             // * invisible main frame
             // * visible subframe and not intersect viewport
             // * invisible subframes
@@ -156,6 +167,8 @@ public class ChildProcessRanking implements Iterable<ChildProcessConnection> {
     // for sizes in production and more memory efficient than linked data structures.
     private final List<ConnectionWithRank> mRankings = new ArrayList<>();
 
+    private boolean mEnableServiceGroupImportance;
+
     public ChildProcessRanking() {
         mMaxSize = -1;
     }
@@ -168,6 +181,13 @@ public class ChildProcessRanking implements Iterable<ChildProcessConnection> {
         mMaxSize = maxSize;
     }
 
+    public void enableServiceGroupImportance() {
+        assert !mEnableServiceGroupImportance;
+        mEnableServiceGroupImportance = true;
+        reshuffleGroupImportance();
+        if (ENABLE_CHECKS) checkGroupImportance();
+    }
+
     /**
      * Iterate from lowest to highest rank. Ranking should not be modified during iteration,
      * including using Iterator.delete.
@@ -187,7 +207,7 @@ public class ChildProcessRanking implements Iterable<ChildProcessConnection> {
         }
         mRankings.add(new ConnectionWithRank(
                 connection, visible, frameDepth, intersectsViewport, importance));
-        sort();
+        reposition(mRankings.size() - 1);
     }
 
     public void removeConnection(ChildProcessConnection connection) {
@@ -198,6 +218,7 @@ public class ChildProcessRanking implements Iterable<ChildProcessConnection> {
 
         // Null is sorted to the end.
         mRankings.remove(i);
+        if (ENABLE_CHECKS) checkOrder();
     }
 
     public void updateConnection(ChildProcessConnection connection, boolean visible,
@@ -212,7 +233,7 @@ public class ChildProcessRanking implements Iterable<ChildProcessConnection> {
         rank.frameDepth = frameDepth;
         rank.intersectsViewport = intersectsViewport;
         rank.importance = importance;
-        sort();
+        reposition(i);
     }
 
     public ChildProcessConnection getLowestRankedConnection() {
@@ -227,8 +248,100 @@ public class ChildProcessRanking implements Iterable<ChildProcessConnection> {
         return -1;
     }
 
-    private void sort() {
-        // Sort is stable.
-        Collections.sort(mRankings, COMPARATOR);
+    private void reposition(final int originalIndex) {
+        ConnectionWithRank connection = mRankings.remove(originalIndex);
+        int newIndex = 0;
+        while (newIndex < mRankings.size()
+                && COMPARATOR.compare(mRankings.get(newIndex), connection) < 0) {
+            ++newIndex;
+        }
+        mRankings.add(newIndex, connection);
+        if (ENABLE_CHECKS) checkOrder();
+
+        if (!mEnableServiceGroupImportance) return;
+
+        if (!connection.shouldBeInLowRankGroup()) {
+            if (connection.connection.getGroup() != NO_GROUP) {
+                connection.connection.updateGroupImportance(NO_GROUP, 0);
+            }
+            return;
+        }
+
+        final boolean atStart = newIndex == 0;
+        final boolean atEnd = newIndex == mRankings.size() - 1;
+
+        final int left =
+                atStart ? 0 : mRankings.get(newIndex - 1).connection.getImportanceInGroup();
+
+        assert atEnd || mRankings.get(newIndex + 1).connection.getGroup() > NO_GROUP;
+        final int right = atEnd ? Integer.MAX_VALUE
+                                : mRankings.get(newIndex + 1).connection.getImportanceInGroup();
+
+        if (connection.connection.getImportanceInGroup() > left
+                && connection.connection.getImportanceInGroup() < right) {
+            return;
+        }
+
+        final int gap = right - left;
+
+        // If there is a large enough gap, place connection close to the end. This is a heuristic
+        // since updating a connection to be the highest ranked (lowest index) occurs very
+        // frequently, eg when switching between tabs.
+        // If gap is small, use average.
+        // If there is no room left, reshuffle everything.
+        if (gap > 2 * FROM_RIGHT) {
+            connection.connection.updateGroupImportance(LOW_RANK_GROUP, right - FROM_RIGHT);
+        } else if (gap > 2) {
+            connection.connection.updateGroupImportance(LOW_RANK_GROUP, left + gap / 2);
+        } else {
+            reshuffleGroupImportance();
+        }
+
+        if (ENABLE_CHECKS) checkGroupImportance();
+    }
+
+    private void reshuffleGroupImportance() {
+        int importance = Integer.MAX_VALUE - FROM_RIGHT;
+        for (int i = mRankings.size() - 1; i >= 0; --i) {
+            ConnectionWithRank connection = mRankings.get(i);
+            if (!connection.shouldBeInLowRankGroup()) break;
+            connection.connection.updateGroupImportance(LOW_RANK_GROUP, importance);
+            importance -= FROM_RIGHT;
+        }
+    }
+
+    private void checkOrder() {
+        boolean crossedLowRankGroupCutoff = false;
+        for (int i = 0; i < mRankings.size(); ++i) {
+            ConnectionWithRank connection = mRankings.get(i);
+            if (i > 0 && COMPARATOR.compare(mRankings.get(i - 1), connection) > 0) {
+                throw new RuntimeException("Not sorted " + mRankings.get(i - 1) + " " + connection);
+            }
+            boolean inLowGroup = connection.shouldBeInLowRankGroup();
+            if (crossedLowRankGroupCutoff && !inLowGroup) {
+                throw new RuntimeException("Not in low rank " + connection);
+            }
+            crossedLowRankGroupCutoff = inLowGroup;
+        }
+    }
+
+    private void checkGroupImportance() {
+        int importance = -1;
+        for (int i = 0; i < mRankings.size(); ++i) {
+            ConnectionWithRank connection = mRankings.get(i);
+            if (connection.shouldBeInLowRankGroup()) {
+                if (connection.connection.getGroup() != LOW_RANK_GROUP) {
+                    throw new RuntimeException("Not in low rank group " + connection);
+                }
+                if (connection.connection.getImportanceInGroup() <= importance) {
+                    throw new RuntimeException("Wrong group importance order " + connection);
+                }
+                importance = connection.connection.getImportanceInGroup();
+            } else {
+                if (connection.connection.getGroup() != NO_GROUP) {
+                    throw new RuntimeException("Should not be in group " + connection);
+                }
+            }
+        }
     }
 }

content/public/android/java/src/org/chromium/content/browser/ContentFeatureList.java

@@ -36,6 +36,8 @@ public abstract class ContentFeatureList {
     public static final String BACKGROUND_MEDIA_RENDERER_HAS_MODERATE_BINDING =
             "BackgroundMediaRendererHasModerateBinding";
 
+    public static final String SERVICE_GROUP_IMPORTANCE = "ServiceGroupImportance";
+
     public static final String SERVICE_WORKER_FOREGROUND_PRIORITY =
             "ServiceWorkerForegroundPriority";
 

content/public/android/junit/src/org/chromium/content/browser/ChildProcessRankingTest.java

@@ -21,9 +21,11 @@ import org.chromium.content_public.browser.ChildProcessImportance;
 @Config(manifest = Config.NONE)
 public class ChildProcessRankingTest {
     private ChildProcessConnection createConnection() {
-        return new TestChildProcessConnection(new ComponentName("pkg", "cls"),
-                false /* bindToCallerCheck */, false /* bindAsExternalService */,
-                null /* serviceBundle */);
+        TestChildProcessConnection connection = new TestChildProcessConnection(
+                new ComponentName("pkg", "cls"), false /* bindToCallerCheck */,
+                false /* bindAsExternalService */, null /* serviceBundle */);
+        connection.start(false /* useStrongBinding */, null /* serviceCallback */);
+        return connection;
     }
 
     private void assertRankingAndRemoveAll(
@@ -49,19 +51,42 @@ public class ChildProcessRankingTest {
         Assert.assertNull(ranking.getLowestRankedConnection());
     }
 
+    private void assertNotInGroup(ChildProcessConnection[] connections) {
+        for (ChildProcessConnection c : connections) {
+            Assert.assertEquals(0, c.getGroup());
+        }
+    }
+
+    private void assertInGroupOrderedByImportance(ChildProcessConnection[] connections) {
+        int importanceSoFar = -1;
+        for (ChildProcessConnection c : connections) {
+            Assert.assertTrue(c.getGroup() > 0);
+            Assert.assertTrue(c.getImportanceInGroup() > importanceSoFar);
+            importanceSoFar = c.getImportanceInGroup();
+        }
+    }
+
     @Test
     public void testRanking() {
         ChildProcessRanking ranking = new ChildProcessRanking(10);
-        doTestRanking(ranking);
+        doTestRanking(ranking, false);
     }
 
     @Test
     public void testRankingWithoutLimit() {
         ChildProcessRanking ranking = new ChildProcessRanking();
-        doTestRanking(ranking);
+        doTestRanking(ranking, false);
     }
 
-    private void doTestRanking(ChildProcessRanking ranking) {
+    @Test
+    public void testEnableGroupAfter() {
+        ChildProcessRanking ranking = new ChildProcessRanking();
+        doTestRanking(ranking, true);
+    }
+
+    private void doTestRanking(ChildProcessRanking ranking, boolean enableGroupImportanceAfter) {
+        if (!enableGroupImportanceAfter) ranking.enableServiceGroupImportance();
+
         ChildProcessConnection c1 = createConnection();
         ChildProcessConnection c2 = createConnection();
         ChildProcessConnection c3 = createConnection();
@@ -108,8 +133,17 @@ public class ChildProcessRankingTest {
         ranking.addConnection(c10, true /* foreground */, 0 /* frameDepth */,
                 true /* intersectsViewport */, ChildProcessImportance.NORMAL);
 
+        if (enableGroupImportanceAfter) {
+            assertNotInGroup(
+                    new ChildProcessConnection[] {c10, c9, c8, c7, c6, c5, c4, c3, c2, c1});
+            ranking.enableServiceGroupImportance();
+        }
+
         assertRankingAndRemoveAll(
                 ranking, new ChildProcessConnection[] {c10, c9, c8, c7, c6, c5, c4, c3, c2, c1});
+
+        assertNotInGroup(new ChildProcessConnection[] {c10, c9, c8});
+        assertInGroupOrderedByImportance(new ChildProcessConnection[] {c7, c6, c5, c4, c3, c2, c1});
     }
 
     @Test
@@ -120,6 +154,7 @@ public class ChildProcessRankingTest {
         ChildProcessConnection c4 = createConnection();
 
         ChildProcessRanking ranking = new ChildProcessRanking(4);
+        ranking.enableServiceGroupImportance();
 
         // Insert in lowest ranked to highest ranked order.
         ranking.addConnection(c1, false /* foreground */, 0 /* frameDepth */,
@@ -132,6 +167,8 @@ public class ChildProcessRankingTest {
                 false /* intersectsViewport */, ChildProcessImportance.IMPORTANT);
 
         assertRankingAndRemoveAll(ranking, new ChildProcessConnection[] {c4, c3, c2, c1});
+        assertNotInGroup(new ChildProcessConnection[] {c4, c3, c2});
+        assertInGroupOrderedByImportance(new ChildProcessConnection[] {c1});
     }
 
     @Test
@@ -142,6 +179,7 @@ public class ChildProcessRankingTest {
         ChildProcessConnection c4 = createConnection();
 
         ChildProcessRanking ranking = new ChildProcessRanking(4);
+        ranking.enableServiceGroupImportance();
 
         // c1,2 are in one tab, and c3,4 are in second tab.
         ranking.addConnection(c1, true /* foreground */, 1 /* frameDepth */,
@@ -165,44 +203,8 @@ public class ChildProcessRankingTest {
                 true /* intersectsViewport */, ChildProcessImportance.NORMAL);
 
         assertRankingAndRemoveAll(ranking, new ChildProcessConnection[] {c4, c3, c2, c1});
-    }
-
-    @Test
-    public void testStability() {
-        ChildProcessConnection c1 = createConnection();
-        ChildProcessConnection c2 = createConnection();
-        ChildProcessConnection c3 = createConnection();
-        ChildProcessConnection c4 = createConnection();
-
-        ChildProcessRanking ranking = new ChildProcessRanking(4);
-
-        // Each connection is its own tab.
-        ranking.addConnection(c1, true /* foreground */, 0 /* frameDepth */,
-                false /* intersectsViewport */, ChildProcessImportance.NORMAL);
-        ranking.addConnection(c2, false /* foreground */, 0 /* frameDepth */,
-                false /* intersectsViewport */, ChildProcessImportance.NORMAL);
-        ranking.addConnection(c3, false /* foreground */, 0 /* frameDepth */,
-                false /* intersectsViewport */, ChildProcessImportance.NORMAL);
-        ranking.addConnection(c4, false /* foreground */, 0 /* frameDepth */,
-                false /* intersectsViewport */, ChildProcessImportance.NORMAL);
-
-        // Tab through each connection.
-        ranking.updateConnection(c2, true /* foreground */, 0 /* frameDepth */,
-                false /* intersectsViewport */, ChildProcessImportance.NORMAL);
-        ranking.updateConnection(c1, false /* foreground */, 0 /* frameDepth */,
-                false /* intersectsViewport */, ChildProcessImportance.NORMAL);
-
-        ranking.updateConnection(c3, true /* foreground */, 0 /* frameDepth */,
-                false /* intersectsViewport */, ChildProcessImportance.NORMAL);
-        ranking.updateConnection(c2, false /* foreground */, 0 /* frameDepth */,
-                false /* intersectsViewport */, ChildProcessImportance.NORMAL);
-
-        ranking.updateConnection(c4, true /* foreground */, 0 /* frameDepth */,
-                false /* intersectsViewport */, ChildProcessImportance.NORMAL);
-        ranking.updateConnection(c3, false /* foreground */, 0 /* frameDepth */,
-                false /* intersectsViewport */, ChildProcessImportance.NORMAL);
-
-        assertRankingAndRemoveAll(ranking, new ChildProcessConnection[] {c4, c3, c2, c1});
+        assertNotInGroup(new ChildProcessConnection[] {c4, c3});
+        assertInGroupOrderedByImportance(new ChildProcessConnection[] {c2, c1});
     }
 
     @Test
@@ -212,6 +214,7 @@ public class ChildProcessRankingTest {
         ChildProcessConnection c3 = createConnection();
 
         ChildProcessRanking ranking = new ChildProcessRanking(4);
+        ranking.enableServiceGroupImportance();
 
         // Insert in lowest ranked to highest ranked order.
         ranking.addConnection(c1, true /* foreground */, 1 /* frameDepth */,
@@ -222,6 +225,8 @@ public class ChildProcessRankingTest {
                 true /* intersectsViewport */, ChildProcessImportance.NORMAL);
 
         assertRankingAndRemoveAll(ranking, new ChildProcessConnection[] {c3, c2, c1});
+        assertNotInGroup(new ChildProcessConnection[] {c3, c2});
+        assertInGroupOrderedByImportance(new ChildProcessConnection[] {c1});
     }
 
     @Test