1116599 - Use a cache to create a GeckoMenu.findItem fast path

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: cache-menus v0.1

BrowserApp.onPrepareOptionsMenu is called many times, and in it, we call GeckoMenu.findItem(id) a lot as well. FWIW, GeckoMenu is the top level menu used in Fennec.

This patch uses a simple HashMap to create a fast path for GeckoMenu.findItem to help bypass the slow path when trying to find commonly used menuitems. I profiled this during Tab changing and the improvement is certainly measurable.

This patch won't make a fast path for every Menu.findItem, only the main menu. That seems to be our most common caller of findItem though.

Comment 1

[Richard Newman [:rnewman]]

Comment on attachment 8542706 [details] [diff] [review]
cache-menus v0.1

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

You ought to remove from the map in two places:

* removeItem(int)
* clear()

::: mobile/android/base/menu/GeckoMenu.java
@@ +90,5 @@
>      // List of all menu items.
>      private final List<GeckoMenuItem> mItems;
>  
> +    // Quick lookup map used to make a fast path in findItem.
> +    private final Map<Integer, MenuItem> mHashItems;

Use SparseArray instead. And call it mItemsById?

@@ +364,5 @@
>              mMenuPresenter.showMenu(viewForMenu);
>      }
>  
>      @Override
>      public MenuItem findItem(int id) {

I did some research, and I didn't find any callers of `findItem` off the UI thread. Which is good, because mHashItems isn't thread-safe.

Could we add:

  ThreadUtils.assertOnUiThread();

here? That'll crash in developer builds and log on release builds.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: cache-menus v0.2

Updated based on comments

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8542844 [details] [diff] [review]
cache-menus v0.2

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

LGTM. Now we see if that assertion exposes a bug!

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/0d284cdc38f6