1352518 - Avoid calling getComputedStyle() in isElementVisible() in UITour.jsm

Status: NEW

(Firefox :: Tours, defect, P5)

Description

[Jet Villegas (inactive)]

Attachment: Add an early return from isElementVisible() to avoid a potentially expensive Style flush.

isElementVisible() shows up in profiles and needs optimization. One way is to avoid an expensive Style flush when an element's document is hidden anyway.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It would be much better if this *never* called getComputedStyle.

Comment 2

[:Gijs (he/him)]

Comment on attachment 8853497 [details] [diff] [review]
Add an early return from isElementVisible() to avoid a potentially expensive Style flush.

(In reply to David Baron :dbaron: ⌚️UTC-4 from comment #1)
> It would be much better if this *never* called getComputedStyle.

I concur. I happen to know reader mode works around this by using nsIDOMWindowUtils's getBoundsWithoutFlushing() method, which might work for this usecase, too? Can you try that, and assuming the uitour tests pass, request review from MattN in case I'm missing some reason that isn't workable?

Otherwise, if there are specific callpaths to this that show up in profiles, we could potentially use that knowledge to optimize those paths - it looks like some of the targets are known/fixed, and for some elements, making the determination of "is this likely to be visible" could potentially be made via other means, or even hardcoded (e.g. the url bar is always visible). Without more details about where this shows up in profiles it's hard to be more definitive about that though.

Comment 3

[Matthew N. [:MattN]]

Note that the function in question was intended to only be used a few sessions per *year* per user. I'd be very interested to know who is calling this code path and it doesn't look like it's about:home snippets (from skimming some of their code) so perhaps it's SelfSupport/Shield? I guess they are calling `getAvailableTargets`/`Mozilla.UITour.getConfiguration("availableTargets")` which is hitting this but still I would hope they would be doing that rarely enough that it wouldn't show in profiles. I would be interested in hearing the use case for them using it in the first place since I suspect it's not for the intended use.

I'm fine with optimizing the code too if there are ways that don't affect behaviour but just wanted to point out that performance shouldn't be a factor for what this API was intended for.

Michael, is shield using availableTargets from UITour at all? If so, for what and how often?

Comment 4

[Osmose [:osmose, :mkelly]]

(In reply to Matthew N. [:MattN] (behind on bugmail; PM if requests are blocking you) from comment #3)
> Michael, is shield using availableTargets from UITour at all? If so, for
> what and how often?

As far as I know, no. We only use `showHeartbeat` and `getConfiguration` for `appinfo`, `selectedSearchEngine`, and `sync`.

Comment 5

[Matthew N. [:MattN]]

Hmm… then maybe it was snippets after all? Rather than guessing, Jet, could you give STR?

Comment 6

[Jet Villegas (inactive)]

(In reply to Matthew N. [:MattN] (behind on bugmail; PM if requests are blocking you) from comment #5)
> Hmm… then maybe it was snippets after all? Rather than guessing, Jet, could
> you give STR?

I came across this while watching Ehsan's demo of the Gecko profiler. IIRC, he was using Nightly on a Mac, and simply went to the first link on HackerNews or Reddit and just started scrolling. I spent about 5 minutes writing the attached patch just so we don't lose the bug. I definitely don't know enough about the UITour feature to see if we can track visibility differently, but it did look like the getComputedStyle() call was taking enough time on our front-end layout to warrant doing it less often.

Comment 7

[:Gijs (he/him)]

(In reply to Jet Villegas (:jet) from comment #6)
> (In reply to Matthew N. [:MattN] (behind on bugmail; PM if requests are
> blocking you) from comment #5)
> > Hmm… then maybe it was snippets after all? Rather than guessing, Jet, could
> > you give STR?
> 
> I came across this while watching Ehsan's demo of the Gecko profiler. IIRC,
> he was using Nightly on a Mac, and simply went to the first link on
> HackerNews or Reddit and just started scrolling. I spent about 5 minutes
> writing the attached patch just so we don't lose the bug. I definitely don't
> know enough about the UITour feature to see if we can track visibility
> differently, but it did look like the getComputedStyle() call was taking
> enough time on our front-end layout to warrant doing it less often.

Is this the demo you're talking about? https://air.mozilla.org/gecko-profiler-introduction/

If so, maybe that can help with working out what happened here...

Comment 8

[Jet Villegas (inactive)]

(In reply to :Gijs from comment #7)

> Is this the demo you're talking about?
> https://air.mozilla.org/gecko-profiler-introduction/
> 
> If so, maybe that can help with working out what happened here...

Yes, that's the one.

Comment 9

[:Gijs (he/him)]

Ah, so in the profile that Ehsan was demo-ing, we showed the reader mode demo panel on an article page, and that triggered isElementVisible from showInfo (which is called from showReaderModeInfoPanel), not from getAvailableTargets. Visible at about 37:16 into the video. So this is because we try to anchor a panel on the button and we check for anchor visibility from the UI Tour code before showing the panel.

Matt, is there any reason the nsIDOMWindowUtils suggestion from comment #2 wouldn't work?

Comment 10

[Matthew N. [:MattN]]

OK, so the patch here would only help if the browser.xul document is hidden which wouldn't be the case in the video and would only help in cases like minimized browser windows in which case the style flush is probably less of an issue I think. I'm fine with the optimization but it won't have much real-world benefit.

(In reply to :Gijs from comment #9)
> Ah, so in the profile that Ehsan was demo-ing, we showed the reader mode
> demo panel on an article page,

Thanks for watching the video. So as I mentioned in comment 3, this code is normally only run a few sessions per year and this is an example of a very rare case. We only run this code path once per profile (often the user's first session) so I don't think this is a high priority to optimize.

> and that triggered isElementVisible from
> showInfo (which is called from showReaderModeInfoPanel), not from
> getAvailableTargets. Visible at about 37:16 into the video. So this is
> because we try to anchor a panel on the button and we check for anchor
> visibility from the UI Tour code before showing the panel.

Right, I was aware of the non-getAvailableTargets callers of isElementVisible but all of them were for things that I knew weren't common (like showing the reader mode info panel) so that's why I assumed getAvailableTargets was the issue because I thought it would be obvious that this is a low priority for cases where UITour UI shows due to its rarity.

> Matt, is there any reason the nsIDOMWindowUtils suggestion from comment #2
> wouldn't work?

Well, I'm not confident in my understanding of the timing issues of using getBoundsWithoutFlushing but isn't the obvious issue that not doing a style flush means that the computed answer to isElementVisible can be stale? And since I believe this info panel is shown right after the reader mode icon is shown I would think it could be introducing a race condition.

For this specific case we could add an optimization to check element.hidden (note, not ownerDocument.hidden) since we do use the attribute[1] to toggle[2] visibility for this target. There is still a chance of regressing things if we ever override the `display` CSS property for elements with @hidden=true especially since the default `[hidden=true]` rule doesn't use !important[3].

I would be fine landing Jet's patch (with a space after the `if`) and calling this done (though it doesn't resolve the issue in the video) since it's such an uncommon path though I can see an argument that it may lead to a bad first impression since we do use info panels in first-run tours sometimes (maybe for 57?). I would also be fine with trying an element.hidden check and depending on the response to my race condition comment I guess we could try getBoundsWithoutFlushing approach but I think it's the most risky and potentially not worth the time investigating or dealing with potential fallout for such a small gain in practice.

[1] https://dxr.mozilla.org/mozilla-central/rev/81e37ef1360ba4505726ddf542ebdcc952a57578/browser/base/content/browser.xul#773,775
[2] https://dxr.mozilla.org/mozilla-central/rev/81e37ef1360ba4505726ddf542ebdcc952a57578/browser/modules/ReaderParent.jsm#89,94,103
[3] https://dxr.mozilla.org/mozilla-central/rev/81e37ef1360ba4505726ddf542ebdcc952a57578/toolkit/content/minimal-xul.css#41-43

Comment 11

[(no longer active)]

FWIW bug 1352501 removed the reader mode promotion feature.  I actually don't remember why this was triaged as P1, but given the infrequency of the usage of this API, and especially given that bug 1352501 was fixed, P1 seems wrong to me.  Putting it back in the pool.