1116615 - Cleanup GeckoMenu and GeckoMenuItem

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

GeckoMenuItem has a bunch of property setters. Those unconditionally call GeckoMenu.onItemChanged to update the UI. Given that the property setters are in a hot path, we should only fire onItemChanged if the property value has actually changed.

GeckoMenu.onItemChanged has a UI thread assertion, but still spawns a runnable on the UI thread. We can avoid the runnable.

Patches coming.

Comment 1

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

Attachment: notify-actual-menuchange v0.1

Only call onItemChanged if the property really changed. I skipped the check for comparing Bitmaps.

Comment 2

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

Attachment: remove-runnable v0.1

Removes the runnable.

Tested on tablets and phone. Menus still update correctly across web content and abut:home and web content with a search engine.

Comment 3

[Richard Newman [:rnewman]]

Also probably worth coalescing/batching/doing explicit change calls. We do stuff like this:

        menu.findItem(R.id.top_sites_edit).setVisible(false);
        menu.findItem(R.id.top_sites_pin).setVisible(false);
        menu.findItem(R.id.top_sites_unpin).setVisible(false);

and

            share.setEnabled(false);
            saveAsPDF.setEnabled(false);
            findInPage.setEnabled(false);

and

        bookmark.setEnabled(!AboutPages.isAboutReader(tab.getURL()));
        bookmark.setVisible(!GeckoProfile.get(this).inGuestMode());
        bookmark.setCheckable(true);
        bookmark.setChecked(tab.isBookmark());
        bookmark.setIcon(resolveBookmarkIconID(tab.isBookmark()));

and

        desktopMode.setChecked(tab.getDesktopMode());
        desktopMode.setIcon(tab.getDesktopMode() ? R.drawable.ic_menu_desktop_mode_on : R.drawable.ic_menu_desktop_mode_off);


-- these will result in onItemChanged being called way more frequently than necessary.

One pattern for this would be to add:

  private volatile boolean shouldIssueChanges;
  private volatile boolean didChange;
  public void beginBatch() {
    didChange = false;
    shouldIssueChanges = false;
  }
  public void endBatch() {
    // On UI thread.
    if (didChange) {
      mMenu.onItemChanged(this);
    }
    shouldIssueChanges = true;
  }

then:

    @Override
    public MenuItem setVisible(boolean visible) {
        mVisible = visible;
        if (shouldIssueChanges) {
            mMenu.onItemChanged(this);
        } else {
            didChange = true;
        }
        return this;
    }

and callers can do this:

+       bookmark.beginBatch();
        bookmark.setEnabled(!AboutPages.isAboutReader(tab.getURL()));
        bookmark.setVisible(!GeckoProfile.get(this).inGuestMode());
        bookmark.setCheckable(true);
        bookmark.setChecked(tab.isBookmark());
        bookmark.setIcon(resolveBookmarkIconID(tab.isBookmark()));
+       bookmark.endBatch();

Comment 4

[Richard Newman [:rnewman]]

We could also put that batching into the parent menu, so we could basically take it out of the change system until we're done futzing with all of its children.

Comment 5

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

