1169419 - Reduce number of profile lookups during use

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Profile Handling, defect)

Description

[Richard Newman [:rnewman]]

This is a follow-up from Bug 1116668.

The first comment in that bug shows a number of places where we call into GeckoProfile.get from around the app.

Repeated calls are unnecessary, because the profile doesn't change during the lifecycle of the app. (And if it were to do so, these calling components would be buggy!)

More importantly, in many situations *any* calls are unnecessary, because the constructing or configuring caller already knows the profile.

For example, Tabs uses the profile in a bunch of places (to register observers, to refresh thumbnails, etc.).

There's a single Tabs instance, but it's attached to a context in GeckoApp.onCreate... mere lines below where we just looked up the profile!

There's a broader issue here: we have lots of singletons with implicit dependencies on other instances, which makes testing harder, order of initialization control harder, etc. etc.

But even without fully addressing that -- really rethinking the relationships between the app, its activities, the profile, and various objects like Tabs -- we can do better.

This bug is to:

* Examine places in the app that routinely call into GeckoProfile.get(), and decide if it's appropriate for them to keep a reference to the returned GeckoProfile.

* Decide whether their lifecycle is appropriate to take a profile as a constructor or init argument.

This should help with Bug 1145192 and Bug 1130550, might give us some small perf wins, and will also reduce the complexity of our runtime component dependencies.

Comment 1

[Nick Alexander :nalexander [he/him]]

All I can say is hooray.  I'll act as a third mentor, if one is needed.

Comment 2

[Dipti Nirmale [:dnirm]]

Attachment: Tabs: Keep reference to the profile taken as init argument.

Starting with Tabs- we can keep reference to the profile which is taken from GeckoApp during first call to the Tabs instance. The profile is further passed to PersistTabsRunnable and Tab.
This helps to avoid calls to GeckoProfile.get() inside Tabs.

Comment 3

[Dipti Nirmale [:dnirm]]

Attachment: BrowserApp: Reduce profile lokups to only once by keeping reference to the returned profile

The single call to GeckoProfile.get() can be avoided by calling super.onCreate() before everything else and setting mProfile = super.mProfile in BrowserApp.onCreate().
I'm not sure about the reason for this comment in BrowserApp.onCreate- "Note that we're calling GeckoProfile.get *before GeckoApp.onCreate*. This means we're reliant on the logic in GeckoProfile to correctly look up our launch intent (via BrowserApp's Activity-ness) and pull out the arguments. Be careful if you change that!"

Comment 4

[Dipti Nirmale [:dnirm]]

I found that GeckoProfile.get is often called by ActivityChooserModel, SuggestedSites and GeckoSharedPrefs. 
If we keep any reference to returned profile in GeckoSharedPrefs.forProfile(), it has to be static. Should there be cached values for SharedPreferences? Or they should have freshly computed values each time?

Comment 5

[Dipti Nirmale [:dnirm]]

Attachment: ActivityChooserModel: Reduce profile lookups to once by keeping reference to the returned profile

Passing profile as argument to ActivityChooserModel.get might need one or more calls to GeckoProfile.get().

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8614401 [details] [diff] [review]
Tabs: Keep reference to the profile taken as init argument.

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

::: mobile/android/base/Tab.java
@@ +112,2 @@
>          mAppContext = context.getApplicationContext();
> +        mDB = profile.getDB();

Just pass the DB as an argument instead of the profile.

