Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2865963003:
[Suggestions UI] Drop Bitmap references from articles under memory pressure. (Closed)
|
Created:
3 years, 7 months ago by Bernhard Bauer Modified:
3 years, 7 months ago CC:
chromium-reviews, danakj+watch_chromium.org, agrieve+watch_chromium.org, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org, vmpstr+watch_chromium.org, memory-dev_chromium.org, Theresa, Maria Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Suggestions UI] Drop Bitmap references from articles under memory pressure.
BUG=718925
Review-Url: https://codereview.chromium.org/2865963003
Cr-Commit-Position: refs/heads/master@{#471156}
Committed: https://chromium.googlesource.com/chromium/src/+/0e283fb794e782da9af62dac7cd2aa8800db359a
Patch Set 1 #Patch Set 2 : ocmment #Patch Set 3 : fix compile error? #Patch Set 4 : comment #
Total comments: 4
Patch Set 5 : fix test #Patch Set 6 : review #
Total comments: 2
Patch Set 7 : comment #
Total comments: 4
Patch Set 8 : rename #Patch Set 9 : add missing file #Patch Set 10 : alternative approach #
Total comments: 2
Patch Set 11 : comment #Patch Set 12 : add junit test #Patch Set 13 : remove annotation #Messages
Total messages: 77 (51 generated)
The CQ bit was checked by bauerb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
The CQ bit was checked by bauerb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bauerb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bauerb@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
Please review. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by bauerb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
neat! We could also use the observer from https://codereview.chromium.org/2862893002/ to trigger pool drains if we need. https://codereview.chromium.org/2865963003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java (right): https://codereview.chromium.org/2865963003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:55: private ReferencePool.Reference<Bitmap> mThumbnailBitmap; nit: import ReferencePool.Reference? I find dotted types are less readable. https://codereview.chromium.org/2865963003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUiDelegate.java (right): https://codereview.chromium.org/2865963003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUiDelegate.java:34: * @return the reference pool to use for large objects that should be dropped under nit: we usually captitalise the first word of the comment. `@return` it is rendered as: Returns: the reference pool to ....
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
On 2017/05/08 17:01:51, dgn wrote: > neat! We could also use the observer from > https://codereview.chromium.org/2862893002/ to trigger pool drains if we need. Yes, but I would be tempted to only do this if we really are under memory pressure, as this does have an impact on the user experience. That's the same reason why I'm not just using weak references. https://codereview.chromium.org/2865963003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java (right): https://codereview.chromium.org/2865963003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:55: private ReferencePool.Reference<Bitmap> mThumbnailBitmap; On 2017/05/08 17:01:51, dgn wrote: > nit: import ReferencePool.Reference? I find dotted types are less readable. Done. Eclipse auto-import always does it like this... https://codereview.chromium.org/2865963003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUiDelegate.java (right): https://codereview.chromium.org/2865963003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUiDelegate.java:34: * @return the reference pool to use for large objects that should be dropped under On 2017/05/08 17:01:51, dgn wrote: > nit: we usually captitalise the first word of the comment. > > `@return` it is rendered as: > > Returns: > the reference pool to .... Done.
The CQ bit was checked by bauerb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nit https://codereview.chromium.org/2865963003/diff/100001/base/android/java/src/... File base/android/java/src/org/chromium/base/ReferencePool.java (right): https://codereview.chromium.org/2865963003/diff/100001/base/android/java/src/... base/android/java/src/org/chromium/base/ReferencePool.java:51: mPool.put(reference, payload); As only the keys are weakly referenced by WeakHashMap, should this here put a new WeakReference(payload) to make it robust against the payload->Reference problem?
The CQ bit was checked by bauerb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bauerb@chromium.org changed reviewers: + rmcilroy@chromium.org
Thanks! Ross, can I get an OWNERS review for base/? Also, +memory-dev@ and some other people as FYI. https://codereview.chromium.org/2865963003/diff/100001/base/android/java/src/... File base/android/java/src/org/chromium/base/ReferencePool.java (right): https://codereview.chromium.org/2865963003/diff/100001/base/android/java/src/... base/android/java/src/org/chromium/base/ReferencePool.java:51: mPool.put(reference, payload); On 2017/05/09 11:35:36, Michael van Ouwerkerk wrote: > As only the keys are weakly referenced by WeakHashMap, should this here put a > new WeakReference(payload) to make it robust against the payload->Reference > problem? As discussed offline, the WeakHashMap usually is the only thing that holds a reference to the payload, so using a WeakReference here would mean that the payload would get immediately garbage-collected the next time. I added a comment pointing out that using circular references might lead to leaks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/09 12:37:17, Bernhard Bauer wrote: > Thanks! Ross, can I get an OWNERS review for base/? > > Also, +memory-dev@ and some other people as FYI. Thanks for heads up! How bitmaps are regenerated after they are dropped?
On 2017/05/09 15:39:49, DmitrySkiba wrote: > On 2017/05/09 12:37:17, Bernhard Bauer wrote: > > Thanks! Ross, can I get an OWNERS review for base/? > > > > Also, +memory-dev@ and some other people as FYI. > > Thanks for heads up! How bitmaps are regenerated after they are dropped? They're loaded from disk, from a LevelDB for article thumbnails, and (less common) from a local file for downloaded files.
On 2017/05/09 15:59:25, Bernhard Bauer wrote: > On 2017/05/09 15:39:49, DmitrySkiba wrote: > > On 2017/05/09 12:37:17, Bernhard Bauer wrote: > > > Thanks! Ross, can I get an OWNERS review for base/? > > > > > > Also, +memory-dev@ and some other people as FYI. > > > > Thanks for heads up! How bitmaps are regenerated after they are dropped? > > They're loaded from disk, from a LevelDB for article thumbnails, and (less > common) from a local file for downloaded files. I mean, SnippetArticle.setThumbnailBitmap() is called only once, and if later the bitmap goes away, the article won't show bitmap until NTP is reloaded? Also, it seems that WeakHashMap implementation in Android won't poll the reference queue until it's touched, i.e. snippet bitmaps will stay in the pool until the next NTP page is opened, even if they are not referenced otherwise. I.e. this will increase memory usage in the normal (when not under memory pressure) case. Can we touch the pool (calling mPool.size() is enough) when NTP page goes away?
On 2017/05/09 16:25:19, DmitrySkiba wrote: > On 2017/05/09 15:59:25, Bernhard Bauer wrote: > > On 2017/05/09 15:39:49, DmitrySkiba wrote: > > > On 2017/05/09 12:37:17, Bernhard Bauer wrote: > > > > Thanks! Ross, can I get an OWNERS review for base/? > > > > > > > > Also, +memory-dev@ and some other people as FYI. > > > > > > Thanks for heads up! How bitmaps are regenerated after they are dropped? > > > > They're loaded from disk, from a LevelDB for article thumbnails, and (less > > common) from a local file for downloaded files. > > I mean, SnippetArticle.setThumbnailBitmap() is called only once, and if later > the bitmap goes away, the article won't show bitmap until NTP is reloaded? No, the thumbnail on the SnippetArticle is only used as a cache. If we bind a ViewHolder and don't have a cached Bitmap on the article, we fetch a new one. > Also, it seems that WeakHashMap implementation in Android won't poll the > reference queue until it's touched, i.e. snippet bitmaps will stay in the pool > until the next NTP page is opened, even if they are not referenced otherwise. Even when there is a garbage collection? Doesn't that kind of defeat the whole purpose of using a WeakHashMap? > I.e. this will increase memory usage in the normal (when not under memory > pressure) case. Can we touch the pool (calling mPool.size() is enough) when NTP > page goes away?
On 2017/05/09 16:39:58, Bernhard Bauer wrote: > On 2017/05/09 16:25:19, DmitrySkiba wrote: > > On 2017/05/09 15:59:25, Bernhard Bauer wrote: > > > On 2017/05/09 15:39:49, DmitrySkiba wrote: > > > > On 2017/05/09 12:37:17, Bernhard Bauer wrote: > > > > > Thanks! Ross, can I get an OWNERS review for base/? > > > > > > > > > > Also, +memory-dev@ and some other people as FYI. > > > > > > > > Thanks for heads up! How bitmaps are regenerated after they are dropped? > > > > > > They're loaded from disk, from a LevelDB for article thumbnails, and (less > > > common) from a local file for downloaded files. > > > > I mean, SnippetArticle.setThumbnailBitmap() is called only once, and if later > > the bitmap goes away, the article won't show bitmap until NTP is reloaded? > > No, the thumbnail on the SnippetArticle is only used as a cache. If we bind a > ViewHolder and don't have a cached Bitmap on the article, we fetch a new one. > > > Also, it seems that WeakHashMap implementation in Android won't poll the > > reference queue until it's touched, i.e. snippet bitmaps will stay in the pool > > until the next NTP page is opened, even if they are not referenced otherwise. > > Even when there is a garbage collection? Doesn't that kind of defeat the whole > purpose of using a WeakHashMap? Yeah, the documentation says about entries being removed once key is not in use, but see expungeStaleEntries() in https://android.googlesource.com/platform/libcore.git/+/master/ojluni/src/mai... - it's the only place where 'queue' is polled, and the only place where map finds out about collected keys. And there is no thread to do that periodically (I guess you could start a thread in the pool to periodically poke the map). > > > I.e. this will increase memory usage in the normal (when not under memory > > pressure) case. Can we touch the pool (calling mPool.size() is enough) when > NTP > > page goes away?
On 2017/05/09 17:07:42, DmitrySkiba wrote: > On 2017/05/09 16:39:58, Bernhard Bauer wrote: > > On 2017/05/09 16:25:19, DmitrySkiba wrote: > > > On 2017/05/09 15:59:25, Bernhard Bauer wrote: > > > > On 2017/05/09 15:39:49, DmitrySkiba wrote: > > > > > On 2017/05/09 12:37:17, Bernhard Bauer wrote: > > > > > > Thanks! Ross, can I get an OWNERS review for base/? > > > > > > > > > > > > Also, +memory-dev@ and some other people as FYI. > > > > > > > > > > Thanks for heads up! How bitmaps are regenerated after they are dropped? > > > > > > > > They're loaded from disk, from a LevelDB for article thumbnails, and (less > > > > common) from a local file for downloaded files. > > > > > > I mean, SnippetArticle.setThumbnailBitmap() is called only once, and if > later > > > the bitmap goes away, the article won't show bitmap until NTP is reloaded? > > > > No, the thumbnail on the SnippetArticle is only used as a cache. If we bind a > > ViewHolder and don't have a cached Bitmap on the article, we fetch a new one. > > > > > Also, it seems that WeakHashMap implementation in Android won't poll the > > > reference queue until it's touched, i.e. snippet bitmaps will stay in the > pool > > > until the next NTP page is opened, even if they are not referenced > otherwise. > > > > Even when there is a garbage collection? Doesn't that kind of defeat the whole > > purpose of using a WeakHashMap? > > Yeah, the documentation says about entries being removed once key is not in use, > but see expungeStaleEntries() in > https://android.googlesource.com/platform/libcore.git/+/master/ojluni/src/mai... > - it's the only place where 'queue' is polled, and the only place where map > finds out about collected keys. And there is no thread to do that periodically > (I guess you could start a thread in the pool to periodically poke the map). So, the Android maintainer for WeakHashMap happens to sit in the same office, so I asked them :-D It turns out that the queue does contain stale entries, but they are only _empty_ WeakReferences. The referents and the values will have been garbage-collected as expected. (I think otherwise a WeakHashMap would be far less useful.) And given that the bulk of memory is in the bitmaps (the values), I think having the empty references around until the next time the pool is touched is acceptable. WDYT?
LGTM with a naming suggestion. https://codereview.chromium.org/2865963003/diff/120001/base/android/java/src/... File base/android/java/src/org/chromium/base/ReferencePool.java (right): https://codereview.chromium.org/2865963003/diff/120001/base/android/java/src/... base/android/java/src/org/chromium/base/ReferencePool.java:28: public class ReferencePool { nit - not sure about the name. How about DiscardableReferencePool (similar to DiscardableMemory in C++ land). https://codereview.chromium.org/2865963003/diff/120001/base/android/java/src/... base/android/java/src/org/chromium/base/ReferencePool.java:39: public class Reference<T> { nit - DiscardableReference to make it clear it could return null even if it was set?
The CQ bit was checked by bauerb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/10 10:37:35, Bernhard Bauer wrote: > On 2017/05/09 17:07:42, DmitrySkiba wrote: > > On 2017/05/09 16:39:58, Bernhard Bauer wrote: > > > On 2017/05/09 16:25:19, DmitrySkiba wrote: > > > > On 2017/05/09 15:59:25, Bernhard Bauer wrote: > > > > > On 2017/05/09 15:39:49, DmitrySkiba wrote: > > > > > > On 2017/05/09 12:37:17, Bernhard Bauer wrote: > > > > > > > Thanks! Ross, can I get an OWNERS review for base/? > > > > > > > > > > > > > > Also, +memory-dev@ and some other people as FYI. > > > > > > > > > > > > Thanks for heads up! How bitmaps are regenerated after they are > dropped? > > > > > > > > > > They're loaded from disk, from a LevelDB for article thumbnails, and > (less > > > > > common) from a local file for downloaded files. > > > > > > > > I mean, SnippetArticle.setThumbnailBitmap() is called only once, and if > > later > > > > the bitmap goes away, the article won't show bitmap until NTP is reloaded? > > > > > > No, the thumbnail on the SnippetArticle is only used as a cache. If we bind > a > > > ViewHolder and don't have a cached Bitmap on the article, we fetch a new > one. > > > > > > > Also, it seems that WeakHashMap implementation in Android won't poll the > > > > reference queue until it's touched, i.e. snippet bitmaps will stay in the > > pool > > > > until the next NTP page is opened, even if they are not referenced > > otherwise. > > > > > > Even when there is a garbage collection? Doesn't that kind of defeat the > whole > > > purpose of using a WeakHashMap? > > > > Yeah, the documentation says about entries being removed once key is not in > use, > > but see expungeStaleEntries() in > > > https://android.googlesource.com/platform/libcore.git/+/master/ojluni/src/mai... > > - it's the only place where 'queue' is polled, and the only place where map > > finds out about collected keys. And there is no thread to do that periodically > > (I guess you could start a thread in the pool to periodically poke the map). > > So, the Android maintainer for WeakHashMap happens to sit in the same office, so > I asked them :-D It turns out that the queue does contain stale entries, but > they are only _empty_ WeakReferences. The referents and the values will have > been garbage-collected as expected. (I think otherwise a WeakHashMap would be > far less useful.) And given that the bulk of memory is in the bitmaps (the > values), I think having the empty references around until the next time the pool > is touched is acceptable. WDYT? I still don't see how exactly 'value' is unreferenced when corresponding key is GCed. I'm probably missing something. I see that: * WeakHashMap has 'table' variable, which is an array of Entries * Entry is an inner class, which inherits from WeakReference * Entry is constructed with (key, value) pair (among other things) and propagates 'key' to the inherited WeakReference, but stores 'value' in a normal reference * I.e. Entry weakly references key, but strongly references value * Thus, all values are strongly referenced via WeakHashMap.table ->Entry.value->value chain This is how WeakHashMap knows when key was GCed: * WeakHashMap.queue is a ReferenceQueue, it's passed to Entry and then to its WeakReference base class * I.e. each Entry is registered in the queue (since Entry is-a WeakReference) * expungeStaleEntries() polls the queue (which returns GCed weak references) and casts the return value to Entry (this downcasts from WeakReference to Entry) * Non-null entries returned from poll() are those which keys were GCed * Such entries are removed from WeakHashMap.table breaking the reference chain to Entry.value * expungeStaleEntries() also has explicit 'value = null' statement to further break the chain * Once the chain is broken, value is free to be GCed Given that I don't see how values are unreferenced without calling expungeStaleEntries() (which is called by most public methods).
The CQ bit was checked by bauerb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bauerb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #10 (id:180001) has been deleted
The CQ bit was checked by bauerb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bauerb@chromium.org changed reviewers: + thierer@google.com
On 2017/05/10 16:25:08, DmitrySkiba wrote: > On 2017/05/10 10:37:35, Bernhard Bauer wrote: > > On 2017/05/09 17:07:42, DmitrySkiba wrote: > > > On 2017/05/09 16:39:58, Bernhard Bauer wrote: > > > > On 2017/05/09 16:25:19, DmitrySkiba wrote: > > > > > On 2017/05/09 15:59:25, Bernhard Bauer wrote: > > > > > > On 2017/05/09 15:39:49, DmitrySkiba wrote: > > > > > > > On 2017/05/09 12:37:17, Bernhard Bauer wrote: > > > > > > > > Thanks! Ross, can I get an OWNERS review for base/? > > > > > > > > > > > > > > > > Also, +memory-dev@ and some other people as FYI. > > > > > > > > > > > > > > Thanks for heads up! How bitmaps are regenerated after they are > > dropped? > > > > > > > > > > > > They're loaded from disk, from a LevelDB for article thumbnails, and > > (less > > > > > > common) from a local file for downloaded files. > > > > > > > > > > I mean, SnippetArticle.setThumbnailBitmap() is called only once, and if > > > later > > > > > the bitmap goes away, the article won't show bitmap until NTP is > reloaded? > > > > > > > > No, the thumbnail on the SnippetArticle is only used as a cache. If we > bind > > a > > > > ViewHolder and don't have a cached Bitmap on the article, we fetch a new > > one. > > > > > > > > > Also, it seems that WeakHashMap implementation in Android won't poll the > > > > > reference queue until it's touched, i.e. snippet bitmaps will stay in > the > > > pool > > > > > until the next NTP page is opened, even if they are not referenced > > > otherwise. > > > > > > > > Even when there is a garbage collection? Doesn't that kind of defeat the > > whole > > > > purpose of using a WeakHashMap? > > > > > > Yeah, the documentation says about entries being removed once key is not in > > use, > > > but see expungeStaleEntries() in > > > > > > https://android.googlesource.com/platform/libcore.git/+/master/ojluni/src/mai... > > > - it's the only place where 'queue' is polled, and the only place where map > > > finds out about collected keys. And there is no thread to do that > periodically > > > (I guess you could start a thread in the pool to periodically poke the map). > > > > So, the Android maintainer for WeakHashMap happens to sit in the same office, > so > > I asked them :-D It turns out that the queue does contain stale entries, but > > they are only _empty_ WeakReferences. The referents and the values will have > > been garbage-collected as expected. (I think otherwise a WeakHashMap would be > > far less useful.) And given that the bulk of memory is in the bitmaps (the > > values), I think having the empty references around until the next time the > pool > > is touched is acceptable. WDYT? > > I still don't see how exactly 'value' is unreferenced when corresponding key is > GCed. I'm probably missing something. I see that: > > * WeakHashMap has 'table' variable, which is an array of Entries > * Entry is an inner class, which inherits from WeakReference > * Entry is constructed with (key, value) pair (among other things) and > propagates 'key' to the inherited WeakReference, but stores 'value' in a normal > reference > * I.e. Entry weakly references key, but strongly references value > * Thus, all values are strongly referenced via WeakHashMap.table > ->Entry.value->value chain > > This is how WeakHashMap knows when key was GCed: > > * WeakHashMap.queue is a ReferenceQueue, it's passed to Entry and then to its > WeakReference base class > * I.e. each Entry is registered in the queue (since Entry is-a WeakReference) > * expungeStaleEntries() polls the queue (which returns GCed weak references) and > casts the return value to Entry (this downcasts from WeakReference to Entry) > * Non-null entries returned from poll() are those which keys were GCed > * Such entries are removed from WeakHashMap.table breaking the reference chain > to Entry.value > * expungeStaleEntries() also has explicit 'value = null' statement to further > break the chain > * Once the chain is broken, value is free to be GCed > > Given that I don't see how values are unreferenced without calling > expungeStaleEntries() (which is called by most public methods). Hm, good point. +thierer, is the value of a WeakHashMap.Entry cleared on GC, or did I misunderstand something? In any case, as the _referent_ should be cleared (right? At this point I'm not sure about anything anymore 😉), there is an alternative approach that stores the payload in the key (at the expense of having to iterate over all entries to drop the payloads), which I've uploaded as the latest patch set. Could you PTAL at that one too? Thanks! https://codereview.chromium.org/2865963003/diff/120001/base/android/java/src/... File base/android/java/src/org/chromium/base/ReferencePool.java (right): https://codereview.chromium.org/2865963003/diff/120001/base/android/java/src/... base/android/java/src/org/chromium/base/ReferencePool.java:28: public class ReferencePool { On 2017/05/10 14:38:11, rmcilroy wrote: > nit - not sure about the name. How about DiscardableReferencePool (similar to > DiscardableMemory in C++ land). Good idea! Done. I kept variable and member names as referencePool just to keep things a bit shorter. https://codereview.chromium.org/2865963003/diff/120001/base/android/java/src/... base/android/java/src/org/chromium/base/ReferencePool.java:39: public class Reference<T> { On 2017/05/10 14:38:11, rmcilroy wrote: > nit - DiscardableReference to make it clear it could return null even if it was > set? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
dskiba@chromium.org changed reviewers: + dskiba@chromium.org
New approach (with Set) LGTM. Btw you should probably mention that the pool is not thread safe, just in case. https://codereview.chromium.org/2865963003/diff/200001/base/android/java/src/... File base/android/java/src/org/chromium/base/DiscardableReferencePool.java (right): https://codereview.chromium.org/2865963003/diff/200001/base/android/java/src/... base/android/java/src/org/chromium/base/DiscardableReferencePool.java:23: * <p>Note that certain kinds of reference cycles that would be garbage-collected when using weak This is not accurate anymore, right? Since now the pool holds weak references.
The CQ bit was checked by bauerb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! This was very enlightening :) https://codereview.chromium.org/2865963003/diff/200001/base/android/java/src/... File base/android/java/src/org/chromium/base/DiscardableReferencePool.java (right): https://codereview.chromium.org/2865963003/diff/200001/base/android/java/src/... base/android/java/src/org/chromium/base/DiscardableReferencePool.java:23: * <p>Note that certain kinds of reference cycles that would be garbage-collected when using weak On 2017/05/10 22:04:19, DmitrySkiba wrote: > This is not accurate anymore, right? Since now the pool holds weak references. True. Removed (and added a comment about thread safety).
New approach looks fine, but could we have some tests please?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by bauerb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/11 10:12:23, rmcilroy wrote: > New approach looks fine, but could we have some tests please? Added some JUnit tests. They rely on the fact that the runtime will actually do a GC when calling Runtime.gc(), so if that should change at some point in the future, they will get flaky :-/ I'm not sure how to do this in a better way though.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
LGTM.
The CQ bit was checked by bauerb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mvanouwerkerk@chromium.org, dskiba@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/2865963003/#ps260001 (title: "remove annotation")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/05/11 11:21:41, Bernhard Bauer wrote: > On 2017/05/11 10:12:23, rmcilroy wrote: > > New approach looks fine, but could we have some tests please? > > Added some JUnit tests. They rely on the fact that the runtime will actually do > a GC when calling Runtime.gc(), so if that should change at some point in the > future, they will get flaky :-/ I'm not sure how to do this in a better way > though. I asked Android guys about this, and they replied with: " System.gc has been a no-op for a while unless it is followed by or following a call to System.runFinalization(). Something like: System.gc(); System.runFinalization(); System.gc(); Should be sufficient, this is the logic strict mode uses for checking instance counts. " (I'm writing a test to make sure we're not leaking activities, see crcrev.com/2873153004.)
On 2017/05/11 15:31:08, DmitrySkiba wrote: > On 2017/05/11 11:21:41, Bernhard Bauer wrote: > > On 2017/05/11 10:12:23, rmcilroy wrote: > > > New approach looks fine, but could we have some tests please? > > > > Added some JUnit tests. They rely on the fact that the runtime will actually > do > > a GC when calling Runtime.gc(), so if that should change at some point in the > > future, they will get flaky :-/ I'm not sure how to do this in a better way > > though. > > I asked Android guys about this, and they replied with: > > " > System.gc has been a no-op for a while unless it is followed by or following a > call to System.runFinalization(). > > Something like: > System.gc(); > System.runFinalization(); > System.gc(); > > Should be sufficient, this is the logic strict mode uses for checking instance > counts. > " > > (I'm writing a test to make sure we're not leaking activities, see > crcrev.com/2873153004.) Thanks for looking into that! Does the fact that the call in question is in JUnit test code, which is only run on the host, change that consideration? Otherwise I'm happy to make that change.
On 2017/05/11 16:55:18, Bernhard Bauer wrote: > On 2017/05/11 15:31:08, DmitrySkiba wrote: > > On 2017/05/11 11:21:41, Bernhard Bauer wrote: > > > On 2017/05/11 10:12:23, rmcilroy wrote: > > > > New approach looks fine, but could we have some tests please? > > > > > > Added some JUnit tests. They rely on the fact that the runtime will actually > > do > > > a GC when calling Runtime.gc(), so if that should change at some point in > the > > > future, they will get flaky :-/ I'm not sure how to do this in a better way > > > though. > > > > I asked Android guys about this, and they replied with: > > > > " > > System.gc has been a no-op for a while unless it is followed by or following a > > call to System.runFinalization(). > > > > Something like: > > System.gc(); > > System.runFinalization(); > > System.gc(); > > > > Should be sufficient, this is the logic strict mode uses for checking instance > > counts. > > " > > > > (I'm writing a test to make sure we're not leaking activities, see > > crcrev.com/2873153004.) > > Thanks for looking into that! Does the fact that the call in question is in > JUnit test code, which is only run on the host, change that consideration? > Otherwise I'm happy to make that change. Oops, didn't noticed that :) Don't know much about "big java", so I guess let's leave it as is.
The CQ bit was checked by bauerb@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1494535063927220,
"parent_rev": "586c7bd9686d7bad11862589b52329bbf94059c3", "commit_rev":
"0e283fb794e782da9af62dac7cd2aa8800db359a"}
Message was sent while issue was closed.
Description was changed from ========== [Suggestions UI] Drop Bitmap references from articles under memory pressure. BUG=718925 ========== to ========== [Suggestions UI] Drop Bitmap references from articles under memory pressure. BUG=718925 Review-Url: https://codereview.chromium.org/2865963003 Cr-Commit-Position: refs/heads/master@{#471156} Committed: https://chromium.googlesource.com/chromium/src/+/0e283fb794e782da9af62dac7cd2... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:260001) as https://chromium.googlesource.com/chromium/src/+/0e283fb794e782da9af62dac7cd2... |
