Optimize Skia Gold Init And Auth

Optimizes the Android Skia Gold code in the following ways:

1. Adds an "Initialize" step and shuffles arguments around so that
   goldctl only has to initialize the working directory once
   instead of every time a comparison is performed, saving ~250ms
   per comparison.
2. Because #1 causes each session to only be valid for a single
   instance/corpus/keys_file combination, adds
   SkiaGoldSessionManager to lazily create sessions as necessary.
3. Short circuits the "Authenticate" step if the session is already
   authenticated, saving ~40ms per comparison.

Change-Id: I72677ec8b0a0a3757ce198e8b8e1d34be7b74257
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2148153Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org>
Reviewed-by: Peter Conn <peconn@chromium.org>
Commit-Queue: Brian Sheedy <bsheedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#759703}

build/android/pylib/local/device/local_device_instrumentation_test_run.py

@@ -141,6 +141,7 @@ class LocalDeviceInstrumentationTestRun(
     self._flag_changers = {}
     self._shared_prefs_to_restore = []
     self._skia_gold_work_dir = None
+    self._skia_gold_session_manager = None
 
   #override
   def TestPackage(self):
@@ -370,6 +371,8 @@ class LocalDeviceInstrumentationTestRun(
     # expectations can be re-used between tests, saving a significant amount
     # of time.
     self._skia_gold_work_dir = tempfile.mkdtemp()
+    self._skia_gold_session_manager = gold_utils.SkiaGoldSessionManager(
+        self._skia_gold_work_dir, self._test_instance.skia_gold_properties)
     if self._test_instance.wait_for_java_debugger:
       apk = self._test_instance.apk_under_test or self._test_instance.test_apk
       logging.warning('*' * 80)
@@ -381,6 +384,7 @@ class LocalDeviceInstrumentationTestRun(
   def TearDown(self):
     shutil.rmtree(self._skia_gold_work_dir)
     self._skia_gold_work_dir = None
+    self._skia_gold_session_manager = None
     # By default, teardown will invoke ADB. When receiving SIGTERM due to a
     # timeout, there's a high probability that ADB is non-responsive. In these
     # cases, sending an ADB command will potentially take a long time to time
@@ -923,8 +927,6 @@ class LocalDeviceInstrumentationTestRun(
 
     gold_properties = self._test_instance.skia_gold_properties
     with tempfile_ext.NamedTemporaryDirectory() as host_dir:
-      gold_session = gold_utils.SkiaGoldSession(
-          working_dir=self._skia_gold_work_dir, gold_properties=gold_properties)
       use_luci = not (gold_properties.local_pixel_tests
                       or gold_properties.no_luci_auth)
 
@@ -948,9 +950,11 @@ class LocalDeviceInstrumentationTestRun(
               'when doing Skia Gold comparison.' % image_name)
           continue
 
+        gold_session = self._skia_gold_session_manager.GetSkiaGoldSession(
+            keys_file=json_path)
+
         status, error = gold_session.RunComparison(
             name=render_name,
-            keys_file=json_path,
             png_file=image_path,
             output_manager=self._env.output_manager,
             use_luci=use_luci)
@@ -966,6 +970,9 @@ class LocalDeviceInstrumentationTestRun(
         if status == status_codes.AUTH_FAILURE:
           _AppendToLog(results,
                        'Gold authentication failed with output %s' % error)
+        elif status == status_codes.INIT_FAILURE:
+          _AppendToLog(results,
+                       'Gold initialization failed with output %s' % error)
         elif status == status_codes.COMPARISON_FAILURE_REMOTE:
           triage_link = gold_session.GetTriageLink(render_name)
           if not triage_link:

chrome/android/javatests/src/org/chromium/chrome/browser/vr/VrBrowserDialogTest.java

@@ -7,7 +7,6 @@ package org.chromium.chrome.browser.vr;
 import static org.chromium.chrome.browser.vr.XrTestFramework.PAGE_LOAD_TIMEOUT_S;
 import static org.chromium.chrome.browser.vr.XrTestFramework.POLL_TIMEOUT_LONG_MS;
 import static org.chromium.chrome.browser.vr.XrTestFramework.POLL_TIMEOUT_SHORT_MS;
-import static org.chromium.chrome.browser.vr.XrTestFramework.VR_SKIA_GOLD_CORPUS;
 import static org.chromium.chrome.test.util.ChromeRestriction.RESTRICTION_TYPE_VIEWER_DAYDREAM;
 import static org.chromium.chrome.test.util.ChromeRestriction.RESTRICTION_TYPE_VIEWER_DAYDREAM_OR_STANDALONE;
 
@@ -55,7 +54,9 @@ public class VrBrowserDialogTest {
 
     @Rule
     public RenderTestRule mRenderTestRule =
-            new RenderTestRule.SkiaGoldBuilder().setCorpus(VR_SKIA_GOLD_CORPUS).build();
+            new RenderTestRule.SkiaGoldBuilder()
+                    .setCorpus(RenderTestRule.Corpus.ANDROID_VR_RENDER_TESTS)
+                    .build();
 
     private VrBrowserTestFramework mVrBrowserTestFramework;
 

chrome/android/javatests/src/org/chromium/chrome/browser/vr/VrBrowserNativeUiTest.java

@@ -7,7 +7,6 @@ package org.chromium.chrome.browser.vr;
 import static org.chromium.chrome.browser.vr.XrTestFramework.NATIVE_URLS_OF_INTEREST;
 import static org.chromium.chrome.browser.vr.XrTestFramework.PAGE_LOAD_TIMEOUT_S;
 import static org.chromium.chrome.browser.vr.XrTestFramework.POLL_TIMEOUT_LONG_MS;
-import static org.chromium.chrome.browser.vr.XrTestFramework.VR_SKIA_GOLD_CORPUS;
 import static org.chromium.chrome.test.util.ChromeRestriction.RESTRICTION_TYPE_VIEWER_DAYDREAM_OR_STANDALONE;
 
 import android.graphics.PointF;
@@ -58,7 +57,9 @@ public class VrBrowserNativeUiTest {
 
     @Rule
     public RenderTestRule mRenderTestRule =
-            new RenderTestRule.SkiaGoldBuilder().setCorpus(VR_SKIA_GOLD_CORPUS).build();
+            new RenderTestRule.SkiaGoldBuilder()
+                    .setCorpus(RenderTestRule.Corpus.ANDROID_VR_RENDER_TESTS)
+                    .build();
 
     private VrBrowserTestFramework mVrBrowserTestFramework;
 

chrome/android/javatests/src/org/chromium/chrome/browser/vr/VrBrowserNavigationTest.java

@@ -8,7 +8,6 @@ import static org.chromium.chrome.browser.vr.XrTestFramework.NATIVE_URLS_OF_INTE
 import static org.chromium.chrome.browser.vr.XrTestFramework.PAGE_LOAD_TIMEOUT_S;
 import static org.chromium.chrome.browser.vr.XrTestFramework.POLL_TIMEOUT_LONG_MS;
 import static org.chromium.chrome.browser.vr.XrTestFramework.POLL_TIMEOUT_SHORT_MS;
-import static org.chromium.chrome.browser.vr.XrTestFramework.VR_SKIA_GOLD_CORPUS;
 import static org.chromium.chrome.test.util.ChromeRestriction.RESTRICTION_TYPE_VIEWER_DAYDREAM;
 import static org.chromium.chrome.test.util.ChromeRestriction.RESTRICTION_TYPE_VIEWER_DAYDREAM_OR_STANDALONE;
 
@@ -78,7 +77,9 @@ public class VrBrowserNavigationTest {
 
     @Rule
     public RenderTestRule mRenderTestRule =
-            new RenderTestRule.SkiaGoldBuilder().setCorpus(VR_SKIA_GOLD_CORPUS).build();
+            new RenderTestRule.SkiaGoldBuilder()
+                    .setCorpus(RenderTestRule.Corpus.ANDROID_VR_RENDER_TESTS)
+                    .build();
 
     private WebXrVrTestFramework mWebXrVrTestFramework;
     private VrBrowserTestFramework mVrBrowserTestFramework;

chrome/android/javatests/src/org/chromium/chrome/browser/vr/VrBrowserWebInputEditingTest.java

@@ -9,7 +9,6 @@ import static org.chromium.chrome.browser.vr.XrTestFramework.POLL_CHECK_INTERVAL
 import static org.chromium.chrome.browser.vr.XrTestFramework.POLL_CHECK_INTERVAL_SHORT_MS;
 import static org.chromium.chrome.browser.vr.XrTestFramework.POLL_TIMEOUT_LONG_MS;
 import static org.chromium.chrome.browser.vr.XrTestFramework.POLL_TIMEOUT_SHORT_MS;
-import static org.chromium.chrome.browser.vr.XrTestFramework.VR_SKIA_GOLD_CORPUS;
 import static org.chromium.chrome.test.util.ChromeRestriction.RESTRICTION_TYPE_VIEWER_DAYDREAM_OR_STANDALONE;
 
 import android.graphics.PointF;
@@ -60,7 +59,9 @@ public class VrBrowserWebInputEditingTest {
 
     @Rule
     public RenderTestRule mRenderTestRule =
-            new RenderTestRule.SkiaGoldBuilder().setCorpus(VR_SKIA_GOLD_CORPUS).build();
+            new RenderTestRule.SkiaGoldBuilder()
+                    .setCorpus(RenderTestRule.Corpus.ANDROID_VR_RENDER_TESTS)
+                    .build();
 
     private VrBrowserTestFramework mVrBrowserTestFramework;
 

chrome/android/javatests/src/org/chromium/chrome/browser/vr/XrTestFramework.java

@@ -62,8 +62,6 @@ public abstract class XrTestFramework {
     public static final int POLL_TIMEOUT_LONG_MS = 10000;
     public static final boolean DEBUG_LOGS = false;
 
-    public static final String VR_SKIA_GOLD_CORPUS = "android-vr-render-tests";
-
     // We need to make sure the port is constant, otherwise the URL changes between test runs, which
     // is really bad for image diff tests. There's nothing special about this port other than that
     // it shouldn't be in use by anything.

chrome/android/javatests/src/org/chromium/chrome/browser/vr/jsdialog/VrBrowserJavaScriptModalDialogTest.java

@@ -7,7 +7,6 @@ package org.chromium.chrome.browser.vr.jsdialog;
 import static org.chromium.chrome.browser.vr.XrTestFramework.PAGE_LOAD_TIMEOUT_S;
 import static org.chromium.chrome.browser.vr.XrTestFramework.POLL_TIMEOUT_LONG_MS;
 import static org.chromium.chrome.browser.vr.XrTestFramework.POLL_TIMEOUT_SHORT_MS;
-import static org.chromium.chrome.browser.vr.XrTestFramework.VR_SKIA_GOLD_CORPUS;
 import static org.chromium.chrome.test.util.ChromeRestriction.RESTRICTION_TYPE_VIEWER_DAYDREAM;
 
 import android.support.test.filters.MediumTest;
@@ -47,7 +46,9 @@ public class VrBrowserJavaScriptModalDialogTest {
 
     @Rule
     public RenderTestRule mRenderTestRule =
-            new RenderTestRule.SkiaGoldBuilder().setCorpus(VR_SKIA_GOLD_CORPUS).build();
+            new RenderTestRule.SkiaGoldBuilder()
+                    .setCorpus(RenderTestRule.Corpus.ANDROID_VR_RENDER_TESTS)
+                    .build();
 
     private ChromeTabbedActivity mActivity;
     private VrBrowserTestFramework mVrBrowserTestFramework;

ui/android/javatests/src/org/chromium/ui/test/util/RENDER_TESTS.md

@@ -155,6 +155,14 @@ If you want to separate your baselines from the default `android-render-tests`
 corpus in Gold, you can call `setCorpus()` on your
 `SkiaGoldBuilder` instance before calling `build()`.
 
+**Note:** Each instance/corpus/description combination results in needing to
+create a new Gold session under the hood, which adds ~250 ms due to extra
+initialization the first time that combination is used in a particular test
+suite run. Instance and corpus are pretty constant, so the main culprit is the
+description. This overhead can be kept low by using identical descriptions in
+multiple test classes, including multiple test cases in a test class that has a
+description, or avoiding descriptions altogether.
+
 ### Running the tests locally
 
 When running instrumentation tests locally, pass the `--local-output` option to

ui/android/javatests/src/org/chromium/ui/test/util/RenderTestRule.java

@@ -16,6 +16,7 @@ import android.view.ViewGroup;
 import android.widget.EditText;
 import android.widget.ImageView;
 
+import androidx.annotation.StringDef;
 import androidx.vectordrawable.graphics.drawable.AnimatedVectorDrawableCompat;
 
 import org.json.JSONException;
@@ -35,6 +36,8 @@ import java.io.File;
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.PrintWriter;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashMap;
@@ -158,6 +161,15 @@ public class RenderTestRule extends TestWatcher {
     private int mSkiaGoldRevision;
     private String mSkiaGoldRevisionDescription;
 
+    @StringDef({Corpus.ANDROID_RENDER_TESTS, Corpus.ANDROID_VR_RENDER_TESTS})
+    @Retention(RetentionPolicy.SOURCE)
+    public @interface Corpus {
+        // Corpus for general use.
+        String ANDROID_RENDER_TESTS = "android-render-tests";
+        // Corpus for VR (virtual reality) features.
+        String ANDROID_VR_RENDER_TESTS = "android-vr-render-tests";
+    }
+
     /**
      * An exception thrown after a Render Test if images do not match the goldens or goldens are
      * missing on a render test device.
@@ -177,11 +189,14 @@ public class RenderTestRule extends TestWatcher {
     }
 
     // Skia Gold-specific constructor used by the builder.
-    protected RenderTestRule(int revision, String corpus, String description) {
+    // Note that each corpus/description combination results in some additional initialization
+    // on the host (~250 ms), so consider whether adding unique descriptions is necessary before
+    // adding them to a bunch of test classes.
+    protected RenderTestRule(int revision, @Corpus String corpus, String description) {
         assert revision >= 0;
 
         mUseSkiaGold = true;
-        mSkiaGoldCorpus = (corpus == null) ? "android-render-tests" : corpus;
+        mSkiaGoldCorpus = (corpus == null) ? Corpus.ANDROID_RENDER_TESTS : corpus;
         mSkiaGoldRevisionDescription = description;
         mSkiaGoldRevision = revision;
 
@@ -500,7 +515,7 @@ public class RenderTestRule extends TestWatcher {
      */
     public static class SkiaGoldBuilder {
         private int mRevision;
-        private String mCorpus;
+        private @Corpus String mCorpus;
         private String mDescription;
 
         public SkiaGoldBuilder setRevision(int revision) {
@@ -508,7 +523,7 @@ public class RenderTestRule extends TestWatcher {
             return this;
         }
 
-        public SkiaGoldBuilder setCorpus(String corpus) {
+        public SkiaGoldBuilder setCorpus(@Corpus String corpus) {
             mCorpus = corpus;
             return this;
         }