Commit cb2f5900 authored by Wei-Yin Chen (陳威尹)'s avatar Wei-Yin Chen (陳威尹) Committed by Commit Bot

[Instant Start] Avoid pre-native GURL usage

Remove native dependency of
ReturnToChromeExperimentsUtil.shouldShowStartSurfaceAsTheHomePage()
and PseudoTab.readAllPseudoTabsFromStateFile() so that they can be
called without leading to native initialization.

Benchmarking results using http://crrev.com/c/2373287:

100 samples on Nexus 5X
                                Median  Diff with control   p-value
FirstDrawCompletedTime         414.0ms   -29.0ms (-7.00%)  0.000000
SingleTabTitleAvailableTime    287.0ms  -31.5ms (-10.98%)  0.000000
FeedStreamCreatedTime          836.5ms   -39.5ms (-4.72%)  0.000030
FeedContentFirstLoadedTime    1898.5ms    -0.5ms (-0.03%)  0.534189
FeedsLoadingPlaceholderShown   234.0ms    -2.0ms (-0.85%)  0.629761

100 samples on Pixel 2 XL
                                Median  Diff with control   p-value
FirstDrawCompletedTime         176.0ms  -18.0ms (-10.23%)  0.000000
SingleTabTitleAvailableTime    131.0ms  -14.0ms (-10.69%)  0.000000
FeedStreamCreatedTime          424.0ms   -31.0ms (-7.31%)  0.000180
FeedContentFirstLoadedTime    1137.0ms   -18.5ms (-1.63%)  0.075061
FeedsLoadingPlaceholderShown   107.0ms    -1.0ms (-0.93%)  0.718766

100 samples on Pixel 3 XL
                                Median  Diff with control   p-value
FirstDrawCompletedTime         155.0ms   -13.5ms (-8.71%)  0.000000
SingleTabTitleAvailableTime    117.0ms  -13.0ms (-11.11%)  0.000000
FeedStreamCreatedTime          356.0ms   -35.5ms (-9.97%)  0.000001
FeedsLoadingPlaceholderShown    94.5ms    -2.0ms (-2.12%)  0.007312
FeedContentFirstLoadedTime     924.0ms    -6.5ms (-0.70%)  0.034100