::: mobile/android/base/Tabs.java
@@ +76,5 @@
>          private final BrowserDB db;
>          private final Context context;
>          private final Iterable<Tab> tabs;
>  
> +        public PersistTabsRunnable(final Context context, GeckoProfile profile, Iterable<Tab> tabsInOrder) {

DB as arg.

@@ +122,2 @@
>          final Context appContext = context.getApplicationContext();
> +        mProfile = profile;

Might as well store just the DB, right? We don't use the profile for anything else.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8614405 [details] [diff] [review]
BrowserApp: Reduce profile lokups to only once by keeping reference to the returned profile

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

I believe this is safe if we use GeckoApp's mProfile member, but I want a second opinion. Martyn?

::: mobile/android/base/BrowserApp.java
@@ +193,5 @@
>      private ActionModeCompat mActionMode;
>      private boolean mHideDynamicToolbarOnActionModeEnd;
>      private TabHistoryController tabHistoryController;
>      private ZoomedView mZoomedView;
> +    private GeckoProfile mProfile;

GeckoApp already has an mProfile member, so don't do this.

@@ +667,5 @@
>      @Override
>      public void onCreate(Bundle savedInstanceState) {
>          final Intent intent = getIntent();
>  
> +

No need for this empty line.

@@ +674,5 @@
>          // look up our launch intent (via BrowserApp's Activity-ness) and pull
>          // out the arguments. Be careful if you change that!
> +        mProfile = GeckoProfile.get(this);
> +
> +        if (mProfile != null && !mProfile.inGuestMode()) {

I believe that it's no longer possible for GeckoProfile.get to return null. Please double check that and clean up this conditional accordingly.

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8614438 [details] [diff] [review]
ActivityChooserModel: Reduce profile lookups to once by keeping reference to the returned profile

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

Let's not do this work at all: have ActivityChooserModel.get, and the constructor, take the history file as an argument.

In other words: rather than getting or being given the profile and a filename, using the two to get a file -- just give it the File!

::: mobile/android/base/widget/ActivityChooserModel.java
@@ +1208,5 @@
>              FileOutputStream fos = null;
>  
>              try {
>                  // Mozilla - Update the location we save files to
> +                File file = mProfile.getFile(historyFileName);

This would instead be provided as an argument in persistHistoricalDataIfNeeded, and the value would be mHistoryFile, which we'd save in the constructor.

Comment 9

[Martyn Haigh (:mhaigh)]

> I believe this is safe if we use GeckoApp's mProfile member, but I want a
> second opinion. Martyn?

I'd prefer to make the mProfile member private and use the getProfile method as BrowserApp doesn't need direct access to the variable; although not necessary I'd be tempted to clean this up a bit given that you're touching this code anyway.

(http://programmers.stackexchange.com/questions/162643/why-is-clean-code-suggesting-avoiding-protected-variables)

Comment 10

[Dipti Nirmale [:dnirm]]

> I'd prefer to make the mProfile member private and use the getProfile method
> as BrowserApp doesn't need direct access to the variable; although not
> necessary I'd be tempted to clean this up a bit given that you're touching
> this code anyway.

This sounds reasonable but GeckoApp.mProfile is accessed and assigned value in GeckoProfile.get

        if (GuestSession.shouldUse(context, args)) {
            final GeckoProfile p = GeckoProfile.getOrCreateGuestProfile(context);
            if (isGeckoApp) {
                ((GeckoApp) context).mProfile = p;
            }
            return p;
        }

        final GeckoProfile fromArgs = GeckoProfile.getFromArgs(context, args);
        if (fromArgs != null) {
            if (isGeckoApp) {
                ((GeckoApp) context).mProfile = fromArgs;
            }
            return fromArgs;
        }

Also, why do we need it here if GeckoApp.getProfile() can take care of the returned profile? Should I then remove this part too after making mProfile private?

Comment 11

[Richard Newman [:rnewman]]

GeckoApp is used as a cache so that GeckoProfile doesn't always need to consult intent args. See my comment in GeckoProfile that we shouldn't do that :)

Believe it or not, our profile handling is way less confusing than it used to be. It's still far from perfect. But this stuff is easy to get wrong, so I'm a big fan of small changes and follow-up bugs.

Comment 12

[Dipti Nirmale [:dnirm]]

Attachment: Tabs: Keep reference to the browser db taken as argument

Comment 13

[Dipti Nirmale [:dnirm]]

Attachment: BrowserApp: Use profile from GeckoApp

Using GeckoApp.getProfile() instead of GeckoApp.mProfile for consistency and in case mProfile is made private.
As Richard suggested, leaving mProfile modifier change to follow-up bug.

Comment 14

[Dipti Nirmale [:dnirm]]

(In reply to Richard Newman [:rnewman] from comment #8)
> Let's not do this work at all: have ActivityChooserModel.get, and the
> constructor, take the history file as an argument.

What should happen in case the file passed as argument is null? Should ActivityChooserModel create a new file as default (which may cause redundancy) and add this data model to the registry?

Comment 15

[Richard Newman [:rnewman]]

(In reply to Dipti Nirmale [:dnirm] from comment #14)

> What should happen in case the file passed as argument is null? Should
> ActivityChooserModel create a new file as default (which may cause
> redundancy) and add this data model to the registry?

The current behavior is that if the filename is null or empty, it doesn't persist or read -- it becomes an in-memory model only.

We can preserve that behavior when we pass in a file, changing those instances of !TextUtils.isEmpty to != null.

The only real change is that the caller has to be careful to use the correct file extension, but IMO the current flexibility is unwarranted.

Comment 16

[Richard Newman [:rnewman]]

Comment on attachment 8615773 [details] [diff] [review]
Tabs: Keep reference to the browser db taken as argument

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

Looks good with these two changes.

::: mobile/android/base/Tabs.java
@@ +72,5 @@
>      private static final AtomicInteger sTabId = new AtomicInteger(0);
>      private volatile boolean mInitialTabsAdded;
>  
>      private Context mAppContext;
> +    private BrowserDB mDb;

mDB

@@ +592,5 @@
>              @Override
>              public void run() {
>                  for (final Tab tab : mOrder) {
>                      if (tab.getThumbnail() == null) {
> +                        tab.loadThumbnailFromDB(mDb);

Capture it outside, just as we did before. Doing it this way will incur the creation of a package-scoped wrapper method to access the private member of the enclosing class.

Comment 17

[Richard Newman [:rnewman]]

Comment on attachment 8616215 [details] [diff] [review]
BrowserApp: Use profile from GeckoApp

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

I think this is correct. Only one way to find out!

::: mobile/android/base/BrowserApp.java
@@ +669,5 @@
>      @Override
>      public void onCreate(Bundle savedInstanceState) {
>          final Intent intent = getIntent();
>  
> +        if (!getProfile().inGuestMode()) {

I think the comment can stay; it still adds a useful note of caution.

Comment 18

[Dipti Nirmale [:dnirm]]

Comment on attachment 8616215 [details] [diff] [review]
BrowserApp: Use profile from GeckoApp

># HG changeset patch
># User Dipti Nirmale <[e-mail]>
># Parent  ff33a9feceb8fa6d1de71aa2901a230babadce53
>Bug 1169419 - BrowserApp: Use profile from GeckoApp. r=rnewman
>
>diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java
>--- a/mobile/android/base/BrowserApp.java
>+++ b/mobile/android/base/BrowserApp.java
>@@ -665,23 +665,17 @@ public class BrowserApp extends GeckoApp
>         }
>         return super.onKeyUp(keyCode, event);
>     }
> 
>     @Override
>     public void onCreate(Bundle savedInstanceState) {
>         final Intent intent = getIntent();
> 
>        // Note that we're calling GeckoProfile.get *before GeckoApp.onCreate*.
>        // This means we're reliant on the logic in GeckoProfile to correctly
>        // look up our launch intent (via BrowserApp's Activity-ness) and pull
>        // out the arguments. Be careful if you change that!
>-        final GeckoProfile p = GeckoProfile.get(this);
>-
>-        if (p != null && !p.inGuestMode()) {
>+        if (!getProfile().inGuestMode()) {
>             // This is *only* valid because we never want to use the guest mode
>             // profile concurrently with a normal profile -- no syncing to it,
>             // no dual-profile usage, nothing. BrowserApp startup with a conventional
>             // GeckoProfile will cause the guest profile to be deleted.
>             GeckoProfile.maybeCleanupGuestProfile(this);
>         }
> 
>         // This has to be prepared prior to calling GeckoApp.onCreate, because
>@@ -936,17 +930,17 @@ public class BrowserApp extends GeckoApp
>     protected void openQueuedTabs() {
>         ThreadUtils.assertNotOnUiThread();
> 
>         int queuedTabCount = TabQueueHelper.getTabQueueLength(BrowserApp.this);
> 
>         Telemetry.addToHistogram("FENNEC_TABQUEUE_QUEUESIZE", queuedTabCount);
>         Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.INTENT, "tabqueue-delayed");
> 
>-        TabQueueHelper.openQueuedUrls(BrowserApp.this, mProfile, TabQueueHelper.FILE_NAME, false);
>+        TabQueueHelper.openQueuedUrls(BrowserApp.this, getProfile(), TabQueueHelper.FILE_NAME, false);
> 
>         // If there's more than one tab then also show the tabs panel.
>         if (queuedTabCount > 1) {
>             ThreadUtils.postToUiThread(new Runnable() {
>                 @Override
>                 public void run() {
>                     showNormalTabs();
>                 }
>@@ -958,17 +952,17 @@ public class BrowserApp extends GeckoApp
>     public void onResume() {
>         super.onResume();
> 
>         final String args = ContextUtils.getStringExtra(getIntent(), "args");
>         // If an external intent tries to start Fennec in guest mode, and it's not already
>         // in guest mode, this will change modes before opening the url.
>         // NOTE: OnResume is called twice sometimes when showing on the lock screen.
>         final boolean enableGuestSession = GuestSession.shouldUse(this, args);
>-        final boolean inGuestSession = GeckoProfile.get(this).inGuestMode();
>+        final boolean inGuestSession = getProfile().inGuestMode();
>         if (enableGuestSession != inGuestSession) {
>             doRestart(getIntent());
>             return;
>         }
> 
>         EventDispatcher.getInstance().unregisterGeckoThreadListener((GeckoEventListener)this,
>             "Prompt:ShowTop");
> 
>@@ -3062,17 +3056,17 @@ public class BrowserApp extends GeckoApp
>             MenuUtils.safeSetEnabled(aMenu, R.id.subscribe, false);
>             MenuUtils.safeSetEnabled(aMenu, R.id.add_search_engine, false);
>             MenuUtils.safeSetEnabled(aMenu, R.id.site_settings, false);
>             MenuUtils.safeSetEnabled(aMenu, R.id.add_to_launcher, false);
> 
>             return true;
>         }
> 
>-        final boolean inGuestMode = GeckoProfile.get(this).inGuestMode();
>+        final boolean inGuestMode = getProfile().inGuestMode();
> 
>         final boolean isAboutReader = AboutPages.isAboutReader(tab.getURL());
>         bookmark.setEnabled(!isAboutReader);
>         bookmark.setVisible(!inGuestMode);
>         bookmark.setCheckable(true);
>         bookmark.setChecked(tab.isBookmark());
>         bookmark.setIcon(resolveBookmarkIconID(tab.isBookmark()));
>         bookmark.setTitle(resolveBookmarkTitleID(tab.isBookmark()));
>@@ -3208,17 +3202,17 @@ public class BrowserApp extends GeckoApp
>                                tab.getContentType().equals("application/vnd.mozilla.xul+xml") ||
>                                tab.getContentType().startsWith("video/")));
> 
>         // Disable find in page for about:home, since it won't work on Java content.
>         findInPage.setEnabled(!isAboutHome(tab));
> 
>         charEncoding.setVisible(GeckoPreferences.getCharEncodingState());
> 
>-        if (mProfile.inGuestMode()) {
>+        if (getProfile().inGuestMode()) {
>             exitGuestMode.setVisible(true);
>         } else {
>             enterGuestMode.setVisible(true);
>         }
> 
>         return true;
>     }
>

Comment 19

[Dipti Nirmale [:dnirm]]

Attachment: BrowserApp: Use profile from GeckoApp.

Brought back the comment which was deleted in last patch.

Comment 20

[Dipti Nirmale [:dnirm]]

Attachment: Tabs: Keep reference to the browser db taken as argument

Changes made from Review comments.

Comment 21

[Dipti Nirmale [:dnirm]]

Comment on attachment 8615773 [details] [diff] [review]
Tabs: Keep reference to the browser db taken as argument

 Changes added to new patch - 8624007

Comment 22

[Dipti Nirmale [:dnirm]]

Thank you for the reviews.

(In reply to Richard Newman [:rnewman] from comment #15)
> The current behavior is that if the filename is null or empty, it doesn't
> persist or read -- it becomes an in-memory model only.

            ActivityChooserModel dataModel = sDataModelRegistry.get(historyFileName);
            if (dataModel == null) {
                dataModel = new ActivityChooserModel(context, historyFileName);
                sDataModelRegistry.put(historyFileName, dataModel);

Should it create in-memory model when the file reference is null? Here it looks for date model with it's file name. I think in case of null reference, it should create model with DEFAULT_HISTORY_FILE_NAME and create new file object. Could you please tell me if this makes sense?

Comment 23

[Richard Newman [:rnewman]]

(In reply to Dipti Nirmale [:dnirm] from comment #22)

> Should it create in-memory model when the file reference is null? Here it
> looks for date model with it's file name.

The only change is that everywhere we used to handle a filename, we now handle a File. Same behavior, just the caller is responsible for making the File instance.

(Sorry for the delayed reply!)

Comment 24

[Dipti Nirmale [:dnirm]]

Attachment: ActivityChooserModel: Get history file as argument

Comment 25

[Dipti Nirmale [:dnirm]]

Attachment: Tabs Test: Pass profile db as argument to tab consturctor

Comment 26

[Dipti Nirmale [:dnirm]]

Attachment: BrowserDatabaseHelper: Reduce number of profile lookups during use

Comment 27

[Dipti Nirmale [:dnirm]]

Attachment: Reduce number of profile lookups in org.mozilla.gecko.home package

Comment 28

[Dipti Nirmale [:dnirm]]

Attachment: Reduce number of profile lookups in org.mozilla.gecko package

Comment 29

[Richard Newman [:rnewman]]

Comment on attachment 8627486 [details] [diff] [review]
Reduce number of profile lookups in org.mozilla.gecko.home package

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

::: mobile/android/base/home/HomeFragment.java
@@ +73,5 @@
>  
>      // Helper for opening a tab in the background.
>      private OnUrlOpenInBackgroundListener mUrlOpenInBackgroundListener;
>  
> +    // Gecko profile

No need for the comment.

@@ +80,5 @@
>      @Override
>      public void onAttach(Activity activity) {
>          super.onAttach(activity);
>  
> +        mProfile = GeckoProfile.get(activity);

Set this to null in onDetach.

::: mobile/android/base/home/TopSitesPanel.java
@@ +81,5 @@
>  
> +    // Gecko profile
> +    private GeckoProfile mProfile;
> +
> +    // Profile DB

No need for these two comments.

@@ +302,5 @@
>          return snapshot;
>      }
>  
>      @Override
>      public void onDestroyView() {

Don't forget to clear the fields in onDestroyView.

Comment 30

[Richard Newman [:rnewman]]

(Sorry for the delay in reviewing these; it's been a busy couple of weeks.)

Comment 31

[Dipti Nirmale [:dnirm]]

Thanks Richard. I'll update this patch with correction.

Comment 32

[Dipti Nirmale [:dnirm]]

Attachment: Reduce number of profile lookups in org.mozilla.gecko.home package

Added changes from feedback comments in last patch

Comment 33

[Dipti Nirmale [:dnirm]]

Comment on attachment 8623972 [details] [diff] [review]
BrowserApp: Use profile from GeckoApp.

carrying forward r+

Comment 34

[Dipti Nirmale [:dnirm]]

Comment on attachment 8624007 [details] [diff] [review]
Tabs: Keep reference to the browser db taken as argument

carrying forward r+

Comment 35

[Dipti Nirmale [:dnirm]]

Hi Richard, could you please review the bug? Please let me know if any changes are needed.

Comment 36

[B. Dahse]

Hi. What happened to this bug? Seems like Dipti did a good job and almost finished it. If the patches are still relevant, I imagine they have bitrotted by now but I could fix them up if that's desirable :)

Comment 37

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.