Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
922943002:
Let multiple EnhancedBookmarkModel share same java cache (Closed)
DescriptionLet multiple EnhancedBookmarkModel share same java cache
This CL uses a static weak reference to let different models use the
same static cache. However if no model is present, the cache is exposed
to be GC'd again.
BUG=457888
Committed: https://crrev.com/04d0fb01ef6b3e6ca8a7c641d709a35bab2565dc
Cr-Commit-Position: refs/heads/master@{#317818}
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : removed cache size parameter #
Total comments: 6
Patch Set 4 : Removed old constructor #
Total comments: 4
Patch Set 5 : Preserved old signatures #
Total comments: 8
Patch Set 6 : Removed context as parameter #
Messages
Total messages: 25 (4 generated)
ianwen@chromium.org changed reviewers: + kkimlabs@chromium.org
kkimlabs@chromium.org changed reviewers: + tedchoc@chromium.org
https://codereview.chromium.org/922943002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java (right): https://codereview.chromium.org/922943002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java:74: mSalientImageCache = getImageCache(maxCacheSize); Then isn't that the cache size can be fixed to the initial call? If cache is shared, I think cache size determination logic should live in the shared place too. Maybe just inside getImageCache.
https://codereview.chromium.org/922943002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java (right): https://codereview.chromium.org/922943002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java:74: mSalientImageCache = getImageCache(maxCacheSize); On 2015/02/13 22:25:30, Kibeom Kim wrote: > Then isn't that the cache size can be fixed to the initial call? If cache is > shared, I think cache size determination logic should live in the shared place > too. Maybe just inside getImageCache. For now, max cache size calculation is done by getting the activity's app context and see how much memory is available. Since a cache is only going to be shared by entities in the same app, same device, is it necessary to move that code here? Most ideally the model should not be aware of the context...
https://codereview.chromium.org/922943002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java (right): https://codereview.chromium.org/922943002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java:74: mSalientImageCache = getImageCache(maxCacheSize); On 2015/02/13 22:35:42, Ian Wen wrote: > On 2015/02/13 22:25:30, Kibeom Kim wrote: > > Then isn't that the cache size can be fixed to the initial call? If cache is > > shared, I think cache size determination logic should live in the shared place > > too. Maybe just inside getImageCache. > > For now, max cache size calculation is done by getting the activity's app > context and see how much memory is available. Since a cache is only going to be > shared by entities in the same app, same device, is it necessary to move that > code here? Most ideally the model should not be aware of the context... I think the API problem of the current form is that, maxCacheSize can be ignored for the subsequent calls if we pass different values. If we can always respect maxCacheSize, that's good, but if not I think we shouldn't the argument in the beginning.
https://codereview.chromium.org/922943002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java (right): https://codereview.chromium.org/922943002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java:74: mSalientImageCache = getImageCache(maxCacheSize); On 2015/02/13 22:57:00, Kibeom Kim wrote: > On 2015/02/13 22:35:42, Ian Wen wrote: > > On 2015/02/13 22:25:30, Kibeom Kim wrote: > > > Then isn't that the cache size can be fixed to the initial call? If cache is > > > shared, I think cache size determination logic should live in the shared > place > > > too. Maybe just inside getImageCache. > > > > For now, max cache size calculation is done by getting the activity's app > > context and see how much memory is available. Since a cache is only going to > be > > shared by entities in the same app, same device, is it necessary to move that > > code here? Most ideally the model should not be aware of the context... > > I think the API problem of the current form is that, maxCacheSize can be ignored > for the subsequent calls if we pass different values. If we can always respect > maxCacheSize, that's good, but if not I think we shouldn't the argument in the > beginning. One solution: compare the cache size with the current one. If not equal, discard the cache and construct a new one.
noyau@chromium.org changed reviewers: + noyau@chromium.org
https://codereview.chromium.org/922943002/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java (right): https://codereview.chromium.org/922943002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java:28: private static WeakReference<LruCache<String, Pair<String, Bitmap>>> sLruCache; FYI: On iOS we also have a cache, but it is at the BookmarkImageService level. It also stores potentially smaller images. When the UI code asks for an image it passes along the size it needs the image to be (there are only two sizes really, one is inside a tile, the other is on the editor) and the properly resized image is cached and returned. So the cache is from URL+Size to ImageRecord (this also contains the precomputed dominant color). The cache is sized to be the maximum number of tiles visible on the screen at once, so that a full UI refresh (rotation for example) always hits the cache. Search for BookmarkImageServiceIOS on cs/ to see our code. Maybe we could share some of this code by moving it into the BookmarkImageService?
https://codereview.chromium.org/922943002/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java (right): https://codereview.chromium.org/922943002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java:28: private static WeakReference<LruCache<String, Pair<String, Bitmap>>> sLruCache; On 2015/02/15 10:24:09, noyau wrote: > FYI: On iOS we also have a cache, but it is at the BookmarkImageService level. > It also stores potentially smaller images. When the UI code asks for an image it > passes along the size it needs the image to be (there are only two sizes really, > one is inside a tile, the other is on the editor) and the properly resized image > is cached and returned. > > So the cache is from URL+Size to ImageRecord (this also contains the precomputed > dominant color). > > The cache is sized to be the maximum number of tiles visible on the screen at > once, so that a full UI refresh (rotation for example) always hits the cache. > > Search for BookmarkImageServiceIOS on cs/ to see our code. > > Maybe we could share some of this code by moving it into the > BookmarkImageService? Yeah I think sharing the cache will be really nice. But I think we want to cache Java bitmap (jobject) https://cs.corp.google.com/#clankium/src/chrome/browser/enhanced_bookmarks/an... in the cache to avoid format conversion every time (at least it involves one copy) , so first we need to abstract Java bitmap by gfx::Image, and it will be some work I think.
On 2015/02/17 21:25:25, Kibeom Kim wrote: > https://codereview.chromium.org/922943002/diff/40001/chrome/android/java/src/... > File > chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java > (right): > > https://codereview.chromium.org/922943002/diff/40001/chrome/android/java/src/... > chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java:28: > private static WeakReference<LruCache<String, Pair<String, Bitmap>>> sLruCache; > On 2015/02/15 10:24:09, noyau wrote: > > FYI: On iOS we also have a cache, but it is at the BookmarkImageService level. > > It also stores potentially smaller images. When the UI code asks for an image > it > > passes along the size it needs the image to be (there are only two sizes > really, > > one is inside a tile, the other is on the editor) and the properly resized > image > > is cached and returned. > > > > So the cache is from URL+Size to ImageRecord (this also contains the > precomputed > > dominant color). > > > > The cache is sized to be the maximum number of tiles visible on the screen at > > once, so that a full UI refresh (rotation for example) always hits the cache. > > > > Search for BookmarkImageServiceIOS on cs/ to see our code. > > > > Maybe we could share some of this code by moving it into the > > BookmarkImageService? > > Yeah I think sharing the cache will be really nice. But I think we want to cache > Java bitmap (jobject) > https://cs.corp.google.com/#clankium/src/chrome/browser/enhanced_bookmarks/an... > in the cache to avoid format conversion every time (at least it involves one > copy) , so first we need to abstract Java bitmap by gfx::Image, and it will be > some work I think. Aye, that's true, if you are not abstracted behind gfx::image, this become complicated.
Ping? It seems we have reached consensus that a java cache is truly needed here, right?
https://codereview.chromium.org/922943002/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModel.java (right): https://codereview.chromium.org/922943002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModel.java:66: this(0); Shoudln't we remove this?
https://codereview.chromium.org/922943002/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModel.java (right): https://codereview.chromium.org/922943002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModel.java:66: this(0); On 2015/02/23 19:11:14, Kibeom Kim wrote: > Shoudln't we remove this? You meant removing 0 as a parameter? Or remove this constructor? Calling EnhancedBookmarksBridge() and EnhancedBookmarksBridge(0) are the same. If you meant the constructor, if a detail activity is launched after the user adds a bookmark, then there is no need to construct a cache. However creating an empty cache here won't hurt that much though.
https://codereview.chromium.org/922943002/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModel.java (right): https://codereview.chromium.org/922943002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModel.java:66: this(0); Then when user opens the detail dialog from enhanced bookmark UI GridView, we're not taking advantage of the cache. Which is also current behavior but since we're unifying the cache, I think we should take an advantage. And as discussed, I think, API wise, cache size argument in EnhancedBookmarksBridge is misleading since it cannot be kept always, depending on the previous calls.
https://codereview.chromium.org/922943002/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModel.java (right): https://codereview.chromium.org/922943002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModel.java:66: this(0); On 2015/02/23 19:44:03, Kibeom Kim wrote: > Then when user opens the detail dialog from enhanced bookmark UI GridView, we're > not taking advantage of the cache. Which is also current behavior but since > we're unifying the cache, I think we should take an advantage. > > And as discussed, I think, API wise, cache size argument in > EnhancedBookmarksBridge is misleading since it cannot be kept always, depending > on the previous calls. Done.
https://codereview.chromium.org/922943002/diff/60001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java (right): https://codereview.chromium.org/922943002/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java:72: public EnhancedBookmarksBridge(Profile profile, int maxCacheSize) { at this point, why even have the size passed in here? why not calculate it in this class instead of the model? https://codereview.chromium.org/922943002/diff/60001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModel.java (right): https://codereview.chromium.org/922943002/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModel.java:64: private EnhancedBookmarksModel(int cacheSize) { keep this constructor public so this change can roll cleanly downstream...then remove it in a followup change.
https://codereview.chromium.org/922943002/diff/60001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java (right): https://codereview.chromium.org/922943002/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java:72: public EnhancedBookmarksBridge(Profile profile, int maxCacheSize) { On 2015/02/24 00:00:43, Ted C wrote: > at this point, why even have the size passed in here? why not calculate it in > this class instead of the model? Cache size was passed in because I did not want the bridge to be aware of the context. Yet since the model is already receiving the context now, I can also pass it to this bridge. Done. https://codereview.chromium.org/922943002/diff/60001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModel.java (right): https://codereview.chromium.org/922943002/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModel.java:64: private EnhancedBookmarksModel(int cacheSize) { On 2015/02/24 00:00:43, Ted C wrote: > keep this constructor public so this change can roll cleanly downstream...then > remove it in a followup change. Done.
Change seems fine to me with a couple quick comments...leaving it up for Kibeom for one quick final pass. Also, WeakReferences scare me...for no reason at all...they just do. Just wanted to get that out there :-) https://codereview.chromium.org/922943002/diff/80001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java (right): https://codereview.chromium.org/922943002/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java:82: mSalientImageCache = getImageCache(getSalientImageCacheSize(context)); I suspect you could just use ApplicationStatus#getApplicationContext here instead of needing the context since it's used for such a trivial use case. https://codereview.chromium.org/922943002/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java:280: if (maxCacheSize <= 0) return null; and we don't really need this anymore either? https://codereview.chromium.org/922943002/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java:301: if (context == null) return 0; can we avoid adding this if we use the application context? https://codereview.chromium.org/922943002/diff/80001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModel.java (right): https://codereview.chromium.org/922943002/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModel.java:66: this(null); And here I would use the application context even if you don't want it in the other file as the code shouldn't crash even in the middle of patches.
Thanks for the comments, which made the patch much better! https://codereview.chromium.org/922943002/diff/80001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java (right): https://codereview.chromium.org/922943002/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java:82: mSalientImageCache = getImageCache(getSalientImageCacheSize(context)); On 2015/02/24 01:35:26, Ted C wrote: > I suspect you could just use ApplicationStatus#getApplicationContext here > instead of needing the context since it's used for such a trivial use case. Wow did not know this method. Done. https://codereview.chromium.org/922943002/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java:280: if (maxCacheSize <= 0) return null; On 2015/02/24 01:35:27, Ted C wrote: > and we don't really need this anymore either? Done. https://codereview.chromium.org/922943002/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksBridge.java:301: if (context == null) return 0; On 2015/02/24 01:35:26, Ted C wrote: > can we avoid adding this if we use the application context? Done. https://codereview.chromium.org/922943002/diff/80001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModel.java (right): https://codereview.chromium.org/922943002/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModel.java:66: this(null); On 2015/02/24 01:35:27, Ted C wrote: > And here I would use the application context even if you don't want it in the > other file as the code shouldn't crash even in the middle of patches. Removed the constructor that required a context. Done. (It won't crash even before, I added a null check.)
lgtm
The CQ bit was checked by ianwen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/922943002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/04d0fb01ef6b3e6ca8a7c641d709a35bab2565dc Cr-Commit-Position: refs/heads/master@{#317818} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
