From daa8ff36cc271d6ded3c0227ad29c69e50e7b555 Mon Sep 17 00:00:00 2001 From: Yi-Ling Chuang Date: Thu, 14 Feb 2019 20:36:20 +0800 Subject: [PATCH 1/2] Use a better way to bind slice for slice precheck. Calling Slice.bindSlice() directly will cause the exception stating that slices are not pinned, which sometimes leads to crash. Hence, change the way we bind slices which handles pinSlice() for us before onBindSlice(). Bug: 120552892 Test: robotests, unit tests Change-Id: I3e65c6b79876dbee5db6f19387bc6b675f734161 --- .../contextualcards/ContextualCardLoader.java | 52 ++++++++++++++----- .../ContextualCardLoaderTest.java | 9 ---- .../ContextualCardLoaderTest.java | 22 +++++++- 3 files changed, 60 insertions(+), 23 deletions(-) diff --git a/src/com/android/settings/homepage/contextualcards/ContextualCardLoader.java b/src/com/android/settings/homepage/contextualcards/ContextualCardLoader.java index 7a0eb0f9f17..1acddabcec0 100644 --- a/src/com/android/settings/homepage/contextualcards/ContextualCardLoader.java +++ b/src/com/android/settings/homepage/contextualcards/ContextualCardLoader.java @@ -18,13 +18,10 @@ package com.android.settings.homepage.contextualcards; import static android.app.slice.Slice.HINT_ERROR; -import static androidx.slice.widget.SliceLiveData.SUPPORTED_SPECS; - import static com.android.settings.slices.CustomSliceRegistry.BLUETOOTH_DEVICES_SLICE_URI; import static com.android.settings.slices.CustomSliceRegistry.CONTEXTUAL_WIFI_SLICE_URI; import static com.android.settings.slices.CustomSliceRegistry.NOTIFICATION_CHANNEL_SLICE_URI; -import android.content.ContentProviderClient; import android.content.ContentResolver; import android.content.Context; import android.database.ContentObserver; @@ -38,6 +35,7 @@ import android.util.Log; import androidx.annotation.NonNull; import androidx.annotation.VisibleForTesting; import androidx.slice.Slice; +import androidx.slice.SliceViewManager; import com.android.settings.R; import com.android.settings.overlay.FeatureFactory; @@ -45,6 +43,8 @@ import com.android.settingslib.utils.AsyncLoaderCompat; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; public class ContextualCardLoader extends AsyncLoaderCompat> { @@ -55,6 +55,7 @@ public class ContextualCardLoader extends AsyncLoaderCompat static final long CARD_CONTENT_LOADER_TIMEOUT_MS = DateUtils.SECOND_IN_MILLIS * 3; private static final String TAG = "ContextualCardLoader"; + private static final long LATCH_TIMEOUT_MS = 200; private final ContentObserver mObserver = new ContentObserver( new Handler(Looper.getMainLooper())) { @@ -186,16 +187,7 @@ public class ContextualCardLoader extends AsyncLoaderCompat return false; } - //check if the uri has a provider associated with. - final ContentProviderClient provider = - mContext.getContentResolver().acquireContentProviderClient(uri); - if (provider == null) { - return false; - } - //release contentProviderClient to prevent from memory leak. - provider.release(); - - final Slice slice = Slice.bindSlice(mContext, uri, SUPPORTED_SPECS); + final Slice slice = bindSlice(uri); //TODO(b/123668403): remove the log here once we do the change with FutureTask final long bindTime = System.currentTimeMillis() - startTime; Log.d(TAG, "Binding time for " + uri + " = " + bindTime); @@ -208,6 +200,40 @@ public class ContextualCardLoader extends AsyncLoaderCompat return true; } + @VisibleForTesting + Slice bindSlice(Uri uri) { + final SliceViewManager manager = SliceViewManager.getInstance(mContext); + final Slice[] returnSlice = new Slice[1]; + final CountDownLatch latch = new CountDownLatch(1); + final SliceViewManager.SliceCallback callback = + new SliceViewManager.SliceCallback() { + @Override + public void onSliceUpdated(Slice slice) { + try { + // We are just making sure the existence of the slice, so ignore + // slice loading state here. + returnSlice[0] = slice; + latch.countDown(); + } catch (Exception e) { + Log.w(TAG, uri + " cannot be indexed", e); + } finally { + manager.unregisterSliceCallback(uri, this); + } + } + }; + // Register a callback until we get a loaded slice. + manager.registerSliceCallback(uri, callback); + // Trigger the binding. + callback.onSliceUpdated(manager.bindSlice(uri)); + try { + latch.await(LATCH_TIMEOUT_MS, TimeUnit.MILLISECONDS); + } catch (InterruptedException e) { + Log.w(TAG, "Error waiting for slice binding for uri" + uri, e); + manager.unregisterSliceCallback(uri, callback); + } + return returnSlice[0]; + } + private int getNumberOfLargeCard(List cards) { return (int) cards.stream() .filter(card -> isLargeCard(card)) diff --git a/tests/robotests/src/com/android/settings/homepage/contextualcards/ContextualCardLoaderTest.java b/tests/robotests/src/com/android/settings/homepage/contextualcards/ContextualCardLoaderTest.java index f04008b199e..beb2d407c62 100644 --- a/tests/robotests/src/com/android/settings/homepage/contextualcards/ContextualCardLoaderTest.java +++ b/tests/robotests/src/com/android/settings/homepage/contextualcards/ContextualCardLoaderTest.java @@ -73,15 +73,6 @@ public class ContextualCardLoaderTest { getContextualCard(sliceUri))).isFalse(); } - @Test - public void isCardEligibleToDisplay_noProvider_returnFalse() { - final String sliceUri = "content://com.android.settings.test.slices/action/flashlight"; - - assertThat( - mContextualCardLoader.isCardEligibleToDisplay( - getContextualCard(sliceUri))).isFalse(); - } - @Test public void getDisplayableCards_twoEligibleCards_shouldShowAll() { final List cards = getContextualCardList().stream().limit(2) diff --git a/tests/unit/src/com/android/settings/homepage/contextualcards/ContextualCardLoaderTest.java b/tests/unit/src/com/android/settings/homepage/contextualcards/ContextualCardLoaderTest.java index 82fcd7db739..55ad1ff6eb7 100644 --- a/tests/unit/src/com/android/settings/homepage/contextualcards/ContextualCardLoaderTest.java +++ b/tests/unit/src/com/android/settings/homepage/contextualcards/ContextualCardLoaderTest.java @@ -21,9 +21,12 @@ import static com.google.common.truth.Truth.assertThat; import android.content.Context; import android.net.Uri; +import androidx.slice.Slice; import androidx.test.InstrumentationRegistry; import androidx.test.runner.AndroidJUnit4; +import com.android.settings.slices.CustomSliceRegistry; + import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -44,7 +47,7 @@ public class ContextualCardLoaderTest { } @Test - public void filter_twoInvalidCards_shouldReturnOneCard() { + public void filterEligibleCards_twoInvalidCards_shouldReturnOneCard() { final String sliceUri1 = "content://com.android.settings.slices/action/flashlight"; //valid final String sliceUri2 = "content://com.android.settings.test.slices/action/flashlight"; final String sliceUri3 = "cotent://com.android.settings.slices/action/flashlight"; @@ -59,6 +62,23 @@ public class ContextualCardLoaderTest { assertThat(result).hasSize(1); } + @Test + public void bindSlice_flashlightUri_shouldReturnFlashlightSlice() { + final Slice loadedSlice = + mContextualCardLoader.bindSlice(CustomSliceRegistry.FLASHLIGHT_SLICE_URI); + + assertThat(loadedSlice.getUri()).isEqualTo(CustomSliceRegistry.FLASHLIGHT_SLICE_URI); + } + + @Test + public void bindSlice_noProvider_shouldReturnNull() { + final String sliceUri = "content://com.android.settings.test.slices/action/flashlight"; + + final Slice loadedSlice = mContextualCardLoader.bindSlice(Uri.parse(sliceUri)); + + assertThat(loadedSlice).isNull(); + } + private ContextualCard getContextualCard(String sliceUri) { return new ContextualCard.Builder() .setName("test_card") From dde76e6d5caa467486f58899855e8de4b1a37208 Mon Sep 17 00:00:00 2001 From: Yi-Ling Chuang Date: Thu, 14 Feb 2019 21:31:11 +0800 Subject: [PATCH 2/2] Make slices precheck executes in parallel. Slice binding takes up sometime which may cause latency. Thus, making it run in parallel instead of sequentially to improve the performance and avoid hitting the timeout problem. Bug: 124366297 Fixes: 123668403 Test: robotests, unit tests Change-Id: I5d9fa4605f59e2acef65aadf6fce85df36d8fff1 --- .../contextualcards/ContextualCardLoader.java | 94 +++++----------- .../contextualcards/EligibleCardChecker.java | 104 ++++++++++++++++++ .../ContextualCardLoaderTest.java | 34 +++++- .../ContextualCardLoaderTest.java | 18 +-- 4 files changed, 171 insertions(+), 79 deletions(-) create mode 100644 src/com/android/settings/homepage/contextualcards/EligibleCardChecker.java diff --git a/src/com/android/settings/homepage/contextualcards/ContextualCardLoader.java b/src/com/android/settings/homepage/contextualcards/ContextualCardLoader.java index 1acddabcec0..b9b2a1c784d 100644 --- a/src/com/android/settings/homepage/contextualcards/ContextualCardLoader.java +++ b/src/com/android/settings/homepage/contextualcards/ContextualCardLoader.java @@ -16,17 +16,13 @@ package com.android.settings.homepage.contextualcards; -import static android.app.slice.Slice.HINT_ERROR; - import static com.android.settings.slices.CustomSliceRegistry.BLUETOOTH_DEVICES_SLICE_URI; import static com.android.settings.slices.CustomSliceRegistry.CONTEXTUAL_WIFI_SLICE_URI; import static com.android.settings.slices.CustomSliceRegistry.NOTIFICATION_CHANNEL_SLICE_URI; -import android.content.ContentResolver; import android.content.Context; import android.database.ContentObserver; import android.database.Cursor; -import android.net.Uri; import android.os.Handler; import android.os.Looper; import android.text.format.DateUtils; @@ -34,8 +30,6 @@ import android.util.Log; import androidx.annotation.NonNull; import androidx.annotation.VisibleForTesting; -import androidx.slice.Slice; -import androidx.slice.SliceViewManager; import com.android.settings.R; import com.android.settings.overlay.FeatureFactory; @@ -43,9 +37,12 @@ import com.android.settingslib.utils.AsyncLoaderCompat; import java.util.ArrayList; import java.util.List; -import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; -import java.util.stream.Collectors; +import java.util.concurrent.TimeoutException; public class ContextualCardLoader extends AsyncLoaderCompat> { @@ -55,8 +52,9 @@ public class ContextualCardLoader extends AsyncLoaderCompat static final long CARD_CONTENT_LOADER_TIMEOUT_MS = DateUtils.SECOND_IN_MILLIS * 3; private static final String TAG = "ContextualCardLoader"; - private static final long LATCH_TIMEOUT_MS = 200; + private static final long ELIGIBILITY_CHECKER_TIMEOUT_MS = 250; + private final ExecutorService mExecutorService; private final ContentObserver mObserver = new ContentObserver( new Handler(Looper.getMainLooper())) { @Override @@ -72,6 +70,7 @@ public class ContextualCardLoader extends AsyncLoaderCompat ContextualCardLoader(Context context) { super(context); mContext = context.getApplicationContext(); + mExecutorService = Executors.newCachedThreadPool(); } @Override @@ -170,68 +169,27 @@ public class ContextualCardLoader extends AsyncLoaderCompat @VisibleForTesting List filterEligibleCards(List candidates) { - return candidates.stream().filter(card -> isCardEligibleToDisplay(card)) - .collect(Collectors.toList()); - } + final List cards = new ArrayList<>(); + final List> eligibleCards = new ArrayList<>(); - @VisibleForTesting - boolean isCardEligibleToDisplay(ContextualCard card) { - final long startTime = System.currentTimeMillis(); - if (card.isCustomCard()) { - return true; + for (ContextualCard card : candidates) { + final EligibleCardChecker future = new EligibleCardChecker(mContext, card); + eligibleCards.add(mExecutorService.submit(future)); } - - final Uri uri = card.getSliceUri(); - - if (!ContentResolver.SCHEME_CONTENT.equals(uri.getScheme())) { - return false; + // Collect future and eligible cards + for (Future cardFuture : eligibleCards) { + try { + //TODO(b/124492762): Log latency and timeout occurrence. + final ContextualCard card = cardFuture.get(ELIGIBILITY_CHECKER_TIMEOUT_MS, + TimeUnit.MILLISECONDS); + if (card != null) { + cards.add(card); + } + } catch (ExecutionException | InterruptedException | TimeoutException e) { + Log.w(TAG, "Failed to get eligible state for card, likely timeout. Skipping", e); + } } - - final Slice slice = bindSlice(uri); - //TODO(b/123668403): remove the log here once we do the change with FutureTask - final long bindTime = System.currentTimeMillis() - startTime; - Log.d(TAG, "Binding time for " + uri + " = " + bindTime); - - if (slice == null || slice.hasHint(HINT_ERROR)) { - Log.w(TAG, "Failed to bind slice, not eligible for display " + uri); - return false; - } - - return true; - } - - @VisibleForTesting - Slice bindSlice(Uri uri) { - final SliceViewManager manager = SliceViewManager.getInstance(mContext); - final Slice[] returnSlice = new Slice[1]; - final CountDownLatch latch = new CountDownLatch(1); - final SliceViewManager.SliceCallback callback = - new SliceViewManager.SliceCallback() { - @Override - public void onSliceUpdated(Slice slice) { - try { - // We are just making sure the existence of the slice, so ignore - // slice loading state here. - returnSlice[0] = slice; - latch.countDown(); - } catch (Exception e) { - Log.w(TAG, uri + " cannot be indexed", e); - } finally { - manager.unregisterSliceCallback(uri, this); - } - } - }; - // Register a callback until we get a loaded slice. - manager.registerSliceCallback(uri, callback); - // Trigger the binding. - callback.onSliceUpdated(manager.bindSlice(uri)); - try { - latch.await(LATCH_TIMEOUT_MS, TimeUnit.MILLISECONDS); - } catch (InterruptedException e) { - Log.w(TAG, "Error waiting for slice binding for uri" + uri, e); - manager.unregisterSliceCallback(uri, callback); - } - return returnSlice[0]; + return cards; } private int getNumberOfLargeCard(List cards) { diff --git a/src/com/android/settings/homepage/contextualcards/EligibleCardChecker.java b/src/com/android/settings/homepage/contextualcards/EligibleCardChecker.java new file mode 100644 index 00000000000..5423ce3ae0c --- /dev/null +++ b/src/com/android/settings/homepage/contextualcards/EligibleCardChecker.java @@ -0,0 +1,104 @@ +/* + * Copyright (C) 2019 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.settings.homepage.contextualcards; + +import static android.app.slice.Slice.HINT_ERROR; + +import android.content.ContentResolver; +import android.content.Context; +import android.net.Uri; +import android.util.Log; + +import androidx.annotation.VisibleForTesting; +import androidx.slice.Slice; +import androidx.slice.SliceViewManager; + +import java.util.concurrent.Callable; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + +public class EligibleCardChecker implements Callable { + + private static final String TAG = "EligibleCardChecker"; + private static final long LATCH_TIMEOUT_MS = 200; + + private final Context mContext; + private final ContextualCard mCard; + + EligibleCardChecker(Context context, ContextualCard card) { + mContext = context; + mCard = card; + } + + @Override + public ContextualCard call() throws Exception { + return isCardEligibleToDisplay(mCard) ? mCard : null; + } + + @VisibleForTesting + boolean isCardEligibleToDisplay(ContextualCard card) { + if (card.isCustomCard()) { + return true; + } + + final Uri uri = card.getSliceUri(); + if (!ContentResolver.SCHEME_CONTENT.equals(uri.getScheme())) { + return false; + } + + final Slice slice = bindSlice(uri); + if (slice == null || slice.hasHint(HINT_ERROR)) { + Log.w(TAG, "Failed to bind slice, not eligible for display " + uri); + return false; + } + return true; + } + + @VisibleForTesting + Slice bindSlice(Uri uri) { + final SliceViewManager manager = SliceViewManager.getInstance(mContext); + final Slice[] returnSlice = new Slice[1]; + final CountDownLatch latch = new CountDownLatch(1); + final SliceViewManager.SliceCallback callback = + new SliceViewManager.SliceCallback() { + @Override + public void onSliceUpdated(Slice slice) { + try { + // We are just making sure the existence of the slice, so ignore + // slice loading state here. + returnSlice[0] = slice; + latch.countDown(); + } catch (Exception e) { + Log.w(TAG, uri + " cannot be indexed", e); + } finally { + manager.unregisterSliceCallback(uri, this); + } + } + }; + // Register a callback until we get a loaded slice. + manager.registerSliceCallback(uri, callback); + // Trigger the binding. + callback.onSliceUpdated(manager.bindSlice(uri)); + try { + latch.await(LATCH_TIMEOUT_MS, TimeUnit.MILLISECONDS); + } catch (InterruptedException e) { + Log.w(TAG, "Error waiting for slice binding for uri" + uri, e); + manager.unregisterSliceCallback(uri, callback); + } + return returnSlice[0]; + } +} diff --git a/tests/robotests/src/com/android/settings/homepage/contextualcards/ContextualCardLoaderTest.java b/tests/robotests/src/com/android/settings/homepage/contextualcards/ContextualCardLoaderTest.java index beb2d407c62..18aa1c4720b 100644 --- a/tests/robotests/src/com/android/settings/homepage/contextualcards/ContextualCardLoaderTest.java +++ b/tests/robotests/src/com/android/settings/homepage/contextualcards/ContextualCardLoaderTest.java @@ -16,6 +16,8 @@ package com.android.settings.homepage.contextualcards; +import static android.app.slice.Slice.HINT_ERROR; + import static com.android.settings.homepage.contextualcards.ContextualCardLoader.DEFAULT_CARD_COUNT; import static com.google.common.truth.Truth.assertThat; @@ -27,6 +29,8 @@ import static org.mockito.Mockito.spy; import android.content.Context; import android.net.Uri; +import androidx.slice.Slice; + import com.android.settings.R; import com.android.settings.slices.CustomSliceRegistry; @@ -43,13 +47,18 @@ import java.util.stream.Collectors; @RunWith(RobolectricTestRunner.class) public class ContextualCardLoaderTest { + private static final String TEST_SLICE_URI = "content://test/test"; + private Context mContext; private ContextualCardLoader mContextualCardLoader; + private EligibleCardChecker mEligibleCardChecker; @Before public void setUp() { mContext = RuntimeEnvironment.application; mContextualCardLoader = spy(new ContextualCardLoader(mContext)); + mEligibleCardChecker = + spy(new EligibleCardChecker(mContext, getContextualCard(TEST_SLICE_URI))); } @Test @@ -61,16 +70,33 @@ public class ContextualCardLoaderTest { .setSummaryText("custom_summary") .build(); - assertThat(mContextualCardLoader.isCardEligibleToDisplay(customCard)).isTrue(); + assertThat(mEligibleCardChecker.isCardEligibleToDisplay(customCard)).isTrue(); } @Test public void isCardEligibleToDisplay_invalidScheme_returnFalse() { final String sliceUri = "contet://com.android.settings.slices/action/flashlight"; - assertThat( - mContextualCardLoader.isCardEligibleToDisplay( - getContextualCard(sliceUri))).isFalse(); + assertThat(mEligibleCardChecker.isCardEligibleToDisplay(getContextualCard(sliceUri))) + .isFalse(); + } + + @Test + public void isCardEligibleToDisplay_nullSlice_returnFalse() { + doReturn(null).when(mEligibleCardChecker).bindSlice(Uri.parse(TEST_SLICE_URI)); + + assertThat(mEligibleCardChecker.isCardEligibleToDisplay(getContextualCard(TEST_SLICE_URI))) + .isFalse(); + } + + @Test + public void isCardEligibleToDisplay_errorSlice_returnFalse() { + final Slice slice = new Slice.Builder(Uri.parse(TEST_SLICE_URI)) + .addHints(HINT_ERROR).build(); + doReturn(slice).when(mEligibleCardChecker).bindSlice(Uri.parse(TEST_SLICE_URI)); + + assertThat(mEligibleCardChecker.isCardEligibleToDisplay(getContextualCard(TEST_SLICE_URI))) + .isFalse(); } @Test diff --git a/tests/unit/src/com/android/settings/homepage/contextualcards/ContextualCardLoaderTest.java b/tests/unit/src/com/android/settings/homepage/contextualcards/ContextualCardLoaderTest.java index 55ad1ff6eb7..447e2b45e7b 100644 --- a/tests/unit/src/com/android/settings/homepage/contextualcards/ContextualCardLoaderTest.java +++ b/tests/unit/src/com/android/settings/homepage/contextualcards/ContextualCardLoaderTest.java @@ -37,13 +37,17 @@ import java.util.List; @RunWith(AndroidJUnit4.class) public class ContextualCardLoaderTest { + private static final Uri TEST_URI = Uri.parse("content://test/test"); + private Context mContext; private ContextualCardLoader mContextualCardLoader; + private EligibleCardChecker mEligibleCardChecker; @Before public void setUp() { mContext = InstrumentationRegistry.getTargetContext(); mContextualCardLoader = new ContextualCardLoader(mContext); + mEligibleCardChecker = new EligibleCardChecker(mContext, getContextualCard(TEST_URI)); } @Test @@ -53,9 +57,9 @@ public class ContextualCardLoaderTest { final String sliceUri3 = "cotent://com.android.settings.slices/action/flashlight"; final List cards = new ArrayList<>(); - cards.add(getContextualCard(sliceUri1)); - cards.add(getContextualCard(sliceUri2)); - cards.add(getContextualCard(sliceUri3)); + cards.add(getContextualCard(Uri.parse(sliceUri1))); + cards.add(getContextualCard(Uri.parse(sliceUri2))); + cards.add(getContextualCard(Uri.parse(sliceUri3))); final List result = mContextualCardLoader.filterEligibleCards(cards); @@ -65,7 +69,7 @@ public class ContextualCardLoaderTest { @Test public void bindSlice_flashlightUri_shouldReturnFlashlightSlice() { final Slice loadedSlice = - mContextualCardLoader.bindSlice(CustomSliceRegistry.FLASHLIGHT_SLICE_URI); + mEligibleCardChecker.bindSlice(CustomSliceRegistry.FLASHLIGHT_SLICE_URI); assertThat(loadedSlice.getUri()).isEqualTo(CustomSliceRegistry.FLASHLIGHT_SLICE_URI); } @@ -74,16 +78,16 @@ public class ContextualCardLoaderTest { public void bindSlice_noProvider_shouldReturnNull() { final String sliceUri = "content://com.android.settings.test.slices/action/flashlight"; - final Slice loadedSlice = mContextualCardLoader.bindSlice(Uri.parse(sliceUri)); + final Slice loadedSlice = mEligibleCardChecker.bindSlice(Uri.parse(sliceUri)); assertThat(loadedSlice).isNull(); } - private ContextualCard getContextualCard(String sliceUri) { + private ContextualCard getContextualCard(Uri sliceUri) { return new ContextualCard.Builder() .setName("test_card") .setCardType(ContextualCard.CardType.SLICE) - .setSliceUri(Uri.parse(sliceUri)) + .setSliceUri(sliceUri) .build(); } }