Skip to content

Commit

Permalink
Resolve jank in CriticalPersistedTabData
Browse files Browse the repository at this point in the history 
Unnecessary saves (i.e. saves when a setter is called but the underlying
value has not changed) will be avoided and as much of serialize() as
possible will be delegated to the background thread.

(cherry picked from commit 181dad7)

Bug: 1184273
Change-Id: I165812f463e82af8db3ed29255f71155032161fa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2733031
Commit-Queue: David Maunder <davidjm@chromium.org>
Reviewed-by: ssid <ssid@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#860370}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2742496
Cr-Commit-Position: refs/branch-heads/4430@{#236}
Cr-Branched-From: e5ce7dc-refs/heads/master@{#857950}
  • Loading branch information
David Maunder authored and Chromium LUCI CQ committed Mar 8, 2021
1 parent bfc0e5d commit de6efd7
Show file tree
Hide file tree
Showing 15 changed files with 460 additions and 87 deletions.
210 changes: 208 additions & 2 deletions ...oid/javatests/src/org/chromium/chrome/browser/tab/state/CriticalPersistedTabDataTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.chromium.chrome.browser.tab.MockTab;
import org.chromium.chrome.browser.tab.Tab;
import org.chromium.chrome.browser.tab.TabImpl;
import org.chromium.chrome.browser.tab.TabLaunchType;
import org.chromium.chrome.browser.tab.WebContentsState;
import org.chromium.chrome.test.ChromeBrowserTestRule;
import org.chromium.components.embedder_support.util.UrlConstants;
Expand Down Expand Up @@ -56,9 +57,29 @@ public class CriticalPersistedTabDataTest {
private static final String OPENER_APP_ID = "OpenerAppId";
private static final int THEME_COLOR = 5;
private static final Integer LAUNCH_TYPE_AT_CREATION = 3;
private static final String TITLE_A = "original title";
private static final String TITLE_B = "new title";
private static final GURL URL_A = new GURL("https://a.com");
private static final GURL URL_B = new GURL("https://b.com");
private static final int ROOT_ID_A = 42;
private static final int ROOT_ID_B = 1;
private static final int PARENT_ID_A = 43;
private static final int PARENT_ID_B = 2;
private static final long TIMESTAMP_A = 44;
private static final long TIMESTAMP_B = 3;
private static final @TabLaunchType Integer TAB_LAUNCH_TYPE_A = TabLaunchType.FROM_LINK;
private static final @TabLaunchType Integer TAB_LAUNCH_TYPE_B = TabLaunchType.FROM_EXTERNAL_APP;
private static final byte[] WEB_CONTENTS_STATE_A_BYTES = {4};
private static final byte[] WEB_CONTENTS_STATE_B_BYTES = {5, 6};
private static final WebContentsState WEB_CONTENTS_STATE_A =
new WebContentsState(ByteBuffer.allocateDirect(WEB_CONTENTS_STATE_A_BYTES.length));
private static final WebContentsState WEB_CONTENTS_STATE_B =
new WebContentsState(ByteBuffer.allocateDirect(WEB_CONTENTS_STATE_B_BYTES.length));

static {
WEB_CONTENTS_STATE.buffer().put(WEB_CONTENTS_STATE_BYTES);
WEB_CONTENTS_STATE_A.buffer().put(WEB_CONTENTS_STATE_A_BYTES);
WEB_CONTENTS_STATE_B.buffer().put(WEB_CONTENTS_STATE_B_BYTES);
}

private CriticalPersistedTabData mCriticalPersistedTabData;
Expand Down Expand Up @@ -235,7 +256,7 @@ public void testSerializationBug() throws InterruptedException {
CriticalPersistedTabData criticalPersistedTabData = new CriticalPersistedTabData(tab, "",
"", PARENT_ID, ROOT_ID, TIMESTAMP, WEB_CONTENTS_STATE, CONTENT_STATE_VERSION,
OPENER_APP_ID, THEME_COLOR, LAUNCH_TYPE_AT_CREATION);
byte[] serialized = criticalPersistedTabData.serialize();
byte[] serialized = criticalPersistedTabData.getSerializeSupplier().get();
PersistedTabDataConfiguration config = PersistedTabDataConfiguration.get(
ShoppingPersistedTabData.class, tab.isIncognito());
CriticalPersistedTabData deserialized =
Expand All @@ -261,11 +282,196 @@ public void testOpenerAppIdNull() {
CriticalPersistedTabData criticalPersistedTabData = new CriticalPersistedTabData(tab, "",
"", PARENT_ID, ROOT_ID, TIMESTAMP, WEB_CONTENTS_STATE, CONTENT_STATE_VERSION, null,
THEME_COLOR, LAUNCH_TYPE_AT_CREATION);
byte[] serialized = criticalPersistedTabData.serialize();
byte[] serialized = criticalPersistedTabData.getSerializeSupplier().get();
PersistedTabDataConfiguration config = PersistedTabDataConfiguration.get(
ShoppingPersistedTabData.class, tab.isIncognito());
CriticalPersistedTabData deserialized =
new CriticalPersistedTabData(tab, serialized, config.getStorage(), config.getId());
Assert.assertEquals(null, deserialized.getOpenerAppId());
}

@UiThreadTest
@SmallTest
@Test
public void testUrlSavedWhenNecessary() {
CriticalPersistedTabData spyCriticalPersistedTabData =
spy(CriticalPersistedTabData.from(mockTab(TAB_ID, false)));
spyCriticalPersistedTabData.setUrl(URL_A);
Assert.assertEquals(URL_A, spyCriticalPersistedTabData.getUrl());
verify(spyCriticalPersistedTabData, times(1)).save();

spyCriticalPersistedTabData.setUrl(URL_A);
Assert.assertEquals(URL_A, spyCriticalPersistedTabData.getUrl());
verify(spyCriticalPersistedTabData, times(1)).save();

spyCriticalPersistedTabData.setUrl(URL_B);
Assert.assertEquals(URL_B, spyCriticalPersistedTabData.getUrl());
verify(spyCriticalPersistedTabData, times(2)).save();

spyCriticalPersistedTabData.setUrl(URL_A);
Assert.assertEquals(URL_A, spyCriticalPersistedTabData.getUrl());
verify(spyCriticalPersistedTabData, times(3)).save();

spyCriticalPersistedTabData.setUrl(null);
Assert.assertNull(spyCriticalPersistedTabData.getUrl());
verify(spyCriticalPersistedTabData, times(4)).save();
}

@UiThreadTest
@SmallTest
@Test
public void testTitleSavedWhenNecessary() {
CriticalPersistedTabData spyCriticalPersistedTabData =
spy(CriticalPersistedTabData.from(mockTab(TAB_ID, false)));
spyCriticalPersistedTabData.setTitle(TITLE_A);
Assert.assertEquals(TITLE_A, spyCriticalPersistedTabData.getTitle());
verify(spyCriticalPersistedTabData, times(1)).save();

spyCriticalPersistedTabData.setTitle(TITLE_A);
Assert.assertEquals(TITLE_A, spyCriticalPersistedTabData.getTitle());
verify(spyCriticalPersistedTabData, times(1)).save();

spyCriticalPersistedTabData.setTitle(TITLE_B);
Assert.assertEquals(TITLE_B, spyCriticalPersistedTabData.getTitle());
verify(spyCriticalPersistedTabData, times(2)).save();

spyCriticalPersistedTabData.setTitle(TITLE_A);
Assert.assertEquals(TITLE_A, spyCriticalPersistedTabData.getTitle());
verify(spyCriticalPersistedTabData, times(3)).save();

spyCriticalPersistedTabData.setTitle(null);
Assert.assertNull(spyCriticalPersistedTabData.getTitle());
verify(spyCriticalPersistedTabData, times(4)).save();
}

@UiThreadTest
@SmallTest
@Test
public void testRootIdSavedWhenNecessary() {
CriticalPersistedTabData spyCriticalPersistedTabData =
spy(CriticalPersistedTabData.from(mockTab(TAB_ID, false)));
spyCriticalPersistedTabData.setRootId(ROOT_ID_A);
Assert.assertEquals(ROOT_ID_A, spyCriticalPersistedTabData.getRootId());
verify(spyCriticalPersistedTabData, times(1)).save();

spyCriticalPersistedTabData.setRootId(ROOT_ID_A);
Assert.assertEquals(ROOT_ID_A, spyCriticalPersistedTabData.getRootId());
verify(spyCriticalPersistedTabData, times(1)).save();

spyCriticalPersistedTabData.setRootId(ROOT_ID_B);
Assert.assertEquals(ROOT_ID_B, spyCriticalPersistedTabData.getRootId());
verify(spyCriticalPersistedTabData, times(2)).save();

spyCriticalPersistedTabData.setRootId(ROOT_ID_A);
Assert.assertEquals(ROOT_ID_A, spyCriticalPersistedTabData.getRootId());
verify(spyCriticalPersistedTabData, times(3)).save();
}

@UiThreadTest
@SmallTest
@Test
public void testParentIdSavedWhenNecessary() {
CriticalPersistedTabData spyCriticalPersistedTabData =
spy(CriticalPersistedTabData.from(mockTab(TAB_ID, false)));
spyCriticalPersistedTabData.setParentId(PARENT_ID_A);
Assert.assertEquals(PARENT_ID_A, spyCriticalPersistedTabData.getParentId());
verify(spyCriticalPersistedTabData, times(1)).save();

spyCriticalPersistedTabData.setParentId(PARENT_ID_A);
Assert.assertEquals(PARENT_ID_A, spyCriticalPersistedTabData.getParentId());
verify(spyCriticalPersistedTabData, times(1)).save();

spyCriticalPersistedTabData.setParentId(PARENT_ID_B);
Assert.assertEquals(PARENT_ID_B, spyCriticalPersistedTabData.getParentId());
verify(spyCriticalPersistedTabData, times(2)).save();

spyCriticalPersistedTabData.setParentId(PARENT_ID_A);
Assert.assertEquals(PARENT_ID_A, spyCriticalPersistedTabData.getParentId());
verify(spyCriticalPersistedTabData, times(3)).save();
}

@UiThreadTest
@SmallTest
@Test
public void testTimestampMillisSavedWhenNecessary() {
CriticalPersistedTabData spyCriticalPersistedTabData =
spy(CriticalPersistedTabData.from(mockTab(TAB_ID, false)));
spyCriticalPersistedTabData.setTimestampMillis(TIMESTAMP_A);
Assert.assertEquals(TIMESTAMP_A, spyCriticalPersistedTabData.getTimestampMillis());
verify(spyCriticalPersistedTabData, times(1)).save();

spyCriticalPersistedTabData.setTimestampMillis(TIMESTAMP_A);
Assert.assertEquals(TIMESTAMP_A, spyCriticalPersistedTabData.getTimestampMillis());
verify(spyCriticalPersistedTabData, times(1)).save();

spyCriticalPersistedTabData.setTimestampMillis(TIMESTAMP_B);
Assert.assertEquals(TIMESTAMP_B, spyCriticalPersistedTabData.getTimestampMillis());
verify(spyCriticalPersistedTabData, times(2)).save();

spyCriticalPersistedTabData.setTimestampMillis(TIMESTAMP_A);
Assert.assertEquals(TIMESTAMP_A, spyCriticalPersistedTabData.getTimestampMillis());
verify(spyCriticalPersistedTabData, times(3)).save();
}

@UiThreadTest
@SmallTest
@Test
public void testLaunchTypeSavedWhenNecessary() {
CriticalPersistedTabData spyCriticalPersistedTabData =
spy(CriticalPersistedTabData.from(mockTab(TAB_ID, false)));
spyCriticalPersistedTabData.setLaunchTypeAtCreation(TAB_LAUNCH_TYPE_A);
Assert.assertEquals(
TAB_LAUNCH_TYPE_A, spyCriticalPersistedTabData.getTabLaunchTypeAtCreation());
verify(spyCriticalPersistedTabData, times(1)).save();

spyCriticalPersistedTabData.setLaunchTypeAtCreation(TAB_LAUNCH_TYPE_A);
Assert.assertEquals(
TAB_LAUNCH_TYPE_A, spyCriticalPersistedTabData.getTabLaunchTypeAtCreation());
verify(spyCriticalPersistedTabData, times(1)).save();

spyCriticalPersistedTabData.setLaunchTypeAtCreation(TAB_LAUNCH_TYPE_B);
Assert.assertEquals(
TAB_LAUNCH_TYPE_B, spyCriticalPersistedTabData.getTabLaunchTypeAtCreation());
verify(spyCriticalPersistedTabData, times(2)).save();

spyCriticalPersistedTabData.setLaunchTypeAtCreation(TAB_LAUNCH_TYPE_A);
Assert.assertEquals(
TAB_LAUNCH_TYPE_A, spyCriticalPersistedTabData.getTabLaunchTypeAtCreation());
verify(spyCriticalPersistedTabData, times(3)).save();

spyCriticalPersistedTabData.setLaunchTypeAtCreation(null);
Assert.assertNull(spyCriticalPersistedTabData.getTabLaunchTypeAtCreation());
verify(spyCriticalPersistedTabData, times(4)).save();
}

@UiThreadTest
@SmallTest
@Test
public void testWebContentsStateSavedWhenNecessary() {
CriticalPersistedTabData spyCriticalPersistedTabData =
spy(CriticalPersistedTabData.from(mockTab(TAB_ID, false)));
spyCriticalPersistedTabData.setWebContentsState(WEB_CONTENTS_STATE_A);
Assert.assertEquals(
WEB_CONTENTS_STATE_A, spyCriticalPersistedTabData.getWebContentsState());
verify(spyCriticalPersistedTabData, times(1)).save();

spyCriticalPersistedTabData.setWebContentsState(WEB_CONTENTS_STATE_A);
Assert.assertEquals(
WEB_CONTENTS_STATE_A, spyCriticalPersistedTabData.getWebContentsState());
verify(spyCriticalPersistedTabData, times(1)).save();

spyCriticalPersistedTabData.setWebContentsState(WEB_CONTENTS_STATE_B);
Assert.assertEquals(
WEB_CONTENTS_STATE_B, spyCriticalPersistedTabData.getWebContentsState());
verify(spyCriticalPersistedTabData, times(2)).save();

spyCriticalPersistedTabData.setWebContentsState(WEB_CONTENTS_STATE_A);
Assert.assertEquals(
WEB_CONTENTS_STATE_A, spyCriticalPersistedTabData.getWebContentsState());
verify(spyCriticalPersistedTabData, times(3)).save();

spyCriticalPersistedTabData.setWebContentsState(null);
Assert.assertNull(spyCriticalPersistedTabData.getWebContentsState());
verify(spyCriticalPersistedTabData, times(4)).save();
}
}
23 changes: 14 additions & 9 deletions .../javatests/src/org/chromium/chrome/browser/tab/state/FilePersistedTabDataStorageTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ private void testFilePersistedDataStorage(FilePersistedTabDataStorage persistedT
throws InterruptedException {
final Semaphore semaphore = new Semaphore(0);
ThreadUtils.runOnUiThreadBlocking(() -> {
persistedTabDataStorage.save(TAB_ID_1, DATA_ID_1, DATA_A, semaphore::release);
persistedTabDataStorage.save(
TAB_ID_1, DATA_ID_1, () -> { return DATA_A; }, semaphore::release);
});
semaphore.acquire();
ThreadUtils.runOnUiThreadBlocking(() -> {
Expand All @@ -86,14 +87,18 @@ private void testFilePersistedDataStorage(FilePersistedTabDataStorage persistedT
public void testRedundantSaveDropped() throws InterruptedException {
FilePersistedTabDataStorage storage = new FilePersistedTabDataStorage();
final Semaphore semaphore = new Semaphore(0);
storage.addSaveRequest(storage.new FileSaveRequest(TAB_ID_1, DATA_ID_1, DATA_A, (res) -> {
Assert.fail("First request should not have been executed as there is a subsequent "
+ "request in the queue with the same Tab ID/Data ID combination");
}));
storage.addSaveRequest(
storage.new FileSaveRequest(TAB_ID_2, DATA_ID_2, DATA_A, semaphore::release));
storage.addSaveRequest(
storage.new FileSaveRequest(TAB_ID_1, DATA_ID_1, DATA_B, semaphore::release));
storage.addSaveRequest(storage.new FileSaveRequest(TAB_ID_1, DATA_ID_1,
()
-> { return DATA_A; },
(res) -> {
Assert.fail(
"First request should not have been executed as there is a subsequent "
+ "request in the queue with the same Tab ID/Data ID combination");
}));
storage.addSaveRequest(storage.new FileSaveRequest(
TAB_ID_2, DATA_ID_2, () -> { return DATA_A; }, semaphore::release));
storage.addSaveRequest(storage.new FileSaveRequest(
TAB_ID_1, DATA_ID_1, () -> { return DATA_B; }, semaphore::release));
ThreadUtils.runOnUiThreadBlocking(() -> {
storage.processNextItemOnQueue();
storage.processNextItemOnQueue();
Expand Down
54 changes: 50 additions & 4 deletions chrome/android/javatests/src/org/chromium/chrome/browser/tab/state/PersistedTabDataTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,18 @@
import org.junit.runner.RunWith;
import org.mockito.MockitoAnnotations;

import org.chromium.base.ThreadUtils;
import org.chromium.base.supplier.ObservableSupplierImpl;
import org.chromium.base.supplier.Supplier;
import org.chromium.base.test.BaseJUnit4ClassRunner;
import org.chromium.base.test.UiThreadTest;
import org.chromium.base.test.util.Batch;
import org.chromium.base.test.util.CallbackHelper;
import org.chromium.chrome.browser.tab.MockTab;
import org.chromium.chrome.browser.tab.Tab;

import java.util.concurrent.TimeoutException;

/**
* Test relating to {@link PersistedTabData}
*/
Expand Down Expand Up @@ -66,7 +71,10 @@ public void testSerializeAndLogOutOfMemoryError() {
Tab tab = MockTab.createAndInitialize(1, false);
OutOfMemoryMockPersistedTabData outOfMemoryMockPersistedTabData =
new OutOfMemoryMockPersistedTabData(tab);
Assert.assertNull(outOfMemoryMockPersistedTabData.serializeAndLog());
Assert.assertNull(outOfMemoryMockPersistedTabData
.getOomAndMetricsWrapper(
outOfMemoryMockPersistedTabData.getSerializeSupplier())
.get());
}

@SmallTest
Expand All @@ -76,16 +84,54 @@ public void testSerializeOutOfMemoryError() {
Tab tab = MockTab.createAndInitialize(1, false);
OutOfMemoryMockPersistedTabData outOfMemoryMockPersistedTabData =
new OutOfMemoryMockPersistedTabData(tab);
outOfMemoryMockPersistedTabData.serialize();
outOfMemoryMockPersistedTabData.getSerializeSupplier().get();
}

@SmallTest
@UiThreadTest
@Test
public void testSerializeSupplierUiBackgroundThread() throws TimeoutException {
CallbackHelper helper = new CallbackHelper();
int count = helper.getCallCount();
ThreadUtils.runOnUiThreadBlocking(() -> {
Tab tab = MockTab.createAndInitialize(1, false);
ThreadVerifierMockPersistedTabData threadVerifierMockPersistedTabData =
new ThreadVerifierMockPersistedTabData(tab);
threadVerifierMockPersistedTabData.save();
helper.notifyCalled();
});
helper.waitForCallback(count);
}

static class ThreadVerifierMockPersistedTabData extends MockPersistedTabData {
ThreadVerifierMockPersistedTabData(Tab tab) {
super(tab, 0 /** unused in ThreadVerifierMockPersistedTabData */);
}

@Override
public Supplier<byte[]> getSerializeSupplier() {
// Verify anything before the supplier is called on the UI thread
ThreadUtils.assertOnUiThread();
return () -> {
// supplier.get() should be called on the background thread - if
// it doesn't other {@link PersistedTabData} such as
// {@link CriticalPersistedTabData} may unnecessarily consume
// the UI thread and cause jank.
ThreadUtils.assertOnBackgroundThread();
return super.getSerializeSupplier().get();
};
}
}

static class OutOfMemoryMockPersistedTabData extends MockPersistedTabData {
OutOfMemoryMockPersistedTabData(Tab tab) {
super(tab, 0 /** unused in OutOfMemoryMockPersistedTabData */);
}
@Override
public byte[] serialize() {
throw new OutOfMemoryError("Out of memory error");
public Supplier<byte[]> getSerializeSupplier() {
return () -> {
throw new OutOfMemoryError("Out of memory error");
};
}
}

Expand Down
4 changes: 2 additions & 2 deletions ...oid/javatests/src/org/chromium/chrome/browser/tab/state/ShoppingPersistedTabDataTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ public void testShoppingProto() {
shoppingPersistedTabData.registerIsTabSaveEnabledSupplier(supplier);
shoppingPersistedTabData.setPriceMicros(
ShoppingPersistedTabDataTestUtils.PRICE_MICROS, null);
byte[] serialized = shoppingPersistedTabData.serialize();
byte[] serialized = shoppingPersistedTabData.getSerializeSupplier().get();
ShoppingPersistedTabData deserialized = new ShoppingPersistedTabData(tab);
deserialized.deserialize(serialized);
Assert.assertEquals(
Expand Down Expand Up @@ -482,7 +482,7 @@ public void testSerializationBug() {
ShoppingPersistedTabDataTestUtils.IS_INCOGNITO);
ShoppingPersistedTabData shoppingPersistedTabData = new ShoppingPersistedTabData(tab);
shoppingPersistedTabData.setPriceMicros(42_000_000L, null);
byte[] serialized = shoppingPersistedTabData.serialize();
byte[] serialized = shoppingPersistedTabData.getSerializeSupplier().get();
PersistedTabDataConfiguration config = PersistedTabDataConfiguration.get(
ShoppingPersistedTabData.class, tab.isIncognito());
ShoppingPersistedTabData deserialized =
Expand Down

0 comments on commit de6efd7

Please sign in to comment.