Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1711063002:
Avoid disk reads in TabPersistentStore for max tab_id. (Closed)
DescriptionAvoid disk reads in TabPersistentStore for max tab_id.
If we have already stored the max tab_id from a prior run, do not
read all the tab state folders looking for it again. The only time
this should happen is during an upgrade from a much older version
of Chrome as the shared preference would not have been set yet.
BUG=587941
Committed: https://crrev.com/bbe95ed8e2dbd8cc7f23d4bcda82b0bae2d7242e
Cr-Commit-Position: refs/heads/master@{#379613}
Patch Set 1 #Patch Set 2 : Remove excess. #
Total comments: 8
Patch Set 3 : Extract shared code. #Patch Set 4 : Fix per review. #
Total comments: 1
Patch Set 5 : Use pref boolean instead. #Patch Set 6 : Minor cleanup. #
Total comments: 6
Patch Set 7 : Move prefs to TabPersistentStore. #
Total comments: 11
Patch Set 8 : Fix review comments. #Patch Set 9 : Update uma. #Patch Set 10 : Add refactoring to this CL. #
Messages
Total messages: 21 (3 generated)
wnwen@chromium.org changed reviewers: + dfalcantara@chromium.org
Hi Dan, This CL adds a fast path for all users who already have their max_tab_id pref set (which should be everyone on M45+). I'll further refactor the shared OnTabStateReadCallback code, but wanted to run this by you first. Thanks, Peter
https://codereview.chromium.org/1711063002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (left): https://codereview.chromium.org/1711063002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:752: if (!folder.isDirectory()) continue; Given that asserts are borked, it might be worth keeping the conditional and getting rid of the assert instead. https://codereview.chromium.org/1711063002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/1711063002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:351: if (TabIdManager.getInstance().hasValidMaxId()) { I don't think this is a safe check. If the TabIdManager is used to generate a new tab in any way before ChromeTabbedActivity has a chance to load the files (e.g. by creating a CustomTabActivity(), it'll always go down the fast path without correctly setting the ID. https://codereview.chromium.org/1711063002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:740: * Load the saved state of the tab model. No tabs will be restored until you call Can you clarify exactly why this is different from the one below? InternalFast() isn't a very useful name, either, since it just implies that does the same thing as the other one, but faster. https://codereview.chromium.org/1711063002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:752: readSavedStateFile(getStateDirectory(), new OnTabStateReadCallback() { Is this TabStateReadCallback exactly the same as the one below? Can we make a callback creator function()?
Presumably I will also check the other places where max id is set and switch them to setMaxId where it makes sense. Do you know if any of them do lots of work like TPS? https://codereview.chromium.org/1711063002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (left): https://codereview.chromium.org/1711063002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:752: if (!folder.isDirectory()) continue; On 2016/02/18 21:19:20, dfalcantara wrote: > Given that asserts are borked, it might be worth keeping the conditional and > getting rid of the assert instead. Done. https://codereview.chromium.org/1711063002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/1711063002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:351: if (TabIdManager.getInstance().hasValidMaxId()) { On 2016/02/18 21:19:20, dfalcantara wrote: > I don't think this is a safe check. If the TabIdManager is used to generate a > new tab in any way before ChromeTabbedActivity has a chance to load the files > (e.g. by creating a CustomTabActivity(), it'll always go down the fast path > without correctly setting the ID. Agreed, added boolean flag. https://codereview.chromium.org/1711063002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:740: * Load the saved state of the tab model. No tabs will be restored until you call On 2016/02/18 21:19:20, dfalcantara wrote: > Can you clarify exactly why this is different from the one below? > InternalFast() isn't a very useful name, either, since it just implies that does > the same thing as the other one, but faster. Done. https://codereview.chromium.org/1711063002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:752: readSavedStateFile(getStateDirectory(), new OnTabStateReadCallback() { On 2016/02/18 21:19:20, dfalcantara wrote: > Is this TabStateReadCallback exactly the same as the one below? Can we make a > callback creator function()? Done.
Starting to remember this is fraught with peril because of all the different startup paths :/ https://chromiumcodereview.appspot.com/1711063002/diff/60001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabIdManager.java (right): https://chromiumcodereview.appspot.com/1711063002/diff/60001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/tab/TabIdManager.java:120: mHasValidMaxId = mPreferences.contains(PREF_NEXT_ID); This is still broken, I think. If Chrome is launched for a CustomTabActivity and the process dies without having opened ChromeTabbedActivity, then the next time ChromeTabbedActivity is launched this value will already be set to true.
Maybe it makes more sense to set a boolean that specifically checks if the pathway that loads all of the TabModel metadata has been run at least once? Setting this boolean in any other location makes it possible for this pathway to be skipped.
On 2016/02/19 22:32:34, dfalcantara wrote: > Maybe it makes more sense to set a boolean that specifically checks if the > pathway that loads all of the TabModel metadata has been run at least once? > Setting this boolean in any other location makes it possible for this pathway to > be skipped. Good idea, will do that. TPS.loadState is the only place that does this, right? Looks like ChromeTabbedActivity calls through TabModelSelectorImpl.loadState. I was thinking we can check if the max_id is less than 1000, and set it to 1000 when we finish checking all the metadata, but a specific boolean is much cleaner.
Yeah, I think TabPersistentStore#loadState is the only place this wonkiness happens. It's not safe to assume that ChromeTabbedActivity is the only place it'll happen in the future, though -- CustomTabActivity now uses TabModelSelectorImpl and TabPersistentStore, too, and they may eventually need to start loading up TabStates, as well.
PTAL. :)
https://chromiumcodereview.appspot.com/1711063002/diff/100001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabIdManager.java (right): https://chromiumcodereview.appspot.com/1711063002/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/tab/TabIdManager.java:36: public static final String PREF_VALID_MAX_ID = 1) Everything related to this boolean should be in TabPersistentStore, not here. This boolean affects/is affected by only code there. 2) Rename to PREF_HAS_COMPUTED_MAX_ID; "valid" is ambiguous https://chromiumcodereview.appspot.com/1711063002/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/tab/TabIdManager.java:46: private boolean mHasValidMaxId; mHasComputedMaxId https://chromiumcodereview.appspot.com/1711063002/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/tab/TabIdManager.java:102: * Set the tab id counter to a known good value and record that we've done so. The following comment won't matter if the boolean is moved to TabPersistentStore: This is a public function, and it sounds completely innocuous. Some other place could call this and mess with the TabPersistentStore's branches.
Thanks! PTAL. :) https://codereview.chromium.org/1711063002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabIdManager.java (right): https://codereview.chromium.org/1711063002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/TabIdManager.java:36: public static final String PREF_VALID_MAX_ID = On 2016/02/23 22:01:08, dfalcantara wrote: > 1) Everything related to this boolean should be in TabPersistentStore, not here. > This boolean affects/is affected by only code there. > > 2) Rename to PREF_HAS_COMPUTED_MAX_ID; "valid" is ambiguous Done. https://codereview.chromium.org/1711063002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/TabIdManager.java:46: private boolean mHasValidMaxId; On 2016/02/23 22:01:08, dfalcantara wrote: > mHasComputedMaxId Done. https://codereview.chromium.org/1711063002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/TabIdManager.java:102: * Set the tab id counter to a known good value and record that we've done so. On 2016/02/23 22:01:09, dfalcantara wrote: > The following comment won't matter if the boolean is moved to > TabPersistentStore: > > This is a public function, and it sounds completely innocuous. Some other place > could call this and mess with the TabPersistentStore's branches. Done. https://codereview.chromium.org/1711063002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/1711063002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:360: assert mTabModelSelector.getModel(true).getCount() == 0; Do asserts still work?
lgtm % comments https://codereview.chromium.org/1711063002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/1711063002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:67: "org.chromium.chrome.browser.tab.TabPersistentStore.HAS_COMPUTED_MAX_ID"; .tab. -> .tabmodel. https://codereview.chromium.org/1711063002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:360: assert mTabModelSelector.getModel(true).getCount() == 0; On 2016/02/25 19:17:32, Peter Wen wrote: > Do asserts still work? Yes, on older Android versions. There's some work planned for getting them to work on later Android versions again too. https://codereview.chromium.org/1711063002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:379: TabIdManager.getInstance().generateValidId(Tab.INVALID_TAB_ID)); I'm not sure this call will ever do anything now. This ID should always be higher than any Tab ever opened because we store that number globally. Before we started using the global next tab ID preference, it used to be possible to get into a state where the computed next tab ID would be lower than all-time maximum because a user could close a bunch of tabs. That said, I don't know if this call is necessary in the new world. It's probably safe to remove. https://codereview.chromium.org/1711063002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:780: createOnTabStateReadCallback(mTabModelSelector.isIncognitoSelected())); nit: indented incorrectly https://codereview.chromium.org/1711063002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:792: public int loadStateInternalWithMaxTabId() throws IOException { nit: loadStateAndComputeMaxTabIdInternal? Can this be protected? Add a comment saying that anyone calling this from outside of a test should call loadState() instead?
Thanks for the review! I have some further refactoring that I'll put in a separate CL (http://crrev.com/1761103002), but will commit this as is. https://codereview.chromium.org/1711063002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/1711063002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:67: "org.chromium.chrome.browser.tab.TabPersistentStore.HAS_COMPUTED_MAX_ID"; On 2016/02/29 19:55:35, dfalcantara wrote: > .tab. -> .tabmodel. Done. https://codereview.chromium.org/1711063002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:360: assert mTabModelSelector.getModel(true).getCount() == 0; On 2016/02/29 19:55:35, dfalcantara wrote: > On 2016/02/25 19:17:32, Peter Wen wrote: > > Do asserts still work? > > Yes, on older Android versions. There's some work planned for getting them to > work on later Android versions again too. Sounds good. https://codereview.chromium.org/1711063002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:379: TabIdManager.getInstance().generateValidId(Tab.INVALID_TAB_ID)); On 2016/02/29 19:55:35, dfalcantara wrote: > I'm not sure this call will ever do anything now. This ID should always be > higher than any Tab ever opened because we store that number globally. Before > we started using the global next tab ID preference, it used to be possible to > get into a state where the computed next tab ID would be lower than all-time > maximum because a user could close a bunch of tabs. > > That said, I don't know if this call is necessary in the new world. It's > probably safe to remove. Done. https://codereview.chromium.org/1711063002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:780: createOnTabStateReadCallback(mTabModelSelector.isIncognitoSelected())); On 2016/02/29 19:55:35, dfalcantara wrote: > nit: indented incorrectly Done. https://codereview.chromium.org/1711063002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:792: public int loadStateInternalWithMaxTabId() throws IOException { On 2016/02/29 19:55:35, dfalcantara wrote: > nit: loadStateAndComputeMaxTabIdInternal? Can this be protected? Add a comment > saying that anyone calling this from outside of a test should call loadState() > instead? Switched to package private as RestoreMigrateTest still calls it.
Folded refactoring into this CL since you'll be taking another look anyways. Dropping plans to do more refactoring on TPS until post-Hera world (M51). Thanks! PTAL. :)
lgtm, I think. Trades off reading the saveState file twice for an easier to read codepath, if I'm reading this correctly.
On 2016/03/07 18:49:27, dfalcantara wrote: > lgtm, I think. Trades off reading the saveState file twice for an easier to > read codepath, if I'm reading this correctly. Yep, although since the IO stream is already warm/cached by the first read, should have no regressions in performance. We'll know by the uma stats. :)
The CQ bit was checked by wnwen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1711063002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1711063002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Avoid disk reads in TabPersistentStore for max tab_id. If we have already stored the max tab_id from a prior run, do not read all the tab state folders looking for it again. The only time this should happen is during an upgrade from a much older version of Chrome as the shared preference would not have been set yet. BUG=587941 ========== to ========== Avoid disk reads in TabPersistentStore for max tab_id. If we have already stored the max tab_id from a prior run, do not read all the tab state folders looking for it again. The only time this should happen is during an upgrade from a much older version of Chrome as the shared preference would not have been set yet. BUG=587941 Committed: https://crrev.com/bbe95ed8e2dbd8cc7f23d4bcda82b0bae2d7242e Cr-Commit-Position: refs/heads/master@{#379613} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/bbe95ed8e2dbd8cc7f23d4bcda82b0bae2d7242e Cr-Commit-Position: refs/heads/master@{#379613} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
