Closed Bug 888986 Opened 11 years ago Closed 11 years ago

Notify observers of sessionstore-windows-restored asynchronously

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: jaws, Assigned: jaws)

References

Details

(Keywords: addon-compat)

Attachments

(2 files, 2 obsolete files)

I hacked up a profiler to see if there is anything slow in browser.js' onLoad and delayedStartup. Most everything is run pretty fast, with the exception of ss.init which takes 238ms out of the entire 311ms. This was on a very basic, extremely low use profile (only used for local build testing, not for general use).

I've attached a copy of the basic profiler.
Attachment #769754 - Attachment is patch: true
Attachment #769754 - Attachment mime type: text/x-patch → text/plain
In my session with about 250+ tabs, most of the time (~1.5s) is spent in ss.restoreWindow() that is called by ss.onLoad(), which is called by ss.init(). We need lots of time to call addTab() 250+ times, reflow and create <xul:browser> elements for every tab.
Attached patch Patch (obsolete) — Splinter Review
So the time spent here was the fault of initializing both the Cheevos and Gecko Profiler add-ons, which are both built with the add-on SDK. The majority of the time was due to Cheevos.

We shouldn't need to notify these observers syncrounously during startup, especially before we've painted the window.

With this patch, my start-of-onLoad->end-of-delayedStartup goes from 311ms to 100ms. This will be a good win for users with heavy-duty Add-on SDK extensions installed.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #769861 - Flags: review?(ttaubert)
Comment on attachment 769861 [details] [diff] [review]
Patch

Review of attachment 769861 [details] [diff] [review]:
-----------------------------------------------------------------

We should do this for private windows as well. Would be good to move this into a separate function, too. Thanks :)
Attachment #769861 - Flags: review?(ttaubert) → feedback+
Attached patch Patch (obsolete) — Splinter Review
This should catch all of the call sites.
Attachment #769861 - Attachment is obsolete: true
Attachment #769876 - Flags: review?(ttaubert)
Comment on attachment 769876 [details] [diff] [review]
Patch

