963873 - [Linter: UseSparseArrays] Consider using SparseArray for HashMap in AndroidGamepadManager.java

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

See bug 962968 comment 0 for the motivation. bug 962968 knocked out a few of them, but there are still some remaining [1].

Note that SparseArray finds keys via binary search, rather than hashing, so there is also a cpu-based performance difference. rnewman recommends profiling to determine how SparseArrays perform comparatively, particularly in highly traversed code paths (see bug 962968 comment 12).

[1]: https://mxr.mozilla.org/mozilla-central/search?string=Map%3CInteger&find=\.java%24&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 1

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

via lint:

 ../../src/main/java/org/mozilla/gecko/AndroidGamepadManager.java:143: Use new SparseArray<Gamepad>(...) instead for better performance

 140     public static void startup() {
 141         ThreadUtils.assertOnUiThread();
 142         if (!sStarted) {
 143             sGamepads = new HashMap<Integer, Gamepad>();
 144             sPendingGamepads = new HashMap<Integer, List<KeyEvent>>();
 145             scanForGamepads();

../../src/main/java/org/mozilla/gecko/AndroidGamepadManager.java:144: Use new SparseArray<List<KeyEvent>>(...) instead for better performance

 141         ThreadUtils.assertOnUiThread();
 142         if (!sStarted) {
 143             sGamepads = new HashMap<Integer, Gamepad>();
 144             sPendingGamepads = new HashMap<Integer, List<KeyEvent>>();
 145             scanForGamepads();
 146             addDeviceListener();

../../src/main/java/org/mozilla/gecko/Tabs.java:53: Use new SparseArray<Tab>(...) instead for better performance

  50     private volatile Tab mSelectedTab;
  51 
  52     // All accesses to mTabs must be synchronized on the Tabs instance.
  53     private final HashMap<Integer, Tab> mTabs = new HashMap<Integer, Tab>();
  54 
  55     private AccountManager mAccountManager;

Priority: 4 / 10
Category: Performance
Severity: Warning
Explanation: HashMap can be replaced with SparseArray.
For maps where the keys are of type integer, it's typically more efficient to use the Android SparseArray API. This check identifies scenarios where you might want to consider using SparseArray instead of HashMap for better performance.

This is particularly useful when the value types are primitives like ints, where you can use SparseIntArray and avoid auto-boxing the values from int to Integer.

If you need to construct a HashMap because you need to call an API outside of your control which requires a Map, you can suppress this warning using for example the @SuppressLint annotation.

Comment 2

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

Moved Tabs.java changes to bug 1175321.

Comment 3

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

Attachment: MozReview Request: Bug 963873 - Replace HashMap with SparseArray in AndroidGamepadManager. r=nalexander

Bug 963873 - Replace HashMap with SparseArray in AndroidGamepadManager. r=nalexander

I don't expect to have many devices attached to a Fennec instance so it's
unlkely we'll run into the performance issues of SparseArray when
adding/removing.

Comment 4

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8623335 [details]
MozReview Request: Bug 963873 - Replace HashMap with SparseArray in AndroidGamepadManager. r=nalexander

https://reviewboard.mozilla.org/r/11487/#review9911

Ship It!

Comment 5

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

https://hg.mozilla.org/integration/fx-team/rev/f79a2670fba4

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f79a2670fba4