|
|
DescriptionStore last saved tab metadata to avoid resaving.
- Do not re-save to disk if new metadata is the same as the old.
- Add stats to determine where saveState is spending most of its
time.
- Move to using uptimeMillis to avoid stats being influenced by time
spent in sleep.
BUG=612574
Committed: https://crrev.com/6a48a177e5b82fa730438097673c61ca24c9af29
Cr-Commit-Position: refs/heads/master@{#400665}
Patch Set 1 #Patch Set 2 : Add more UMA stats. #
Total comments: 6
Patch Set 3 : Add histogram for array length. #
Total comments: 2
Patch Set 4 : ) #
Messages
Total messages: 16 (6 generated)
wnwen@chromium.org changed reviewers: + dfalcantara@chromium.org
This will increase memory usage (by a couple KB depending on how many tabs are open), but I'd like to try it out at least on one or two dev releases so we can see how much time can be saved by not writing the metadata twice. WDYT? Storing something like Arrays.hashCode() is faster, but there may be collisions, however rare... Using a more complex hashing algorithm would help, but use more time. Storing a dirty bit for each tab model is another option.
Description was changed from ========== Store last saved tab metadata to avoid resaving. - Do not re-save to disk if new metadata is the same as the old. BUG=612574 ========== to ========== Store last saved tab metadata to avoid resaving. - Do not re-save to disk if new metadata is the same as the old. - Add stats to determine where saveState is spending most of its time. - Move to using uptimeMillis to avoid stats being influenced by time spent in sleep. BUG=612574 ==========
https://codereview.chromium.org/2057733002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/2057733002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:277: byte[] currentMetadata = serializeTabMetadata(); If we're already doing this, can we record how large the byte array is? There are some bugs involving trying to save metadata files that are too large, or merging two different ones together, and I'm curious to see just how much data we're storing here. https://codereview.chromium.org/2057733002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:278: if (!Arrays.equals(mLastSavedMetadata, currentMetadata)) { Any idea how expensive this operation is versus the simple hash you mentioned? Seems like some simple things to check would be array lengths before going for a full out equality check, but I don't know how the Arrays.equals algorithm. Also, it seems like the correct place for this would be in saveListToFile, because that's called from more places. IIRC this function is only called when the app is being minimized.
cc+twellington for TPS metadata size FYI. https://codereview.chromium.org/2057733002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/2057733002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:277: byte[] currentMetadata = serializeTabMetadata(); On 2016/06/14 17:11:32, dfalcantara wrote: > If we're already doing this, can we record how large the byte array is? There > are some bugs involving trying to save metadata files that are too large, or > merging two different ones together, and I'm curious to see just how much data > we're storing here. Done. https://codereview.chromium.org/2057733002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:278: if (!Arrays.equals(mLastSavedMetadata, currentMetadata)) { On 2016/06/14 17:11:32, dfalcantara wrote: > Any idea how expensive this operation is versus the simple hash you mentioned? > Seems like some simple things to check would be array lengths before going for a > full out equality check, but I don't know how the Arrays.equals algorithm. > > Also, it seems like the correct place for this would be in saveListToFile, > because that's called from more places. IIRC this function is only called when > the app is being minimized. Arrays.equals already checks for pointer equality and length, and then just does a direct loop comparing each byte. This is likely to be at most a 10-20k element tight forloop breaking on inequality, should be super fast. Arrays.hashCode will likely suffice for 99.9999999% of the cases, but can result in collisions and not updating when we should. :-/ Moved to saveListToFile.
lgtm https://codereview.chromium.org/2057733002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/2057733002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:278: if (!Arrays.equals(mLastSavedMetadata, currentMetadata)) { On 2016/06/14 18:18:03, Peter Wen wrote: > On 2016/06/14 17:11:32, dfalcantara wrote: > > Any idea how expensive this operation is versus the simple hash you mentioned? > > > Seems like some simple things to check would be array lengths before going for > a > > full out equality check, but I don't know how the Arrays.equals algorithm. > > > > Also, it seems like the correct place for this would be in saveListToFile, > > because that's called from more places. IIRC this function is only called > when > > the app is being minimized. > > Arrays.equals already checks for pointer equality and length, and then just does > a direct loop comparing each byte. This is likely to be at most a 10-20k element > tight forloop breaking on inequality, should be super fast. Arrays.hashCode will > likely suffice for 99.9999999% of the cases, but can result in collisions and > not updating when we should. :-/ > > Moved to saveListToFile. Yeah, I figured the hash code would be _worse_ because it'd have to compute a hash code based on all the elements in the array. https://codereview.chromium.org/2057733002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/2057733002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:721: if (!Arrays.equals(mLastSavedMetadata, listData)) { if (Arrays.equals(...)) return true; saveListToFile etc etc
wnwen@chromium.org changed reviewers: + holte@chromium.org
+holte@ for histogram OWNERS, one new histogram for file sizes and two for timing. https://codereview.chromium.org/2057733002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/2057733002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:278: if (!Arrays.equals(mLastSavedMetadata, currentMetadata)) { On 2016/06/14 20:20:58, dfalcantara wrote: > On 2016/06/14 18:18:03, Peter Wen wrote: > > On 2016/06/14 17:11:32, dfalcantara wrote: > > > Any idea how expensive this operation is versus the simple hash you > mentioned? > > > > > Seems like some simple things to check would be array lengths before going > for > > a > > > full out equality check, but I don't know how the Arrays.equals algorithm. > > > > > > Also, it seems like the correct place for this would be in saveListToFile, > > > because that's called from more places. IIRC this function is only called > > when > > > the app is being minimized. > > > > Arrays.equals already checks for pointer equality and length, and then just > does > > a direct loop comparing each byte. This is likely to be at most a 10-20k > element > > tight forloop breaking on inequality, should be super fast. Arrays.hashCode > will > > likely suffice for 99.9999999% of the cases, but can result in collisions and > > not updating when we should. :-/ > > > > Moved to saveListToFile. > > Yeah, I figured the hash code would be _worse_ because it'd have to compute a > hash code based on all the elements in the array. Right, as it's doing a multiplication for every element. So it would just save a very small amount of memory (which the histogram will confirm). https://codereview.chromium.org/2057733002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/2057733002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:721: if (!Arrays.equals(mLastSavedMetadata, listData)) { On 2016/06/14 20:20:58, dfalcantara wrote: > if (Arrays.equals(...)) return true; > > saveListToFile etc etc Done.
@holte - friendly ping. 🎀
On 2016/06/16 15:26:34, Peter Wen wrote: > @holte - friendly ping. 🎀 lgtm
The CQ bit was checked by wnwen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2057733002/#ps60001 (title: ")")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2057733002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Store last saved tab metadata to avoid resaving. - Do not re-save to disk if new metadata is the same as the old. - Add stats to determine where saveState is spending most of its time. - Move to using uptimeMillis to avoid stats being influenced by time spent in sleep. BUG=612574 ========== to ========== Store last saved tab metadata to avoid resaving. - Do not re-save to disk if new metadata is the same as the old. - Add stats to determine where saveState is spending most of its time. - Move to using uptimeMillis to avoid stats being influenced by time spent in sleep. BUG=612574 Committed: https://crrev.com/6a48a177e5b82fa730438097673c61ca24c9af29 Cr-Commit-Position: refs/heads/master@{#400665} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6a48a177e5b82fa730438097673c61ca24c9af29 Cr-Commit-Position: refs/heads/master@{#400665} |