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

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P3
normal
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
Firefox 63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [qf:p1:f64][fxperf:p1])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

10 months ago
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.
Whiteboard: [qf] → [qf][fxperf]
(Assignee)

Updated

10 months ago
Assignee: nobody → ehsan
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
Attachment #8990784 - Flags: review?(mconley)

Updated

10 months ago
Blocks: lazytabs
Priority: -- → P3
(Assignee)

Updated

10 months ago
Attachment #8990784 - Attachment is obsolete: true
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
Attachment #8991086 - Flags: review?(dao+bmo) → review+
Whiteboard: [qf][fxperf] → [qf:p1:f64][fxperf:p1]
(Assignee)

Updated

9 months ago
Attachment #8991086 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Keywords: checkin-needed

Comment 8

9 months ago
Pushed by apavel@mozilla.com:
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
Keywords: checkin-needed

Comment 9

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4d3bd7a6b705
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.