1478675 - A third of the time spent in browser.xul's onLoad listener is tracking protection init code eagerly accessing DOM nodes

Status: RESOLVED FIXED

(Firefox :: Site Identity, defect, P2)

Description

[Florian Quèze [:florian]]

See this profile: https://perfht.ml/2LQ1gm8

This is on a very fast macbook. I just opened a new browser window (Cmd+N), the browser.xul's onLoad handler took 19ms. 7ms of it was spent in browser-trackingprotection.js calling querySelector and so triggering some Rust code.

The affected code could (and should!) trivially be changed to use getElementById: https://searchfox.org/mozilla-central/rev/943a6cf31a96eb439db8f241ab4df25c14003bb8/browser/base/content/browser-trackingprotection.js#34-41,44

Emilio, is querySelector expected to be that slow?

Comment 1

[Emilio Cobos Álvarez (:emilio)]

That's not the querySelector code. That's this GetBindingURL, which is there so that the XBL binding for the element is setup when it's accessed from JS even though we haven't run layout:

  https://searchfox.org/mozilla-central/rev/943a6cf31a96eb439db8f241ab4df25c14003bb8/dom/base/Element.cpp#587

That can go away once we get rid of XBL, maybe sooner if consumers aren't tripped by it. But last time I tried to remove it I didn't have a useful browser so...

Comment 2

[Florian Quèze [:florian]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)
> That's not the querySelector code. That's this GetBindingURL, which is there
> so that the XBL binding for the element is setup when it's accessed from JS
> even though we haven't run layout

That's surprising to me. This is during the 'load' event, so after 'MozBeforeInitialXULLayout' and 'DOMContentLoaded' where we should have already processed styles and attached bindings, right? A we paying a significant cost here only when accessing currently hidden DOM nodes that would have a binding attached if the node was visible? If so, should this JS code be changed to have lazy getters?

Actually, we spend a lot more time in GetBindingURL during MozBeforeInitialXULLayout: https://perfht.ml/2NHvBUg

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Oh, well, yeah, makes sense if this is in a hidden subtree...

I don't know how would migrating to any other mechanism would help, we need to check if we _might_ have a binding. Migrating everything to another mechanism that allows us to remove that would be lovely though.

Comment 4

[Johann Hofmann [:johannh]]

This will be fixed by bug 1475670, I hope.

Comment 5

[Johann Hofmann [:johannh]]

We eliminated the querySelector calls so technically this is fixed, is there anything else you'd like to do here or can we close this?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

I'm not sure, I'd need to see another profile, but assuming script no longer accesses tons of hidden element subtrees or such this would be ok to close.

Comment 7

[Florian Quèze [:florian]]

In a profile on the ref hardware on the current nightly, I don't see any obvious issue in the onLoad listener: https://perfht.ml/2GUTkSE so let's close this as fixed.