(In reply to Richard Newman [:rnewman] from comment #3)
> Also probably worth coalescing/batching/doing explicit change calls. We do
> stuff like this:
> 
>         bookmark.setEnabled(!AboutPages.isAboutReader(tab.getURL()));
>         bookmark.setVisible(!GeckoProfile.get(this).inGuestMode());
>         bookmark.setCheckable(true);
>         bookmark.setChecked(tab.isBookmark());
>         bookmark.setIcon(resolveBookmarkIconID(tab.isBookmark()));

> One pattern for this would be to add:
> 
>   private volatile boolean shouldIssueChanges;
>   private volatile boolean didChange;
>   public void beginBatch() {
>     didChange = false;
>     shouldIssueChanges = false;
>   }
>   public void endBatch() {
>     // On UI thread.
>     if (didChange) {
>       mMenu.onItemChanged(this);
>     }
>     shouldIssueChanges = true;
>   }
> 
> then:
> 
>     @Override
>     public MenuItem setVisible(boolean visible) {
>         mVisible = visible;
>         if (shouldIssueChanges) {
>             mMenu.onItemChanged(this);
>         } else {
>             didChange = true;
>         }
>         return this;
>     }
> 
> and callers can do this:
> 
> +       bookmark.beginBatch();
>         bookmark.setEnabled(!AboutPages.isAboutReader(tab.getURL()));
>         bookmark.setVisible(!GeckoProfile.get(this).inGuestMode());
>         bookmark.setCheckable(true);
>         bookmark.setChecked(tab.isBookmark());
>         bookmark.setIcon(resolveBookmarkIconID(tab.isBookmark()));
> +       bookmark.endBatch();

This seems like a good idea, maybe for a followup. As you point out in comment 4, we'd need to add something to GeckoMenu itself to catch most of the updates since only "bookmark" and "desktopMode" make multiple calls on the same MenuItem. Adding code to GeckoMenu adds complexity, if we want to track only the MenuItems that need to be updated when the batch ends.

You asked about the number of property setter calls so I tested with my patches:

Startup: 80 calls
Tab switch: 7 to 10 calls based on the type of web content in the tabs.

Comment 6

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

(In reply to Mark Finkle (:mfinkle) from comment #5)

> You asked about the number of property setter calls so I tested with my
> patches:
> 
> Startup: 80 calls
> Tab switch: 7 to 10 calls based on the type of web content in the tabs.

I wonder if we could set the menu.xml to reasonable defaults to minimize the number of onItemUpdate calls during startup?

Comment 7

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f4e2c4c7474

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8542736 [details] [diff] [review]
notify-actual-menuchange v0.1

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

::: mobile/android/base/menu/GeckoMenuItem.java
@@ +271,5 @@
>      }
>  
>      @Override
>      public MenuItem setIcon(Drawable icon) {
>          mIcon = icon;

You might as well do == here (and for Bitmaps, too) -- very often these will actually be the same instance, because everything in the system is cached.

@@ +362,5 @@
>      }
>  
>      @Override
>      public MenuItem setTitle(CharSequence title) {
> +        if (!TextUtils.equals(mTitle, title)) {

In the interests of avoiding potential horrors, perhaps punt this to a follow-up to do a range-limited comparison, always do the assignment, and just selectively call the callback.

We realistically only care if the first ~50 characters have changed, and we don't want to compare all 2,000 chars of a long title just to find out they're the same.

@@ +372,5 @@
>  
>      @Override
>      public MenuItem setTitle(int title) {
> +        CharSequence newTitle = mMenu.getResources().getString(title);
> +        if (!TextUtils.equals(mTitle, newTitle)) {

Just call into setTitle(CharSequence).

(If you want to be really smart, save the last `int`, invalidate that on setTitle(CharSequence), and do an int == here.)

Comment 9

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

(In reply to Richard Newman [:rnewman] from comment #8)

> >      public MenuItem setIcon(Drawable icon) {
> >          mIcon = icon;
> 
> You might as well do == here (and for Bitmaps, too) -- very often these will
> actually be the same instance, because everything in the system is cached.

Done
 
> >      @Override
> >      public MenuItem setTitle(CharSequence title) {
> > +        if (!TextUtils.equals(mTitle, title)) {
> 
> In the interests of avoiding potential horrors, perhaps punt this to a
> follow-up to do a range-limited comparison, always do the assignment, and
> just selectively call the callback.
> 
> We realistically only care if the first ~50 characters have changed, and we
> don't want to compare all 2,000 chars of a long title just to find out
> they're the same.

I left this in for now. I see what you mean about edge case string lengths, but I don't think the comparison would be the performance killer in that case. We can revert this if we see problems.

> >      public MenuItem setTitle(int title) {
> > +        CharSequence newTitle = mMenu.getResources().getString(title);
> > +        if (!TextUtils.equals(mTitle, newTitle)) {
> 
> Just call into setTitle(CharSequence).

Done

Comment 10

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

remote:   https://hg.mozilla.org/integration/fx-team/rev/73b35d5c9916
remote:   https://hg.mozilla.org/integration/fx-team/rev/4a7b2b24738a

Comment 11

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

Attachment: textutils-equals v0.1

Fixes for a failing test

Comment 12

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

Comment on attachment 8543000 [details] [diff] [review]
textutils-equals v0.1

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

Thumbs up!

Comment 13

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

Fix for testAppMenuPathways 
https://hg.mozilla.org/integration/fx-team/rev/d3c44062ebbb

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/73b35d5c9916
https://hg.mozilla.org/mozilla-central/rev/4a7b2b24738a

Comment 15

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

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