Closed Bug 1116599 Opened 5 years ago Closed 5 years ago

Use a cache to create a GeckoMenu.findItem fast path

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 37

People

(Reporter: mfinkle, Assigned: mfinkle)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch cache-menus v0.1 (obsolete) — Splinter Review
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.
Attachment #8542706 - Flags: review?(rnewman)
Assignee: nobody → mark.finkle
Status: NEW → ASSIGNED
OS: Mac OS X → Android
Hardware: x86 → All
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.
Attachment #8542706 - Flags: review?(rnewman) → feedback+
Attached patch cache-menus v0.2Splinter Review
Updated based on comments
Attachment #8542706 - Attachment is obsolete: true
Attachment #8542844 - Flags: review?(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!
Attachment #8542844 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/mozilla-central/rev/0d284cdc38f6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
You need to log in before you can comment on or make changes to this bug.