962968 - Consider using SparseArray instead of HashMap for numeric keys

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

----
# Use optimized data containers

Take advantage of optimized containers in the Android framework, such as SparseArray, SparseBooleanArray, and LongSparseArray. The generic HashMap implementation can be quite memory inefficient because it needs a separate entry object for every mapping. Additionally, the SparseArray classes are more efficient because they avoid the system's need to autobox the key and sometimes value (which creates yet another object or two per entry). And don't be afraid of dropping down to raw arrays when that makes sense.
----

mobile/android/base/favicons/decoders/ICODecoder.java
155:        HashMap<Integer, IconDirectoryEntry> preferenceMap = new HashMap<Integer, IconDirectoryEntry>();

mobile/android/base/favicons/Favicons.java
65:    private static final Map<Integer, LoadFaviconTask> sLoadTasks = Collections.synchronizedMap(new HashMap<Integer, LoadFaviconTask>());

mobile/android/base/GeckoJavaSampler.java
66:        private Map<Integer,Sample[]> mSamples = new HashMap<Integer,Sample[]>();

mobile/android/base/home/PanelManager.java
54:    private static final Map<Integer, RequestCallback> sCallbacks = Collections.synchronizedMap(new HashMap<Integer, RequestCallback>());

mobile/android/base/PrefsHelper.java
27:    private static final Map<Integer, PrefHandler> sCallbacks = new HashMap<Integer, PrefHandler>();

mobile/android/base/util/ActivityResultHandlerMap.java
11:    private Map<Integer, ActivityResultHandler> mMap = new HashMap<Integer, ActivityResultHandler>();

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Replace HashMap with SparseArray in ICODecoder (r=rnewman)

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Replace HashMap with SparseArray in Favicons (r=rnewman)

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Replace HashMap with SparseArray in GeckoJavaSampler (r=rnewman)

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: Replace HashMap with SparseArray in PanelManager (r=rnewman)

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: Replace HashMap with SparseArray in PrefsHelper (r=rnewman)

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: Replace HashMap with SparseArray in ActivityResultHandlerMap (r=rnewman)

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8364418 [details] [diff] [review]
Replace HashMap with SparseArray in GeckoJavaSampler (r=rnewman)

Review of attachment 8364418 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine, but looking at the code it seems like GeckoJavaSampler should be rewritten to do something else -- ThreadLocals, maybe.

Comment 8

[Richard Newman [:rnewman]]

Thanks for tackling this so quickly, Lucas!

Now we'll see how much of a saving it is…

Comment 9

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/65bfef117974
https://hg.mozilla.org/integration/fx-team/rev/4d79320f696e
https://hg.mozilla.org/integration/fx-team/rev/6944859764c3
https://hg.mozilla.org/integration/fx-team/rev/f8cb6d26dd7d
https://hg.mozilla.org/integration/fx-team/rev/533642b8d6df
https://hg.mozilla.org/integration/fx-team/rev/5d550701bd49

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/65bfef117974
https://hg.mozilla.org/mozilla-central/rev/4d79320f696e
https://hg.mozilla.org/mozilla-central/rev/6944859764c3
https://hg.mozilla.org/mozilla-central/rev/f8cb6d26dd7d
https://hg.mozilla.org/mozilla-central/rev/533642b8d6df
https://hg.mozilla.org/mozilla-central/rev/5d550701bd49

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Any reason you stopped at those six examples, Richard? Ex: [1] [2]

On that note, we can probably be more explicit about setting initial sizes too, such as in [1] where I imagine we have 6 results.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/TopSitesPanel.java#780
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.java#50

Comment 12

[Richard Newman [:rnewman]]

(In reply to Michael Comella (:mcomella) from comment #11)
> Any reason you stopped at those six examples, Richard? Ex: [1] [2]

Didn't want to keep copy-pasting from grep :D


> On that note, we can probably be more explicit about setting initial sizes
> too, such as in [1] where I imagine we have 6 results.

Concur.


> [1]:
> https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/
> TopSitesPanel.java#780

Err, this is a HashMap<String, Bitmap>…


> [2]:
> https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.
> java#50

HashMap is sometimes faster for lookup than SparseArray (depends on size); we should profile before attacking this kind of hot-path stuff.

File two follow-ups?

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Richard Newman [:rnewman] from comment #12)
> File two follow-ups?

bug 963863 - initialCapacity
bug 963873 - SparseArray (again)