963288 - Remove unnecessary synchronization on Tabs#isOpen

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

Bug 729463 added the Tabs#isOpen method and made it synchronized; however, this method does not write to mSelectedTab and it does not use mTabs.

It would be nice if we could figure out a better way to contain the synchronization logic, or at least a way to make the requirements more explicit. I think it's unreasonable to expect that anyone who touches Tabs will completely familiarize themselves with the complex synchronization rules in that file. Good FE discussion topic!

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Remove unnecessary synchronization in Tabs

I'm fairly confident that getTabForUrl (from bug 914296) also does not need to be synchronized, so I fixed that as well.

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8364657 [details] [diff] [review]
Remove unnecessary synchronization in Tabs

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

r+ with these changes.

::: mobile/android/base/Tabs.java
@@ +667,5 @@
>      public int getTabIdForUrl(String url) {
>          return getTabIdForUrl(url, Tabs.getInstance().getSelectedTab().isPrivate());
>      }
>  
> +    public Tab getTabForUrl(String url) {

While you're here, please also rename this to `getFirstTabForURL` and `getTabIdForUrl` to `getFirstTabIDForURL`. Best to reinforce the notion that there's a many-to-one mapping.

@@ +672,2 @@
>          int tabId = getTabIdForUrl(url);
>          return getTab(tabId);

This method was using synchronization because of this: we iterate over the open tabs once to get the ID, then again to get the tab. COWAL only buys us consistency within a single iteration.

The right answer is to kill the synchronization and invert gTFU and gTIFU:

    public Tab getFirstTabForURL(String url, boolean isPrivate) {
        for (Tab tab : mOrder) {
            String tabUrl = tab.getURL();
            if (AboutPages.isAboutReader(tabUrl)) {
                tabUrl = ReaderModeUtils.getUrlFromAboutReader(tabUrl);
            }
            if (TextUtils.equals(tabUrl, url) && isPrivate == tab.isPrivate()) {
                return tab;
            }
        }

        return null;
    }

    public int getFirstTabIDForURL(String url, boolean isPrivate) {
        final Tab tab = getFirstTabForURL(url, isPrivate);
        if (tab == null) {
            return -1;
        }
        return tab.getId();
    }

Comment 3

[Brian Nicholson (:bnicholson)]

(In reply to Richard Newman [:rnewman] from comment #2)
> This method was using synchronization because of this: we iterate over the
> open tabs once to get the ID, then again to get the tab. COWAL only buys us
> consistency within a single iteration.

I did notice this, but I came to the conclusion that it didn't matter. Say we do remove a tab between getTabIdForUrl and getTab -- getTab will then return null because the tab is no longer in the list. That's the same result as if getTabForUrl is synchronized and the remove operation had occurred a tick earlier, no? The only difference being we don't do the extra work calling getTabIdForUrl(url), but that's at the expense of being synchronized.

Comment 4

[Richard Newman [:rnewman]]

(In reply to Brian Nicholson (:bnicholson) from comment #3)

> I did notice this, but I came to the conclusion that it didn't matter.

Don't forget that HashMap isn't thread-safe.

---
Note that this implementation is not synchronized. If multiple threads access a hash map concurrently, and at least one of the threads modifies the map structurally, it must be synchronized externally.
---

Don't fall into the trap of thinking that reads don't need to be synchronized!

The inversion I suggested will not only preserve consistency between mTabs and mOrder (even if it's not strictly necessary), but also avoid the need to touch mTabs, which avoids the need to synchronize.

And as you observed, it also saves a lookup (in a SparseArray, now that Lucas just landed his patch from this morning). That lookup ain't free.

Win-win!

Comment 5

[Brian Nicholson (:bnicholson)]

(In reply to Richard Newman [:rnewman] from comment #4)
> (In reply to Brian Nicholson (:bnicholson) from comment #3)
> 
> > I did notice this, but I came to the conclusion that it didn't matter.
> 
> Don't forget that HashMap isn't thread-safe.
> 
> ---
> Note that this implementation is not synchronized. If multiple threads
> access a hash map concurrently, and at least one of the threads modifies the
> map structurally, it must be synchronized externally.
> ---
> 
> Don't fall into the trap of thinking that reads don't need to be
> synchronized!

Not sure what HashMap you're referring to here -- the mTabs reference in getTab? getTab is synchronized.

Comment 6

[Richard Newman [:rnewman]]

(In reply to Brian Nicholson (:bnicholson) from comment #5)

> Not sure what HashMap you're referring to here -- the mTabs reference in
> getTab? getTab is synchronized.

Ah, the version of this method that was in my head was accessing mTabs directly. (Oh, my fallible brain.)

You're right, removing the synchronization here would slightly reduce the critical section (it would still take the lock for getTab) at the cost of a probably irrelevant loss of consistency.

Academic, though: inverting those methods makes this moot!

Comment 7

[Brian Nicholson (:bnicholson)]

Attachment: Remove unnecessary synchronization in Tabs, v2

Glad we're on the same page now :)

I agree that the getter should return a Tab, not an ID. While making that change, though, I found another bug: getTabForUrl returns a tab only if its state matches the state of the selected tab. That method is called only at [1], so AFAICT, there's no need for selected state check.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/favicons/Favicons.java#242

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8366078 [details] [diff] [review]
Remove unnecessary synchronization in Tabs, v2

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

::: mobile/android/base/BrowserApp.java
@@ +1369,5 @@
>  
>          // Set the target tab to null so it does not get selected (on editing
>          // mode exit) in lieu of the tab we are about to select.
>          mTargetTabForEditingMode = null;
> +        Tabs.getInstance().selectTab(tab.getId());

Just use `tabs` here instead of getting the instance again.

::: mobile/android/base/Tabs.java
@@ +671,5 @@
>                  tabUrl = ReaderModeUtils.getUrlFromAboutReader(tabUrl);
>              }
> +            if (TextUtils.equals(tabUrl, url) &&
> +                    (isPrivate == null || isPrivate == tab.isPrivate())) {
> +                return tab;

I'd rephrase this: put the condition before the URL check.

  if (url == null) {
      return null;
  }
  for (Tab tab : mOrder) {
      if (isPrivate != null && isPrivate != tab.isPrivate()) {
          continue;
      }
      String tabURL = tab.getURL();
      if (AboutPages.isAboutReader(tabUrl)) {
          tabUrl = ReaderModeUtils.getUrlFromAboutReader(tabUrl);
      }
      if (url.equals(tabUrl)) {
          return tab;
      }
  }


But I'd really like us -- one day -- to make progress on using two Tabs instances, one for private and one for not, and get rid of this nastiness altogether.

Comment 9

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/fx-team/rev/24979a717e7d

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/24979a717e7d