Review of attachment 769876 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, LGTM. Let's see what it breaks!
Attachment #769876 - Flags: review?(ttaubert) → review+
(In reply to Tim Taubert [:ttaubert] from comment #5)
> Let's see what it breaks!

Indeed!

https://hg.mozilla.org/integration/mozilla-inbound/rev/95e83b0f7cb2
Summary: ss.init() takes 76% of startup time (onLoad + delayedStartup) → ss.init() takes a huge amount of startup time (onLoad + delayedStartup) for users with multiple Add-on SDK extensions installed
Adding the addon-compat keyword since the timing of the "sessionstore-windows-restored" and "sessionstore-browser-state-restored" will change with this patch. This will now fire after _delayedStartup has finished, and as such may introduce problems for add-ons that are very time-sensitive.
Keywords: addon-compat
https://hg.mozilla.org/mozilla-central/rev/95e83b0f7cb2
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Depends on: 889618
I talked to Gavin and we decided to back it out for now:

https://hg.mozilla.org/integration/fx-team/rev/a17f506ad422

This will fix the regression reported in bug 889618. A possible fix would be to not start loading tabs until we fired "sessionstore-windows-restored". That way add-ons would have time to register protocols and pages.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Tim Taubert [:ttaubert] from comment #9)
> I talked to Gavin and we decided to back it out for now:
> 
> https://hg.mozilla.org/integration/fx-team/rev/a17f506ad422
> 
> This will fix the regression reported in bug 889618. A possible fix would be
> to not start loading tabs until we fired "sessionstore-windows-restored".
> That way add-ons would have time to register protocols and pages.

That sounds like it would delay the rendering of the first tab by a large amount if one had multiple windows, which doesn't sound great for 'snappiness'/perception. Or am I missing something about how sessionrestore fires that notification? Could we do an 'event pump' kind of loading of each window in turn (starting with the active one, of course) and load tabs for each window when its window is restored? I think we've talked about async window restoring before, and I'm not sure how hard it would be, offhand...


Separately, however: I've read bug 889618, but it sounded to me like that's a consumer whose assumptions (when my tab loads, the chrome bits I depend on have initialized already) should be fixed - also for performance reasons, really! That way, even without the patch in this bug, they could prep the chrome part of the add-on lazily for the bit that runs in the tab. And I mean, it's our consumer, too, so it should be possible to fix it, right? Was this the only SDK add-on that broke?

I understand that breaking add-ons shouldn't happen regularly, but the amount of time this code seems to be eating into first paint / responsiveness time is pretty dramatic.
(In reply to Tim Taubert [:ttaubert] from comment #9)
> I talked to Gavin and we decided to back it out for now:
> 
> https://hg.mozilla.org/integration/fx-team/rev/a17f506ad422
> 
> This will fix the regression reported in bug 889618. A possible fix would be
> to not start loading tabs until we fired "sessionstore-windows-restored".
> That way add-ons would have time to register protocols and pages.

I don't understand why we were so quick to back this out? So far we only found that this caused a bug for users who tried to use session restore to reload an add-on page. The likelihood of someone having an add-on page as their homepage (or default-restored tab) seems quite low in practice (likely the developer of the add-on, as the bug filed showed), whereas the benefits of this patch would help a significant majority of users.
What if we relanded this but included some code to auto-reload pages that failed to load due to protocol errors after sessionstore-windows-restored is observed?
Flags: needinfo?(ttaubert)
(In reply to Jared Wein [:jaws] from comment #11)
> I don't understand why we were so quick to back this out?

Jetpack and Session Store have been interweaved for quite some time now and this is not a new problem. I think there's no need to rush this in if we possibly break add-ons, or UX.

> So far we only
> found that this caused a bug for users who tried to use session restore to
> reload an add-on page.

Yes, but all an add-on needs to do is register a custom page. Let's take new tab page add-ons for example that replace the default browser.newtab.url value. If you restart with one of those tabs open you'll get a load error.

(In reply to Jared Wein [:jaws] from comment #12)
> What if we relanded this but included some code to auto-reload pages that
> failed to load due to protocol errors after sessionstore-windows-restored is
> observed?

I think that sounds like a feasible solution. It would be great if we would know that an add-on registers a specific protocol and could force it to load when that protocol is being requested but we don't have that kind of infrastructure. Also providing entry points for low-level add-on stuff that needs to be done as early as possible is probably too complicated as well and no one will understand the difference.

If the selected tab after startup is the one having a custom (temporarily invalid) page however, it would not be great to display a load error page that disappears quickly (or not so quickly ;) after add-ons have been loaded. I wonder if there's some way to resolve an "internal" URL before we're trying to actually load it.
Flags: needinfo?(ttaubert)
Now that I thought a little more about it... what about add-ons that modify pages like about:newtab? Add-ons that use page-mod to remove stuff from pages like AdBlock? If we start to load pages before those add-ons are registered we'll possibly "break" those when restoring tabs.
After our discussion on IRC, are we good to reland this now?
Flags: needinfo?(ttaubert)
Status: REOPENED → ASSIGNED
Target Milestone: Firefox 25 → ---
Jared, can you please check if you still see the slowdown with multiple SDK add-ons on Nightly? We recently landed a couple of patches that defer sessionstore initialization by a few ticks (bug 881049 and bug 898308) and should probably help with this issue.
Flags: needinfo?(ttaubert)
I just built with 5065fdc12408 as the base.

Without patch: 232ms
With patch: 118ms

So this still does seem like something we should take. Anything else that I should do?
Flags: needinfo?(ttaubert)
Ok, I see what you mean. We don't actually defer anything specific that's important we just block the main thread for too long in one big chunk. Now that sessionstore is done off the next tick the chunk got a little smaller. Still it's true that we could leave a little time for event queue processing between restoring the first window and initializing add-ons.

I wonder if that's something that the add-on SDK itself should implement though. It might also be interesting to not load all add-ons at once off this sessionstore notification but let a tick between each of them?

If the add-on SDK could now also try to detect which add-ons should be loaded synchronously (maybe with a flag?) then we would not even trigger bug 889618.
Flags: needinfo?(ttaubert)
Attached patch Patch v2Splinter Review
This patch forces a reload on network error pages upon session restore. I had to use a really ugly 3-dispatches in order to get the page to reload after the Cheevos add-on had been loaded.

I don't know why it took three. I initially thought one would suffice because I wanted this code to run after all of the observers had finished executing. I also tried installing more add-ons but the number of dispatches required didn't go up.

What do you think about this?
Attachment #769876 - Attachment is obsolete: true
Attachment #782927 - Flags: review?(ttaubert)
(In reply to Jared Wein [:jaws] (Vacation 8/1 to 8/4) from comment #2)
> We shouldn't need to notify these observers syncrounously during startup,
> especially before we've painted the window.
> 
> With this patch, my start-of-onLoad->end-of-delayedStartup goes from 311ms
> to 100ms. This will be a good win for users with heavy-duty Add-on SDK
> extensions installed.

delayedStartup runs after we've painted the window, so I don't see how that's a useful measurement.
(In reply to Dão Gottwald [:dao] from comment #20)
> (In reply to Jared Wein [:jaws] (Vacation 8/1 to 8/4) from comment #2)
> > We shouldn't need to notify these observers syncrounously during startup,
> > especially before we've painted the window.
> > 
> > With this patch, my start-of-onLoad->end-of-delayedStartup goes from 311ms
> > to 100ms. This will be a good win for users with heavy-duty Add-on SDK
> > extensions installed.
> 
> delayedStartup runs after we've painted the window, so I don't see how
> that's a useful measurement.

delayedStartup runs after we've painted the window, but tpaint measures how long until the content MozAfterPaint fires. This patch should lower tpaint measures for users with multiple SDK add-ons installed.
Your patch just causes the UI to be blocked at some later point in time, whenever your timeout fires. That may be while content is loading or when its done loading (if it loads really fast, like I assume the tpaint page does), but either way that's not really a win from the user's perspective.
Comment on attachment 782927 [details] [diff] [review]
Patch v2

I agree with Dão in that I think this patch does unfortunately not buy us anything. We should investigate if we can initialize add-ons in chunks though like I suggested in comment #18. This is something that the add-on SDK would need to implement.

Delaying the add-on initialization blocks just a different (later) action but certainly not the first paint of the window. Even tpaint times should not be affected anymore as sessionstore will now always be initialized on the next tick and add-on initialization doesn't run in the same frame as MozAfterPaint.
Attachment #782927 - Flags: review?(ttaubert)
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → WONTFIX
Summary: ss.init() takes a huge amount of startup time (onLoad + delayedStartup) for users with multiple Add-on SDK extensions installed → Notify observers of sessionstore-windows-restored asynchronously
Whiteboard: [Snappy]
(In reply to Tim Taubert [:ttaubert] from comment #23)
> Even tpaint times should
> not be affected anymore as sessionstore will now always be initialized on
> the next tick and add-on initialization doesn't run in the same frame as
> MozAfterPaint.

I'm not so sure about this not affecting tpaint as it uses the MozAfterPaint of the content area which comes after the MozAfterPaint for the browser.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: