1391298 - More sessionstore overhead when running speedometer

Status: RESOLVED WORKSFORME

(Firefox :: Session Restore, defect, P2)

Description

[(no longer active)]

This is very similar to bug 1373672, this time the function in question is ScrollPositionListener.  The line number points to <https://searchfox.org/mozilla-central/rev/5ce320a16f6998dc2b36e30458131d029d107077/browser/components/sessionstore/content/content-sessionStore.js#495> for me, not sure which function observe is calling but this looks like a call coming from StateChangeNotifier.  Please see this profile:

https://perfht.ml/2vGXjL3

Again, the costly part is touching a content object which causes it to be Xray-wrapped.

Comment 1

[(no longer active)]

Bao, are you interested to work on this bug by any chance?  :-)

Comment 2

[Bao Quan [:beekill]]

Sure :). I'm glad to work on this bug. The most important thing, I think, is to find out which observe function is showing in your profile. From there, we can find a way to solve this. I'll report back what I found.

Comment 3

[Bao Quan [:beekill]]

:ehsan, can you take a look again at the profile [1]? It looks weird because there is a call to the |contains| function in FrameTree.jsm, which was removed in bug 1373672. Maybe this is from older version of Firefox? If so, can you reprofile again to see if the problem is still there?

[1]: http://bit.ly/2wROqju

Comment 4

[(no longer active)]

Huh I have no idea where the profile link in comment 0 comes from, my apologies, that looks like a totally different profile.  :/

This is a trunk profile from Speedometer: http://bit.ly/2xzAhV7.  I'm just going to morph this bug to cover the rest of the sessionstore overhead showing up in Speedometer.  If needed we can split this into multiple dependencies, but I won't spend the time to look into the profile too deeply here.

Here are a couple of big ticket items:

 * This timeout should perhaps be moved to the idle queue: https://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/browser/components/sessionstore/content/content-sessionStore.js#806
 * Do we need to process the DOMTitleChange event synchronously?  https://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/browser/components/sessionstore/content/content-sessionStore.js#369  I suspect not since the event itself is dispatched asynchronously: https://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/dom/base/nsDocument.cpp#7289  If not, could this handling also be moved to the idle queue?

There's probably more to dig into by more careful examination of the profile...

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Hey mikedeboer, I notice that the MessageQueue now uses the idle timer (yay!), but the DOMTitleChange stuff doesn't... do you know if that's planned?

Comment 6

[Mike de Boer [:mikedeboer]]

Well, there's bug 1350569 that JanH mentioned a while back... solving that would mean even more perf wins than only moving the tab data collection to the idle timer and then we have bug 1395066 that's about doing just that IIRC, because it would involve doing that _and_ making sure we trigger a data collection way less often.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The meta keyword is there, the bug doesn't depend on other bugs and there is no activity for 12 months.
:dao, maybe it's time to close this bug?

Comment 8

[Dão Gottwald [:dao]]

Needs re-profiling to see if this is still an issue.

Comment 9

[Daniel Holbert [:dholbert]]

[this was on the queue for perf triage meeting]
mstange, are we seeing sessionstore show up in Speedometer 3 profiles? If not, maybe we can close this?

Comment 10

[Daniel Holbert [:dholbert]]

(specifically content-sessionStore.js, the JS file in question that we were concerned with in comment 0 here. It still seems to exist under that same name, fwiw.)

Comment 11

[Markus Stange [:mstange]]

It's not showing up in Speedometer 3 profiles.