1474384 - <xul:browser>.currentURI for lazy browsers is very eager to allocate nsIURI objects

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P3)

Description

[(no longer active)]

See this profile please:

https://perfht.ml/2KLp2Dp

The culprit is this line: https://searchfox.org/mozilla-central/rev/28daa2806c89684b3dfa4f0b551db1d099dda7c2/browser/base/content/tabbrowser.js#1907

Here, every time browser.currentURI is called, we create a new nsIURI object!  This is very inefficient, as this profile shows.  The majority of the time spent here is doing this work, resulting in UI jank.

The browser session being profiled here contains a large session that has been restored and hence contains a lot of lazy browsers.

Comment 1

[(no longer active)]

Attachment: Memoize the nsIURI object returned from the currentURI getter for lazy <xul:browser> objects

Comment 2

[(no longer active)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=807d98523161d522d3ae242f7dc6b511f8441cf9

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8990784 [details] [diff] [review]
Memoize the nsIURI object returned from the currentURI getter for lazy <xul:browser> objects

>--- a/browser/base/content/tabbrowser.js
>+++ b/browser/base/content/tabbrowser.js
>@@ -1899,17 +1899,23 @@ window._gBrowser = {
>           getter = () => aTab.hasAttribute("muted");
>           break;
>         case "contentTitle":
>           getter = () => SessionStore.getLazyTabValue(aTab, "title");
>           break;
>         case "currentURI":
>           getter = () => {
>             let url = SessionStore.getLazyTabValue(aTab, "url");
>-            return Services.io.newURI(url);
>+            // Avoid recreating the same nsIURI object over and over again...
>+            if ("_cachedURL" in browser &&
>+                browser._cachedURL &&
>+                browser._cachedURL.spec == url) {
>+              return browser._cachedURL;

The browser._cachedURL null-check makes the |"_cachedURL" in browser| check redundant.

What's the point of the string comparison?

As for the name, I'd prefer _cachedCurrentURI or so.

Please also delete this in _insertBrowser, e.g. after this line: https://searchfox.org/mozilla-central/rev/c579ce13ca7864c5df9711eda730ceb00501aed3/browser/base/content/tabbrowser.js#1990

Comment 4

[(no longer active)]

Attachment: Memoize the nsIURI object returned from the currentURI getter for lazy <xul:browser> objects

Comment 5

[(no longer active)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb7c2d7f7020398918850161d497000aa302f6a4

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8991086 [details] [diff] [review]
Memoize the nsIURI object returned from the currentURI getter for lazy <xul:browser> objects

>--- a/browser/base/content/tabbrowser.js
>+++ b/browser/base/content/tabbrowser.js
>@@ -1899,17 +1899,21 @@ window._gBrowser = {
>           getter = () => aTab.hasAttribute("muted");
>           break;
>         case "contentTitle":
>           getter = () => SessionStore.getLazyTabValue(aTab, "title");
>           break;
>         case "currentURI":
>           getter = () => {
>             let url = SessionStore.getLazyTabValue(aTab, "url");
>-            return Services.io.newURI(url);
>+            // Avoid recreating the same nsIURI object over and over again...
>+            if (browser._cachedURL) {
>+              return browser._cachedURL;
>+            }
>+            return browser._cachedURL = Services.io.newURI(url);

_cachedCurrentURI please

r=me with that change or a compelling reason for preferring _cachedURL as the name

Comment 7

[(no longer active)]

Attachment: Memoize the nsIURI object returned from the currentURI getter for lazy <xul:browser> objects

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d3bd7a6b705
Memoize the nsIURI object returned from the currentURI getter for lazy <xul:browser> objects; r=dao

Comment 9

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/4d3bd7a6b705