Closed Bug 1164942 Opened 9 years ago Closed 9 years ago

Take pocket out of the critical path of onLocationChange

Categories

(Firefox :: Pocket, defect, P3)

38 Branch
defect
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 41
Iteration:
41.1 - May 25
Tracking Status
firefox39 --- fixed
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(1 file, 1 obsolete file)

      No description provided.
Attached file MozReview Request: bz://1164942/Gijs (obsolete) —
/r/8783 - Bug 1164942 - remove pocket from onLocationChange if not logged in, r?jaws

Pull down this commit:

hg pull -r f503a17940da7f8b55453fc37d714f56ad5e737f https://reviewboard-hg.mozilla.org/gecko/
Attachment #8606025 - Flags: review?(jaws)
need to recheck if this is a win because this is not trivially equivalent to the earlier patch (because now we need to do the work to ensure we don't regress behaviour):

baseline:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=8dfd513d452d

with patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6f7dfc33967
Comment on attachment 8606025 [details]
MozReview Request: bz://1164942/Gijs

https://reviewboard.mozilla.org/r/8781/#review7489

::: browser/base/content/browser.js:7755
(Diff revision 1)
> +        let cookie = subject.QueryInterface(Ci.nsICookie2);

I think we should consult with someone who knows the cookie code better to make sure that this observer is not gonna kill our perf.

What if we changed onLocationChange to leave the checking for the cookie to the end? Like so:

  onLocationChange(browser, locationURI) {
    if (!locationURI) {
      return;
    }
    let widgetGroupWrapper = CustomizableUI.getWidget("pocket-button");
    let win = browser.ownerDocument.defaultView;
    let widget = widgetGroupWrapper.forWindow(win);
    if (!node)
      return;
    let enabled = locationURI.schemeIs("http") ||
                  locationURI.schemeIs("https") ||
                  locationURI.spec.startsWith("about:reader?url=") ||
                  !win.pktApi.isUserLoggedIn();
    node.disabled = !enabled;
  },

This will allow early exits from before going in to the assumed-expensive isUserLogged() function. The previous implementation required checking all conditions since it used && whereas this uses ||.
Drive by: Jared, just a thought: If doing this enabling/disabling is causing a performance issue, we could just remove that functionality all together and leave the button always enabled. 

The code already supports showing an error message if the user tries to save a non-savable page. (You can see if if you leave the button enabled and click it on a page like about:home)

That way nothing has to happen on location changes, it is all self contained when the user clicks the button.
(In reply to Nate Weiner from comment #4)
> Drive by: Jared, just a thought: If doing this enabling/disabling is causing
> a performance issue, we could just remove that functionality all together
> and leave the button always enabled. 
> 
> The code already supports showing an error message if the user tries to save
> a non-savable page. (You can see if if you leave the button enabled and
> click it on a page like about:home)
> 
> That way nothing has to happen on location changes, it is all self contained
> when the user clicks the button.

This simplifies things a lot, and considering all this is really about is file/about URIs which most people won't see a lot of, I think it makes sense.

Philipp, can you confirm this is OK? Or, alternatively, can we also disable the button when the user is not logged in to pocket, so we can do away with the cookie service + customizableui check on every navigation/tabswitch?

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
>   onLocationChange(browser, locationURI) {
>     if (!locationURI) {
>       return;
>     }
>     let widgetGroupWrapper = CustomizableUI.getWidget("pocket-button");
>     let win = browser.ownerDocument.defaultView;
>     let widget = widgetGroupWrapper.forWindow(win);
>     if (!node)
>       return;
>     let enabled = locationURI.schemeIs("http") ||
>                   locationURI.schemeIs("https") ||
>                   locationURI.spec.startsWith("about:reader?url=") ||
>                   !win.pktApi.isUserLoggedIn();
>     node.disabled = !enabled;
>   },
> 
> This will allow early exits from before going in to the assumed-expensive
> isUserLogged() function. The previous implementation required checking all
> conditions since it used && whereas this uses ||.

I mean, the previous example would have bailed out if any of the negated conditions was false, too... but the order does matter, so moving it to the end makes sense.

Unfortunately, I'm 99% sure (but haven't verified) that on startup onLocationChange is initially fired with about:blank or about:home, and then with the http URL that the talos test puts in, and so this would still cause hits, though not as many, I guess. :-(
Flags: needinfo?(philipp)
Attachment #8606025 - Attachment is obsolete: true
Comment on attachment 8606025 [details]
MozReview Request: bz://1164942/Gijs

/r/8783 - Bug 1164942 - do not load pocket's jsm or other scripts until used, r?jaws

Pull down this commit:

hg pull -r c7466ef7c7a0b8c5ef6bc187b56b18925f917747 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8606025 - Attachment is obsolete: false
Attachment #8606025 - Flags: review?(jaws)
(I removed the panelUI lazy getter because there's already one in browser.js and they get loaded in the same scope)
Assuming that this does improve performance and the error message looks acceptable (I wasn't able to trigger it. do you have a screen shot?), I'm OK with leaving it enabled.

Disabling the button when the user is signed out will not work though, because then the button becomes the onboarding mechanism.
Flags: needinfo?(philipp)
(In reply to Philipp Sackl [:phlsa] please use needinfo from comment #8)
> Assuming that this does improve performance and the error message looks
> acceptable (I wasn't able to trigger it. do you have a screen shot?), I'm OK
> with leaving it enabled.

http://imgur.com/lAqIVmy

> Disabling the button when the user is signed out will not work though,
> because then the button becomes the onboarding mechanism.

I meant only in the same cases where we disable it for logged-in users: non-http/https/aboutreader pages. Anyway, it sounds like you're OK with the first option which is also simplest to implement and most performant.
Flags: needinfo?(philipp)
We shouldn't disable it on those pages for logged-out users because we want people to be able to sign up if they are on about:home or about:newtab for example.
Flags: needinfo?(philipp)
Comment on attachment 8606025 [details]
MozReview Request: bz://1164942/Gijs

https://reviewboard.mozilla.org/r/8781/#review7527

Ship It!

::: browser/components/customizableui/CustomizableWidgets.jsm:1109
(Diff revision 2)
> -      onViewShowing: Pocket.onPanelViewShowing,
> +      // Use forwarding functions here to avoid loading Pocket.jsm on startup:

This is clever, I like it :)
Attachment #8606025 - Flags: review?(jaws) → review+
Priority: -- → P3
remote:   https://hg.mozilla.org/integration/fx-team/rev/12357ab6e997
Status: NEW → ASSIGNED
Iteration: --- → 41.1 - May 25
Points: --- → 2
Flags: qe-verify-
Flags: in-testsuite-
Flags: firefox-backlog+
(In reply to Wes Kocher (:KWierso) from comment #13)
> Backed out in https://hg.mozilla.org/integration/fx-team/rev/d41f771a3927
> for b2g build orange:
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=3095433&repo=fx-team

Err, what? Did you back out the wrong thing? There is no way this change (which changes 0 c++ files, and 0 files under dom/ ) would have caused that failure.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(wkocher)
Looking at the failures, there was a green retrigger, so I've just gone ahead and relanded this.
Flags: needinfo?(wkocher)
Might have just been some taskcluster fluke or something, but your push had two of those failures, and they went away on my backout push: https://treeherder.mozilla.org/#/jobs?repo=fx-team&tochange=b40c5eb3c978&fromchange=1725e7758d6b&filter-searchStr=b2g%20emulator%20%28B%29

Lets have you reland and see what happens the second time?
Amusingly, the first thing that happened was the same as last time, "ah, hell, I'll just star 'leaked window property: pktUIMessaging' as 'leaked 2 window(s) until shutdown [url = about:blank],' close enough," "um, again?," "oops, I just did that three times on one push."

Backed out in https://hg.mozilla.org/integration/fx-team/rev/162c7e7a8a9b for https://treeherder.mozilla.org/logviewer.html#?job_id=3106361&repo=fx-team and https://treeherder.mozilla.org/logviewer.html#?job_id=3106626&repo=fx-team and https://treeherder.mozilla.org/logviewer.html#?job_id=3106383&repo=fx-team and https://treeherder.mozilla.org/logviewer.html#?job_id=3106627&repo=fx-team and https://treeherder.mozilla.org/logviewer.html#?job_id=3106124&repo=fx-team and https://treeherder.mozilla.org/logviewer.html#?job_id=3106628&repo=fx-team - at least on Windows opt, you're pretty much going to leak pktUIMessaging in browser_UITour_pocket.js
Also in 10.6 and 10.10 opt, but not apparently in either Mac debug nor in the Windows debugs which have finished so far.
Depends on: 1165890
I could repro this locally, so I've relanded with a fix. Also filed bug 1165890 to avoid misstarring this kind of thing in future.
https://hg.mozilla.org/mozilla-central/rev/8d96b303d33a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Comment on attachment 8606025 [details]
MozReview Request: bz://1164942/Gijs

Approval Request Comment
[Feature/regressing bug #]: pocket performance
[User impact if declined]: even bigger ts_paint/tpaint hit than with this patch
[Describe test coverage new/current, TreeHerder]: there is some pocket testing
[Risks and why]: reasonably low - OK for the early beta that we're in here
[String/UUID change made/needed]: no
Attachment #8606025 - Flags: approval-mozilla-beta?
Attachment #8606025 - Flags: approval-mozilla-aurora?
Comment on attachment 8606025 [details]
MozReview Request: bz://1164942/Gijs

Approved for uplift to aurora and beta to improve Pocket performance. Looks a bit risky from the discussion, but we can take fixes in early beta.
Attachment #8606025 - Flags: approval-mozilla-beta?
Attachment #8606025 - Flags: approval-mozilla-beta+
Attachment #8606025 - Flags: approval-mozilla-aurora?
Attachment #8606025 - Flags: approval-mozilla-aurora+
Attachment #8606025 - Attachment is obsolete: true
Attachment #8620298 - Flags: review+
You need to log in before you can comment on or make changes to this bug.