Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1850793002:
Suppress SparseArray lint warning in Mojo RouterImpl (Closed)
|
Created:
4 years, 8 months ago by johnme Modified:
4 years, 8 months ago Reviewers:
yzshen1 CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSuppress SparseArray lint warning in Mojo RouterImpl
org.chromium.mojo.bindings.RouterImpl uses a HashMap<Long, MessageReceiver> to store the callbacks for all Mojo method calls.
Lint complains that this causes unnecessary garbage collection thrashing due to boxing/unboxing of the long map keys, and instead recommends to use an android.util.LongSparseArray.
But LongSparseArray is probably not suitable either, since the documentation says it's "not intended to be appropriate for data structures that may contain large numbers of items".
This patch suppresses the warning for now.
BUG=600699
Committed: https://crrev.com/4d8bdcdeae1ffd4e969e65c8cc87b4491c4d3d1f
Cr-Commit-Position: refs/heads/master@{#385201}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Suppress instead #Messages
Total messages: 15 (5 generated)
johnme@chromium.org changed reviewers: + yzshen@chromium.org
https://codereview.chromium.org/1850793002/diff/1/mojo/public/java/bindings/s... File mojo/public/java/bindings/src/org/chromium/mojo/bindings/RouterImpl.java (right): https://codereview.chromium.org/1850793002/diff/1/mojo/public/java/bindings/s... mojo/public/java/bindings/src/org/chromium/mojo/bindings/RouterImpl.java:99: private LongSparseArray<MessageReceiver> mResponders = new LongSparseArray<MessageReceiver>(); Would you please explain why this should be done using LongSparseArray? I learnt that it is generally slower than HashMap. It uses a binary search and insert/remove require modifying an array structure.
On 2016/04/04 15:31:42, yzshen1 wrote: > https://codereview.chromium.org/1850793002/diff/1/mojo/public/java/bindings/s... > File mojo/public/java/bindings/src/org/chromium/mojo/bindings/RouterImpl.java > (right): > > https://codereview.chromium.org/1850793002/diff/1/mojo/public/java/bindings/s... > mojo/public/java/bindings/src/org/chromium/mojo/bindings/RouterImpl.java:99: > private LongSparseArray<MessageReceiver> mResponders = new > LongSparseArray<MessageReceiver>(); > Would you please explain why this should be done using LongSparseArray? > > I learnt that it is generally slower than HashMap. It uses a binary search and > insert/remove require modifying an array structure. The main reason it's a lint rule is that SparseArray uses less memory & performs fewer allocations. If this is actually performance critical, then we could also add a @Suppress to silence the lint warning. http://developer.android.com/reference/android/util/SparseArray.html
On 2016/04/04 19:42:39, agrieve wrote: > On 2016/04/04 15:31:42, yzshen1 wrote: > > > https://codereview.chromium.org/1850793002/diff/1/mojo/public/java/bindings/s... > > File mojo/public/java/bindings/src/org/chromium/mojo/bindings/RouterImpl.java > > (right): > > > > > https://codereview.chromium.org/1850793002/diff/1/mojo/public/java/bindings/s... > > mojo/public/java/bindings/src/org/chromium/mojo/bindings/RouterImpl.java:99: > > private LongSparseArray<MessageReceiver> mResponders = new > > LongSparseArray<MessageReceiver>(); > > Would you please explain why this should be done using LongSparseArray? > > > > I learnt that it is generally slower than HashMap. It uses a binary search and > > insert/remove require modifying an array structure. > > The main reason it's a lint rule is that SparseArray uses less memory & performs > fewer allocations. If this is actually performance critical, then we could also > add a @Suppress to silence the lint warning. > > http://developer.android.com/reference/android/util/SparseArray.html Without any perf data, I honestly don't know which way is better. Each mojo method call which expects a callback has a responder inserted into (and later removed from, which the response arrives) the map. I would say it is quite performance critical. Do you feel like to do some profiling to find out which way is better? That would be great and I would be happy to answer any question. If you don't have free cycles to do this, a @suppress seems okay to me, too.
On 2016/04/04 20:37:00, yzshen1 wrote: > On 2016/04/04 19:42:39, agrieve wrote: > > On 2016/04/04 15:31:42, yzshen1 wrote: > > > > > > https://codereview.chromium.org/1850793002/diff/1/mojo/public/java/bindings/s... > > > File > mojo/public/java/bindings/src/org/chromium/mojo/bindings/RouterImpl.java > > > (right): > > > > > > > > > https://codereview.chromium.org/1850793002/diff/1/mojo/public/java/bindings/s... > > > mojo/public/java/bindings/src/org/chromium/mojo/bindings/RouterImpl.java:99: > > > private LongSparseArray<MessageReceiver> mResponders = new > > > LongSparseArray<MessageReceiver>(); > > > Would you please explain why this should be done using LongSparseArray? > > > > > > I learnt that it is generally slower than HashMap. It uses a binary search > and > > > insert/remove require modifying an array structure. > > > > The main reason it's a lint rule is that SparseArray uses less memory & > performs > > fewer allocations. If this is actually performance critical, then we could > also > > add a @Suppress to silence the lint warning. > > > > http://developer.android.com/reference/android/util/SparseArray.html > > Without any perf data, I honestly don't know which way is better. > Each mojo method call which expects a callback has a responder inserted into > (and later removed from, which the response arrives) the map. I would say it is > quite performance critical. > > Do you feel like to do some profiling to find out which way is better? That > would be great and I would be happy to answer any question. > If you don't have free cycles to do this, a @suppress seems okay to me, too. which the response arrives -> *when* the response arrives
Description was changed from ========== Use LongSparseArray instead of HashMap in Mojo to appease lint BUG=none ========== to ========== Suppress SparseArray lint warning in Mojo RouterImpl org.chromium.mojo.bindings.RouterImpl uses a HashMap<Long, MessageReceiver> to store the callbacks for all Mojo method calls. Lint complains that this causes unnecessary garbage collection thrashing due to boxing/unboxing of the long map keys, and instead recommends to use an android.util.LongSparseArray. But LongSparseArray is probably not suitable either, since the documentation says it's "not intended to be appropriate for data structures that may contain large numbers of items". This patch suppresses the warning for now. BUG=600699 ==========
On 2016/04/04 20:37:00, yzshen1 wrote: > On 2016/04/04 19:42:39, agrieve wrote: > > On 2016/04/04 15:31:42, yzshen1 wrote: > > > https://codereview.chromium.org/1850793002/diff/1/mojo/public/java/bindings/s... > > > mojo/public/java/bindings/src/org/chromium/mojo/bindings/RouterImpl.java:99: > > > private LongSparseArray<MessageReceiver> mResponders = new > > > LongSparseArray<MessageReceiver>(); > > > Would you please explain why this should be done using LongSparseArray? > > > > > > I learnt that it is generally slower than HashMap. It uses a binary > > > search and insert/remove require modifying an array structure. > > > > The main reason it's a lint rule is that SparseArray uses less memory & > > performs fewer allocations. If this is actually performance critical, then > > we could also add a @Suppress to silence the lint warning. > > > > http://developer.android.com/reference/android/util/SparseArray.html > > Without any perf data, I honestly don't know which way is better. > Each mojo method call which expects a callback has a responder inserted into > (and later removed from, when the response arrives) the map. I would say it > is quite performance critical. > > Do you feel like to do some profiling to find out which way is better? That > would be great and I would be happy to answer any question. > If you don't have free cycles to do this, a @suppress seems okay to me, too. Sorry, I haven't got time to profile this. From your description, it sounds that neither HashMap<Long, T> (gc thrashing due to autoboxing) nor LongSparseArray (slow remove operation) would perform well, and instead what we really need is a hybrid "LongHashMap<T>" class specialized for primitive long keys. I've filed https://crbug.com/600699 to track that, and in the meantime I've changed this patch to just suppress the warning.
lgtm
The CQ bit was checked by johnme@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1850793002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1850793002/20001
Message was sent while issue was closed.
Description was changed from ========== Suppress SparseArray lint warning in Mojo RouterImpl org.chromium.mojo.bindings.RouterImpl uses a HashMap<Long, MessageReceiver> to store the callbacks for all Mojo method calls. Lint complains that this causes unnecessary garbage collection thrashing due to boxing/unboxing of the long map keys, and instead recommends to use an android.util.LongSparseArray. But LongSparseArray is probably not suitable either, since the documentation says it's "not intended to be appropriate for data structures that may contain large numbers of items". This patch suppresses the warning for now. BUG=600699 ========== to ========== Suppress SparseArray lint warning in Mojo RouterImpl org.chromium.mojo.bindings.RouterImpl uses a HashMap<Long, MessageReceiver> to store the callbacks for all Mojo method calls. Lint complains that this causes unnecessary garbage collection thrashing due to boxing/unboxing of the long map keys, and instead recommends to use an android.util.LongSparseArray. But LongSparseArray is probably not suitable either, since the documentation says it's "not intended to be appropriate for data structures that may contain large numbers of items". This patch suppresses the warning for now. BUG=600699 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Suppress SparseArray lint warning in Mojo RouterImpl org.chromium.mojo.bindings.RouterImpl uses a HashMap<Long, MessageReceiver> to store the callbacks for all Mojo method calls. Lint complains that this causes unnecessary garbage collection thrashing due to boxing/unboxing of the long map keys, and instead recommends to use an android.util.LongSparseArray. But LongSparseArray is probably not suitable either, since the documentation says it's "not intended to be appropriate for data structures that may contain large numbers of items". This patch suppresses the warning for now. BUG=600699 ========== to ========== Suppress SparseArray lint warning in Mojo RouterImpl org.chromium.mojo.bindings.RouterImpl uses a HashMap<Long, MessageReceiver> to store the callbacks for all Mojo method calls. Lint complains that this causes unnecessary garbage collection thrashing due to boxing/unboxing of the long map keys, and instead recommends to use an android.util.LongSparseArray. But LongSparseArray is probably not suitable either, since the documentation says it's "not intended to be appropriate for data structures that may contain large numbers of items". This patch suppresses the warning for now. BUG=600699 Committed: https://crrev.com/4d8bdcdeae1ffd4e969e65c8cc87b4491c4d3d1f Cr-Commit-Position: refs/heads/master@{#385201} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4d8bdcdeae1ffd4e969e65c8cc87b4491c4d3d1f Cr-Commit-Position: refs/heads/master@{#385201} |
