Cleanup GeckoMenu and GeckoMenuItem

RESOLVED FIXED in Firefox 37

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mfinkle, Assigned: mfinkle)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 37
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

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.
Created attachment 8542736 [details] [diff] [review]
notify-actual-menuchange v0.1

Only call onItemChanged if the property really changed. I skipped the check for comparing Bitmaps.
Assignee: nobody → mark.finkle
Attachment #8542736 - Flags: review?(rnewman)
Created attachment 8542737 [details] [diff] [review]
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.
Attachment #8542737 - Flags: review?(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();
Attachment #8542737 - Flags: review?(rnewman) → review+
Status: NEW → ASSIGNED
OS: Mac OS X → Android
Hardware: x86 → All
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.
(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.
(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?
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f4e2c4c7474
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.)
Attachment #8542736 - Flags: review?(rnewman) → review+
(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
remote:   https://hg.mozilla.org/integration/fx-team/rev/73b35d5c9916
remote:   https://hg.mozilla.org/integration/fx-team/rev/4a7b2b24738a
Created attachment 8543000 [details] [diff] [review]
textutils-equals v0.1

Fixes for a failing test
Attachment #8543000 - Flags: review?(michael.l.comella)
Comment on attachment 8543000 [details] [diff] [review]
textutils-equals v0.1

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

Thumbs up!
Attachment #8543000 - Flags: review?(michael.l.comella) → review+
Fix for testAppMenuPathways 
https://hg.mozilla.org/integration/fx-team/rev/d3c44062ebbb
https://hg.mozilla.org/mozilla-central/rev/73b35d5c9916
https://hg.mozilla.org/mozilla-central/rev/4a7b2b24738a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
https://hg.mozilla.org/mozilla-central/rev/d3c44062ebbb

Updated

3 years ago
Depends on: 1117746
You need to log in before you can comment on or make changes to this bug.