Skip to content

Commit

Permalink
Discard Occluded Bitmaps on Android
Browse files Browse the repository at this point in the history
There a few places that don't release their bitmaps upon being occluded
or offscreened. These contribute significantly to Chrome's memory and
aren't needed when the feature/surface they are associated isn't in use.

The memory remains resident even after the feature is dismissed.
Re-triggering the feature would free the memory so it isn't a leak.
However, profiling suggests use of these features ablates the
browser process memory by ~5 MB until restarted.

This primarily affects:
- Tab Selection Editor
- Tab Grid Dialog
- Tab Group Bottom Toolbar

Add a flag that drops the bitmaps in these scenarios upon the
corresponding surface no longer being required. We should measure the
impact of this in experiments as this may increase jank in some cases.

Bug: 1366128
Change-Id: Iaecc971b21c50e28f76d83a169b155b75fffdaba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3910189
Reviewed-by: Theresa Sullivan <twellington@chromium.org>
Commit-Queue: Calder Kitagawa <ckitagawa@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1054752}
  • Loading branch information
ckitagawa-work authored and Chromium LUCI CQ committed Oct 4, 2022
1 parent 0604ab2 commit 9fe443b
Show file tree
Hide file tree
Showing 18 changed files with 149 additions and 24 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.chromium.base.supplier.ObservableSupplierImpl;
import org.chromium.base.supplier.Supplier;
import org.chromium.chrome.browser.compositor.layouts.content.TabContentManager;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.share.ShareDelegate;
import org.chromium.chrome.browser.tab.Tab;
import org.chromium.chrome.browser.tabmodel.TabCreatorManager;
Expand Down Expand Up @@ -193,6 +194,11 @@ public void prepareDialog() {
@Override
public void postHiding() {
mTabListCoordinator.postHiding();
if (ChromeFeatureList.sDiscardOccludedBitmaps.isEnabled()) {
// TODO(crbug/1366128): This shouldn't be required if resetWithListOfTabs(null) is
// called. Find out why this helps and fix upstream if possible.
mTabListCoordinator.softCleanup();
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.chromium.base.Callback;
import org.chromium.base.metrics.RecordUserAction;
import org.chromium.base.supplier.Supplier;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.share.ChromeShareExtras;
import org.chromium.chrome.browser.share.ShareDelegate;
import org.chromium.chrome.browser.share.ShareDelegate.ShareOrigin;
Expand Down Expand Up @@ -344,6 +345,10 @@ void hideDialog(boolean showAnimation) {
public void finishedHidingDialogView() {
mDialogController.resetWithListOfTabs(null);
mDialogController.postHiding();
if (ChromeFeatureList.sDiscardOccludedBitmaps.isEnabled()) {
// Purge the bitmap reference in the animation.
mModel.set(TabGridPanelProperties.ANIMATION_SOURCE_VIEW, null);
}
}

void onReset(@Nullable List<Tab> tabs) {
Expand All @@ -370,6 +375,10 @@ void onReset(@Nullable List<Tab> tabs) {
} else if (mModel.get(TabGridPanelProperties.IS_DIALOG_VISIBLE)) {
mModel.set(TabGridPanelProperties.IS_DIALOG_VISIBLE, false);
mDialogController.postHiding();
if (ChromeFeatureList.sDiscardOccludedBitmaps.isEnabled()) {
// Purge the bitmap reference in the animation.
mModel.set(TabGridPanelProperties.ANIMATION_SOURCE_VIEW, null);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@

import org.chromium.base.ApiCompatibilityUtils;
import org.chromium.base.Callback;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.tab_ui.R;
import org.chromium.components.browser_ui.widget.animation.Interpolators;
import org.chromium.components.browser_ui.widget.scrim.ScrimCoordinator;
Expand Down Expand Up @@ -350,6 +351,10 @@ void setupDialogAnimation(View sourceView) {
mHideDialogAnimation.play(hideAnimator);
mHideDialogAnimation.removeAllListeners();
mHideDialogAnimation.addListener(mHideDialogAnimationListener);

if (ChromeFeatureList.sDiscardOccludedBitmaps.isEnabled()) {
updateAnimationCardView(null);
}
return;
}

Expand Down Expand Up @@ -665,6 +670,17 @@ void updateDialogWithOrientation(int orientation) {
}

private void updateAnimationCardView(View view) {
if (view == null) {
((ImageView) mAnimationCardView.findViewById(R.id.tab_favicon)).setImageDrawable(null);
((TextView) (mAnimationCardView.findViewById(R.id.tab_title))).setText("");
((ImageView) (mAnimationCardView.findViewById(R.id.tab_thumbnail)))
.setImageDrawable(null);
((ImageView) mAnimationCardView.findViewById(R.id.action_button))
.setImageDrawable(null);
mAnimationCardView.findViewById(R.id.background_view).setBackground(null);
return;
}

// Update the dummy animation card view with the actual item view from grid tab switcher
// recyclerView.
FrameLayout.LayoutParams params =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,25 @@
class TabSelectionEditorCoordinator {
static final String COMPONENT_NAME = "TabSelectionEditor";

// TODO(977271): Unify similar interfaces in other components that used the TabListCoordinator.
/**
* Interface for resetting the selectable tab grid.
*/
interface ResetHandler {
/**
* Handles the reset event.
* @param tabs List of {@link Tab}s to reset.
* @param preSelectedCount First {@code preSelectedCount} {@code tabs} are pre-selected.
* @param quickMode whether to use quick mode.
*/
void resetWithListOfTabs(@Nullable List<Tab> tabs, int preSelectedCount, boolean quickMode);

/**
* Handles cleanup.
*/
void postHiding();
}

/**
* An interface to control the TabSelectionEditor.
*/
Expand Down Expand Up @@ -206,8 +225,22 @@ public int getSpanSize(int i) {
mTabSelectionEditorLayoutChangeProcessor = PropertyModelChangeProcessor.create(
mModel, mTabSelectionEditorLayout, TabSelectionEditorLayoutBinder::bind, false);

ResetHandler resetHandler = new ResetHandler() {
@Override
public void resetWithListOfTabs(
@Nullable List<Tab> tabs, int preSelectedCount, boolean quickMode) {
TabSelectionEditorCoordinator.this.resetWithListOfTabs(
tabs, preSelectedCount, quickMode);
}

@Override
public void postHiding() {
mTabListCoordinator.postHiding();
mTabListCoordinator.softCleanup();
}
};
mTabSelectionEditorMediator = new TabSelectionEditorMediator(mContext,
mTabModelSelector, mTabListCoordinator, this::resetWithListOfTabs, mModel,
mTabModelSelector, mTabListCoordinator, resetHandler, mModel,
mSelectionDelegate, mTabSelectionEditorLayout.getToolbar(), displayGroups);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,24 +42,10 @@
class TabSelectionEditorMediator
implements TabSelectionEditorCoordinator.TabSelectionEditorController,
TabSelectionEditorAction.ActionDelegate {
// TODO(977271): Unify similar interfaces in other components that used the TabListCoordinator.
/**
* Interface for resetting the selectable tab grid.
*/
interface ResetHandler {
/**
* Handles the reset event.
* @param tabs List of {@link Tab}s to reset.
* @param preSelectedCount First {@code preSelectedCount} {@code tabs} are pre-selected.
* @param quickMode whether to use quick mode.
*/
void resetWithListOfTabs(@Nullable List<Tab> tabs, int preSelectedCount, boolean quickMode);
}

private final Context mContext;
private final TabModelSelector mTabModelSelector;
private final TabListCoordinator mTabListCoordinator;
private final ResetHandler mResetHandler;
private final TabSelectionEditorCoordinator.ResetHandler mResetHandler;
private final PropertyModel mModel;
private final SelectionDelegate<Integer> mSelectionDelegate;
private final TabSelectionEditorToolbar mTabSelectionEditorToolbar;
Expand Down Expand Up @@ -99,7 +85,8 @@ public void onClick(View v) {
};

TabSelectionEditorMediator(Context context, TabModelSelector tabModelSelector,
TabListCoordinator tabListCoordinator, ResetHandler resetHandler, PropertyModel model,
TabListCoordinator tabListCoordinator,
TabSelectionEditorCoordinator.ResetHandler resetHandler, PropertyModel model,
SelectionDelegate<Integer> selectionDelegate,
TabSelectionEditorToolbar tabSelectionEditorToolbar, boolean actionOnRelatedTabs) {
mContext = context;
Expand Down Expand Up @@ -309,6 +296,9 @@ public void hide() {
mVisibleTabs.clear();
mResetHandler.resetWithListOfTabs(null, 0, /*quickMode=*/false);
mModel.set(TabSelectionEditorProperties.IS_VISIBLE, false);
if (ChromeFeatureList.sDiscardOccludedBitmaps.isEnabled()) {
mResetHandler.postHiding();
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertTrue;

import static org.chromium.chrome.browser.flags.ChromeFeatureList.DISCARD_OCCLUDED_BITMAPS;
import static org.chromium.chrome.browser.flags.ChromeFeatureList.GRID_TAB_SWITCHER_FOR_TABLETS;
import static org.chromium.chrome.browser.flags.ChromeFeatureList.TAB_GRID_LAYOUT_ANDROID;
import static org.chromium.chrome.browser.flags.ChromeFeatureList.TAB_GROUPS_ANDROID;
Expand Down Expand Up @@ -127,7 +128,8 @@
@CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE})
@Restriction({Restriction.RESTRICTION_TYPE_NON_LOW_END_DEVICE})
@Features.EnableFeatures({TAB_GRID_LAYOUT_ANDROID, TAB_GROUPS_ANDROID,
TAB_GROUPS_FOR_TABLETS, GRID_TAB_SWITCHER_FOR_TABLETS, TAB_STRIP_IMPROVEMENTS})
TAB_GROUPS_FOR_TABLETS, GRID_TAB_SWITCHER_FOR_TABLETS, TAB_STRIP_IMPROVEMENTS,
DISCARD_OCCLUDED_BITMAPS})
public class TabGridDialogTest {
// clang-format on
private static final String CUSTOMIZED_TITLE1 = "wfh tips";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

import static org.chromium.base.test.util.Restriction.RESTRICTION_TYPE_LOW_END_DEVICE;
import static org.chromium.base.test.util.Restriction.RESTRICTION_TYPE_NON_LOW_END_DEVICE;
import static org.chromium.chrome.browser.flags.ChromeFeatureList.DISCARD_OCCLUDED_BITMAPS;
import static org.chromium.chrome.browser.flags.ChromeFeatureList.GRID_TAB_SWITCHER_FOR_TABLETS;
import static org.chromium.chrome.browser.flags.ChromeFeatureList.TAB_GROUPS_ANDROID;
import static org.chromium.chrome.browser.flags.ChromeFeatureList.TAB_GROUPS_FOR_TABLETS;
Expand Down Expand Up @@ -107,11 +108,12 @@
/**
* End-to-end test for TabSelectionEditor.
*/
// clang-format off
@RunWith(ChromeJUnit4ClassRunner.class)
@CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE, "force-fieldtrials=Study/Group",
"force-fieldtrial-params=Study.Group:enable_launch_polish/true"})
@EnableFeatures({TAB_GROUPS_ANDROID, GRID_TAB_SWITCHER_FOR_TABLETS + "<Study",
TAB_STRIP_IMPROVEMENTS, TAB_GROUPS_FOR_TABLETS})
TAB_STRIP_IMPROVEMENTS, TAB_GROUPS_FOR_TABLETS, DISCARD_OCCLUDED_BITMAPS})
@DisableFeatures(TAB_TO_GTS_ANIMATION)
@Batch(Batch.PER_CLASS)
public class TabSelectionEditorTest {
Expand All @@ -123,6 +125,7 @@ public class TabSelectionEditorTest {
"/chrome/test/data/android/share/link_share_http_canonical.html";
private static final String PAGE_WITH_NO_CANONICAL_URL =
"/chrome/test/data/android/share/link_share_no_canonical.html";
// clang-format on

@ClassRule
public static ChromeTabbedActivityTestRule sActivityTestRule =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -748,6 +748,35 @@ public void hideDialog_WithVisibilityListener_BasicAnimation() {
verifyZeroInteractions(mDialogController);
}

@Test
public void hideDialog_FadeOutAnimation_ClearsViewAnimation() {
// Mock that the animation source view is set, and the dialog is showing.
mModel.set(TabGridPanelProperties.ANIMATION_SOURCE_VIEW, mView);
mModel.set(TabGridPanelProperties.IS_DIALOG_VISIBLE, true);

mMediator.hideDialog(false);

// Animation source view should not be specified.
assertThat(mModel.get(TabGridPanelProperties.ANIMATION_SOURCE_VIEW), equalTo(null));
verify(mDialogController).resetWithListOfTabs(eq(null));
}

@Test
public void hideDialog_WithVisibilityListener_ClearsViewAnimation() {
// Mock that the animation source view exists, and the dialog is showing.
mModel.set(TabGridPanelProperties.ANIMATION_SOURCE_VIEW, mView);
mModel.set(TabGridPanelProperties.IS_DIALOG_VISIBLE, true);
// Set visibility listener.
mModel.set(TabGridPanelProperties.VISIBILITY_LISTENER, mMediator);

mMediator.hideDialog(false);

// Animation source view should not be specified.
assertThat(mModel.get(TabGridPanelProperties.ANIMATION_SOURCE_VIEW), equalTo(null));
assertThat(mModel.get(TabGridPanelProperties.IS_DIALOG_VISIBLE), equalTo(false));
verifyZeroInteractions(mDialogController);
}

@Test
public void hideDialog_ZoomOutAnimation() {
// Mock that the animation source view is null, and the dialog is showing.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ public void cacheNativeFlags() {
add(ChromeFeatureList.sCreateSafebrowsingOnStartup);
add(ChromeFeatureList.sCriticalPersistedTabData);
add(ChromeFeatureList.sDiscoverMultiColumn);
add(ChromeFeatureList.sDiscardOccludedBitmaps);
add(ChromeFeatureList.sDownloadsAutoResumptionNative);
add(ChromeFeatureList.sEarlyLibraryLoad);
add(ChromeFeatureList.sElasticOverscroll);
Expand Down
6 changes: 6 additions & 0 deletions chrome/browser/about_flags.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9618,6 +9618,12 @@ const FeatureEntry kFeatureEntries[] = {
kOsAndroid | kOsDesktop,
FEATURE_VALUE_TYPE(features::kEnableWebUsbOnExtensionServiceWorker)},

#if BUILDFLAG(IS_ANDROID)
{"discard-occluded-bitmaps", flag_descriptions::kDiscardOccludedBitmapsName,
flag_descriptions::kDiscardOccludedBitmapsDescription, kOsAndroid,
FEATURE_VALUE_TYPE(chrome::android::kDiscardOccludedBitmaps)},
#endif

// NOTE: Adding a new flag requires adding a corresponding entry to enum
// "LoginCustomFlags" in tools/metrics/histograms/enums.xml. See "Flag
// Histograms" in tools/metrics/histograms/README.md (run the
Expand Down
5 changes: 5 additions & 0 deletions chrome/browser/flag-metadata.json
Original file line number Diff line number Diff line change
Expand Up @@ -1475,6 +1475,11 @@
// This is a web-developer facing flag and should not be removed.
"expiry_milestone": -1
},
{
"name": "discard-occluded-bitmaps",
"owners": [ "ckitagawa" ],
"expiry_milestone": 112
},
{
"name": "display-alignment-assistance",
"owners": [ "cros-peripherals@google.com" ],
Expand Down
6 changes: 6 additions & 0 deletions chrome/browser/flag_descriptions.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3529,6 +3529,12 @@ const char kChromeSharingHubLaunchAdjacentDescription[] =
"In multi-window mode, launches share hub actions in an adjacent window. "
"For internal debugging.";

const char kDiscardOccludedBitmapsName[] =
"Discard occluded bitmaps in Android UI";
const char kDiscardOccludedBitmapsDescription[] =
"Proactively discard cached bitmaps that are occluded/offscreen. Applies "
"to several UI surfaces. May introduce jank if the bitmap is needed again.";

const char kEnableCbdSignOutName[] =
"Decouple Sign out from clearing browsing data";
const char kEnableCbdSignOutDescription[] =
Expand Down
3 changes: 3 additions & 0 deletions chrome/browser/flag_descriptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -1988,6 +1988,9 @@ extern const char kChromeSharingCrowLaunchTabDescription[];
extern const char kChromeSharingHubLaunchAdjacentName[];
extern const char kChromeSharingHubLaunchAdjacentDescription[];

extern const char kDiscardOccludedBitmapsName[];
extern const char kDiscardOccludedBitmapsDescription[];

extern const char kEnableCbdSignOutName[];
extern const char kEnableCbdSignOutDescription[];

Expand Down
5 changes: 5 additions & 0 deletions chrome/browser/flags/android/chrome_feature_list.cc
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@ const base::Feature* const kFeaturesExposedToJava[] = {
&kCCTResourcePrefetch,
&kCCTShowAboutBlankUrl,
&kCCTToolbarCustomizations,
&kDiscardOccludedBitmaps,
&kDontAutoHideBrowserControls,
&kCacheDeprecatedSystemLocationSetting,
&kChromeNewDownloadTab,
Expand Down Expand Up @@ -628,6 +629,10 @@ BASE_FEATURE(kCCTToolbarCustomizations,
"CCTToolbarCustomizations",
base::FEATURE_ENABLED_BY_DEFAULT);

BASE_FEATURE(kDiscardOccludedBitmaps,
"DiscardOccludedBitmaps",
base::FEATURE_DISABLED_BY_DEFAULT);

BASE_FEATURE(kDontAutoHideBrowserControls,
"DontAutoHideBrowserControls",
base::FEATURE_DISABLED_BY_DEFAULT);
Expand Down
1 change: 1 addition & 0 deletions chrome/browser/flags/android/chrome_feature_list.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ BASE_DECLARE_FEATURE(kCCTResourcePrefetch);
BASE_DECLARE_FEATURE(kCCTRetainingState);
BASE_DECLARE_FEATURE(kCCTShowAboutBlankUrl);
BASE_DECLARE_FEATURE(kCCTToolbarCustomizations);
BASE_DECLARE_FEATURE(kDiscardOccludedBitmaps);
BASE_DECLARE_FEATURE(kDontAutoHideBrowserControls);
BASE_DECLARE_FEATURE(kCacheDeprecatedSystemLocationSetting);
BASE_DECLARE_FEATURE(kChromeNewDownloadTab);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,7 @@ public static boolean getFieldTrialParamByFeatureAsBoolean(
public static final String CCT_SHOW_ABOUT_BLANK_URL = "CCTShowAboutBlankUrl";
public static final String CCT_TOOLBAR_CUSTOMIZATIONS = "CCTToolbarCustomizations";
public static final String CLOSE_TAB_SUGGESTIONS = "CloseTabSuggestions";
public static final String DISCARD_OCCLUDED_BITMAPS = "DiscardOccludedBitmaps";
public static final String DONT_AUTO_HIDE_BROWSER_CONTROLS = "DontAutoHideBrowserControls";
public static final String CHROME_NEW_DOWNLOAD_TAB = "ChromeNewDownloadTab";
public static final String CHROME_SHARE_LONG_SCREENSHOT = "ChromeShareLongScreenshot";
Expand Down Expand Up @@ -671,6 +672,8 @@ public static boolean getFieldTrialParamByFeatureAsBoolean(
new CachedFlag(CREATE_SAFEBROWSING_ON_STARTUP, false);
public static final CachedFlag sCriticalPersistedTabData =
new CachedFlag(CRITICAL_PERSISTED_TAB_DATA, false);
public static final CachedFlag sDiscardOccludedBitmaps =
new CachedFlag(DISCARD_OCCLUDED_BITMAPS, false);
public static final CachedFlag sDownloadsAutoResumptionNative =
new CachedFlag(DOWNLOADS_AUTO_RESUMPTION_NATIVE, true);
public static final CachedFlag sEarlyLibraryLoad = new CachedFlag(EARLY_LIBRARY_LOAD, true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import android.view.View;

import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.toolbar.R;
import org.chromium.ui.modelutil.PropertyKey;
import org.chromium.ui.modelutil.PropertyModel;
Expand Down Expand Up @@ -39,14 +40,18 @@ static void bind(PropertyModel model, ViewHolder view, PropertyKey propertyKey)
model.get(BottomControlsProperties.BOTTOM_CONTROLS_CONTAINER_HEIGHT_PX);
} else if (BottomControlsProperties.Y_OFFSET == propertyKey) {
view.sceneLayer.setYOffset(model.get(BottomControlsProperties.Y_OFFSET));
} else if (BottomControlsProperties.ANDROID_VIEW_VISIBLE == propertyKey) {
view.root.setVisibility(model.get(BottomControlsProperties.ANDROID_VIEW_VISIBLE)
? View.VISIBLE
: View.INVISIBLE);
} else if (BottomControlsProperties.COMPOSITED_VIEW_VISIBLE == propertyKey) {
} else if (BottomControlsProperties.ANDROID_VIEW_VISIBLE == propertyKey
|| BottomControlsProperties.COMPOSITED_VIEW_VISIBLE == propertyKey) {
final boolean showAndroidView =
model.get(BottomControlsProperties.ANDROID_VIEW_VISIBLE);
final boolean showCompositedView =
model.get(BottomControlsProperties.COMPOSITED_VIEW_VISIBLE);
view.root.setVisibility(showAndroidView ? View.VISIBLE : View.INVISIBLE);
view.sceneLayer.setIsVisible(showCompositedView);
if (!showAndroidView && !showCompositedView
&& ChromeFeatureList.sDiscardOccludedBitmaps.isEnabled()) {
view.root.getResourceAdapter().dropCachedBitmap();
}
} else if (BottomControlsProperties.IS_OBSCURED == propertyKey) {
view.root.setImportantForAccessibility(model.get(BottomControlsProperties.IS_OBSCURED)
? View.IMPORTANT_FOR_ACCESSIBILITY_NO_HIDE_DESCENDANTS
Expand Down
Loading

0 comments on commit 9fe443b

Please sign in to comment.