Bug: 1041865
Change-Id: I640aaeb6f2dae64b9152fd94d0cd354a487f5bf5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2372900Reviewed-by: default avatarAndrew Grieve <agrieve@chromium.org>
Reviewed-by: default avatarMichael Thiessen <mthiesse@chromium.org>
Reviewed-by: default avatarXi Han <hanxi@chromium.org>
Commit-Queue: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809226}
parent 840ffc16
...@@ -27,10 +27,10 @@ import org.chromium.base.NativeLibraryLoadedStatus; ...@@ -27,10 +27,10 @@ import org.chromium.base.NativeLibraryLoadedStatus;
import org.chromium.base.NativeLibraryLoadedStatus.NativeLibraryLoadedStatusProvider; import org.chromium.base.NativeLibraryLoadedStatus.NativeLibraryLoadedStatusProvider;
import org.chromium.base.StrictModeContext; import org.chromium.base.StrictModeContext;
import org.chromium.base.TraceEvent; import org.chromium.base.TraceEvent;
import org.chromium.base.annotations.CheckDiscard;
import org.chromium.base.annotations.JNINamespace; import org.chromium.base.annotations.JNINamespace;
import org.chromium.base.annotations.MainDex; import org.chromium.base.annotations.MainDex;
import org.chromium.base.annotations.NativeMethods; import org.chromium.base.annotations.NativeMethods;
import org.chromium.base.annotations.RemovableInRelease;
import org.chromium.base.compat.ApiHelperForM; import org.chromium.base.compat.ApiHelperForM;
import org.chromium.base.metrics.RecordHistogram; import org.chromium.base.metrics.RecordHistogram;
import org.chromium.base.metrics.UmaRecorderHolder; import org.chromium.base.metrics.UmaRecorderHolder;
...@@ -235,7 +235,7 @@ public class LibraryLoader { ...@@ -235,7 +235,7 @@ public class LibraryLoader {
return mUseModernLinker; return mUseModernLinker;
} }
@RemovableInRelease @CheckDiscard("Can't use @RemovableInRelease because Release build with DCHECK_IS_ON needs it")
public void enableJniChecks() { public void enableJniChecks() {
if (!BuildConfig.DCHECK_IS_ON) return; if (!BuildConfig.DCHECK_IS_ON) return;
......
...@@ -15,6 +15,7 @@ import static androidx.test.espresso.matcher.ViewMatchers.withText; ...@@ -15,6 +15,7 @@ import static androidx.test.espresso.matcher.ViewMatchers.withText;
import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertThat;
import static org.hamcrest.CoreMatchers.allOf; import static org.hamcrest.CoreMatchers.allOf;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.greaterThan;
import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.notNullValue;
...@@ -42,10 +43,13 @@ import org.hamcrest.core.AllOf; ...@@ -42,10 +43,13 @@ import org.hamcrest.core.AllOf;
import org.junit.Assert; import org.junit.Assert;
import org.junit.Rule; import org.junit.Rule;
import org.junit.Test; import org.junit.Test;
import org.junit.rules.ErrorCollector;
import org.junit.runner.RunWith; import org.junit.runner.RunWith;
import org.chromium.base.BuildConfig;
import org.chromium.base.Callback; import org.chromium.base.Callback;
import org.chromium.base.CommandLine; import org.chromium.base.CommandLine;
import org.chromium.base.NativeLibraryLoadedStatus;
import org.chromium.base.StreamUtil; import org.chromium.base.StreamUtil;
import org.chromium.base.library_loader.LibraryLoader; import org.chromium.base.library_loader.LibraryLoader;
import org.chromium.base.test.util.CommandLineFlags; import org.chromium.base.test.util.CommandLineFlags;
...@@ -124,6 +128,9 @@ public class InstantStartTest { ...@@ -124,6 +128,9 @@ public class InstantStartTest {
public ChromeRenderTestRule mRenderTestRule = public ChromeRenderTestRule mRenderTestRule =
ChromeRenderTestRule.Builder.withPublicCorpus().build(); ChromeRenderTestRule.Builder.withPublicCorpus().build();
@Rule
public ErrorCollector collector = new ErrorCollector();
/** /**
* Only launch Chrome without waiting for a current tab. * Only launch Chrome without waiting for a current tab.
* This test could not use {@link ChromeActivityTestRule#startMainActivityFromLauncher()} * This test could not use {@link ChromeActivityTestRule#startMainActivityFromLauncher()}
...@@ -811,6 +818,30 @@ public class InstantStartTest { ...@@ -811,6 +818,30 @@ public class InstantStartTest {
Assert.assertEquals(123, ((StaticLayout) activeLayout).getCurrentTabIdForTesting()); Assert.assertEquals(123, ((StaticLayout) activeLayout).getCurrentTabIdForTesting());
} }
@Test
@SmallTest
@Restriction({UiRestriction.RESTRICTION_TYPE_PHONE})
@EnableFeatures({ChromeFeatureList.START_SURFACE_ANDROID + "<Study"})
// clang-format off
@CommandLineFlags.Add({ChromeSwitches.DISABLE_NATIVE_INITIALIZATION,
"force-fieldtrials=Study/Group",
"force-fieldtrial-params=Study.Group:start_surface_variation/single"})
public void testNoGURLPreNative() {
// clang-format on
if (!BuildConfig.DCHECK_IS_ON) return;
collector.checkThat(StartSurfaceConfiguration.isStartSurfaceSinglePaneEnabled(), is(true));
collector.checkThat(TextUtils.isEmpty(HomepageManager.getHomepageUri()), is(false));
Assert.assertFalse(
NativeLibraryLoadedStatus.getProviderForTesting().areMainDexNativeMethodsReady());
ReturnToChromeExperimentsUtil.shouldShowStartSurfaceAsTheHomePage();
ReturnToChromeExperimentsUtil.shouldShowStartSurfaceAsTheHomePageNoTabs();
PseudoTab.getAllPseudoTabsFromStateFile();
Assert.assertFalse("There should be no GURL usages triggering native library loading",
NativeLibraryLoadedStatus.getProviderForTesting().areMainDexNativeMethodsReady());
}
/** /**
* Toggles the header and checks whether the header has the right status. * Toggles the header and checks whether the header has the right status.
* *
......
...@@ -14,7 +14,6 @@ import org.chromium.base.Log; ...@@ -14,7 +14,6 @@ import org.chromium.base.Log;
import org.chromium.base.StreamUtil; import org.chromium.base.StreamUtil;
import org.chromium.chrome.browser.flags.CachedFeatureFlags; import org.chromium.chrome.browser.flags.CachedFeatureFlags;
import org.chromium.chrome.browser.flags.ChromeFeatureList; import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.ntp.NewTabPage;
import org.chromium.chrome.browser.tab.Tab; import org.chromium.chrome.browser.tab.Tab;
import org.chromium.chrome.browser.tab.state.CriticalPersistedTabData; import org.chromium.chrome.browser.tab.state.CriticalPersistedTabData;
import org.chromium.chrome.browser.tabmodel.TabList; import org.chromium.chrome.browser.tabmodel.TabList;
...@@ -24,6 +23,7 @@ import org.chromium.chrome.browser.tabmodel.TabModelSelector; ...@@ -24,6 +23,7 @@ import org.chromium.chrome.browser.tabmodel.TabModelSelector;
import org.chromium.chrome.browser.tabmodel.TabPersistentStore; import org.chromium.chrome.browser.tabmodel.TabPersistentStore;
import org.chromium.chrome.browser.tabmodel.TabbedModeTabPersistencePolicy; import org.chromium.chrome.browser.tabmodel.TabbedModeTabPersistencePolicy;
import org.chromium.chrome.browser.tabpersistence.TabStateDirectory; import org.chromium.chrome.browser.tabpersistence.TabStateDirectory;
import org.chromium.chrome.browser.tasks.ReturnToChromeExperimentsUtil;
import org.chromium.chrome.browser.tasks.tab_management.TabUiFeatureUtilities; import org.chromium.chrome.browser.tasks.tab_management.TabUiFeatureUtilities;
import java.io.ByteArrayInputStream; import java.io.ByteArrayInputStream;
...@@ -347,7 +347,10 @@ public class PseudoTab { ...@@ -347,7 +347,10 @@ public class PseudoTab {
(index, id, url, isIncognito, isStandardActiveIndex, isIncognitoActiveIndex) (index, id, url, isIncognito, isStandardActiveIndex, isIncognitoActiveIndex)
-> { -> {
// Skip restoring of non-selected NTP to match the real restoration logic. // Skip restoring of non-selected NTP to match the real restoration logic.
if (NewTabPage.isNTPUrl(url) && !isStandardActiveIndex) return; if (ReturnToChromeExperimentsUtil.isCanonicalizedNTPUrl(url)
&& !isStandardActiveIndex) {
return;
}
PseudoTab tab = PseudoTab.fromTabId(id); PseudoTab tab = PseudoTab.fromTabId(id);
if (isStandardActiveIndex) { if (isStandardActiveIndex) {
assert sActiveTabFromStateFile == null; assert sActiveTabFromStateFile == null;
......
...@@ -22,7 +22,6 @@ import org.chromium.chrome.browser.flags.ChromeFeatureList; ...@@ -22,7 +22,6 @@ import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.flags.IntCachedFieldTrialParameter; import org.chromium.chrome.browser.flags.IntCachedFieldTrialParameter;
import org.chromium.chrome.browser.homepage.HomepageManager; import org.chromium.chrome.browser.homepage.HomepageManager;
import org.chromium.chrome.browser.locale.LocaleManager; import org.chromium.chrome.browser.locale.LocaleManager;
import org.chromium.chrome.browser.ntp.NewTabPage;
import org.chromium.chrome.browser.tab.TabLaunchType; import org.chromium.chrome.browser.tab.TabLaunchType;
import org.chromium.chrome.browser.tabmodel.TabModel; import org.chromium.chrome.browser.tabmodel.TabModel;
import org.chromium.chrome.browser.tabmodel.TabModelSelector; import org.chromium.chrome.browser.tabmodel.TabModelSelector;
...@@ -211,17 +210,11 @@ public final class ReturnToChromeExperimentsUtil { ...@@ -211,17 +210,11 @@ public final class ReturnToChromeExperimentsUtil {
return chromeActivity; return chromeActivity;
} }
/** public static boolean isCanonicalizedNTPUrl(String url) {
* TODO(crbug/1041865): avoid using GURL since {@link #shouldShowStartSurfaceAsTheHomePage()} // Avoid loading native library due to GURL usage since
* is in the critical path in Instant Start. // #shouldShowStartSurfaceAsTheHomePage() is in the critical path in Instant Start.
*/ return url.equals("chrome://newtab/") || url.equals("chrome-native://newtab/")
private static boolean isNTPUrl(String url) { || url.equals("about:newtab");
if (CachedFeatureFlags.isEnabled(ChromeFeatureList.INSTANT_START)) {
try (StrictModeContext ignored = StrictModeContext.allowDiskReads()) {
return NewTabPage.isNTPUrl(url);
}
}
return NewTabPage.isNTPUrl(url);
} }
/** /**
...@@ -270,7 +263,7 @@ public final class ReturnToChromeExperimentsUtil { ...@@ -270,7 +263,7 @@ public final class ReturnToChromeExperimentsUtil {
// accessibility is not enabled and not on tablet. // accessibility is not enabled and not on tablet.
String homePageUrl = HomepageManager.getHomepageUri(); String homePageUrl = HomepageManager.getHomepageUri();
return StartSurfaceConfiguration.isStartSurfaceSinglePaneEnabled() return StartSurfaceConfiguration.isStartSurfaceSinglePaneEnabled()
&& (TextUtils.isEmpty(homePageUrl) || isNTPUrl(homePageUrl)) && (TextUtils.isEmpty(homePageUrl) || isCanonicalizedNTPUrl(homePageUrl))
&& !ChromeAccessibilityUtil.get().isAccessibilityEnabled() && !ChromeAccessibilityUtil.get().isAccessibilityEnabled()
&& !DeviceFormFactor.isNonMultiDisplayContextOnTablet( && !DeviceFormFactor.isNonMultiDisplayContextOnTablet(
ContextUtils.getApplicationContext()); ContextUtils.getApplicationContext());
......
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 to comment