Closed
Bug 888986
Opened 11 years ago
Closed 11 years ago
Notify observers of sessionstore-windows-restored asynchronously
Categories
(Firefox :: Session Restore, defect)
Firefox
Session Restore
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: jaws, Assigned: jaws)
References
Details
(Keywords: addon-compat)
Attachments
(2 files, 2 obsolete files)
19.60 KB,
patch
|
Details | Diff | Splinter Review | |
6.61 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•11 years ago
|
Attachment #769754 -
Attachment is patch: true
Attachment #769754 -
Attachment mime type: text/x-patch → text/plain
Comment 1•11 years ago
|
||
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.
Assignee | ||
Comment 2•11 years ago
|
||
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.
Comment 3•11 years ago
|
||
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+
Assignee | ||
Comment 4•11 years ago
|
||
This should catch all of the call sites.
Attachment #769861 -
Attachment is obsolete: true
Attachment #769876 -
Flags: review?(ttaubert)
Comment 5•11 years ago
|
||
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+
Assignee | ||
Comment 6•11 years ago
|
||
(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
Assignee | ||
Comment 7•11 years ago
|
||
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
Comment 8•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/95e83b0f7cb2
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Comment 9•11 years ago
|
||
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 → ---
Comment 10•11 years ago
|
||
(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.
Assignee | ||
Comment 11•11 years ago
|
||
(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.
Assignee | ||
Comment 12•11 years ago
|
||
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)
Comment 13•11 years ago
|
||
(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)
Comment 14•11 years ago
|
||
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.
Assignee | ||
Comment 15•11 years ago
|
||
After our discussion on IRC, are we good to reland this now?
Flags: needinfo?(ttaubert)
Assignee | ||
Updated•11 years ago
|
Status: REOPENED → ASSIGNED
Target Milestone: Firefox 25 → ---
Comment 16•11 years ago
|
||
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)
Assignee | ||
Comment 17•11 years ago
|
||
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)
Comment 18•11 years ago
|
||
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)
Assignee | ||
Comment 19•11 years ago
|
||
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)
Comment 20•11 years ago
|
||
(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.
Assignee | ||
Comment 21•11 years ago
|
||
(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.
Comment 22•11 years ago
|
||
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 23•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 11 years ago → 11 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]
Comment 24•11 years ago
|
||
(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.
Description
•