899671 - SocialUI.init takes on average 7.3ms during delayedStartup

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

On an XP machine which has a tpaint time of 150ms, 7.3ms of that time is spent running SocialUI.init even when there are no social providers installed or enabled.

Much of this is due to initializing many of the Social widgets,

> SocialChatBar.init();
> SocialMark.init();
> SocialShare.init();
> SocialMenu.init();
> SocialToolbar.init();
> SocialSidebar.init();

I don't see why these need to be initialized if Social is disabled.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: minimize social startup

most of those init functions have since been emptied and should be simply removed, but I cannot imagine those are a timing issue.  SocialToolbar and SocialSidebar both do stuff that shouldn't be done in their init regardless of enabled state.  It seems more likely it is the work being done in Social.init.  

This patch minimizes what happens on startup if social is not enabled.  All tests pass locally.  Let me know if this reduces your startup time.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Pre-patch times

The attachment shows the pre-patch times when opening 100 windows (so divide the number by 100 to roughly get the per-window cost). It was during init that the "social:provider-set" notification was sent which then caused all of these to be updated.

I haven't checked yet if your patch fixes this part of it, but that will likely be necessary too. Sorry, I should have mentioned it in comment #0.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #1)
> This patch minimizes what happens on startup if social is not enabled.

browser-social looks good, but I don't understand the changes to Social.jsm - aren't they all branches taken only when providers are installed?  (I'm all for optimizing that case, but this bug should probably just stick with the scenario in comment 0)

(In reply to Jared Wein [:jaws] (Vacation 8/1 to 8/4) from comment #2)
> It was during init
> that the "social:provider-set" notification was sent which then caused all
> of these to be updated.

I'm a little confused by this too - you mean we are sending social:provider-set during init even when there are no providers installed?

Comment 4

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #3)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #1)
> > This patch minimizes what happens on startup if social is not enabled.
> 
> browser-social looks good, but I don't understand the changes to Social.jsm
> - aren't they all branches taken only when providers are installed?  (I'm
> all for optimizing that case, but this bug should probably just stick with
> the scenario in comment 0)

I'm not sure what you're looking at.  The initial fix is to avoid doing anything in Social.init when not enabled.  The social.enabled pref observer then needs to do a little more work (to make up for Social.init).  Then, since the pref observer now sets the provider cache, provider-update no longer needs to.  None of those have to do with installation of a provider.

> (In reply to Jared Wein [:jaws] (Vacation 8/1 to 8/4) from comment #2)
> > It was during init
> > that the "social:provider-set" notification was sent which then caused all
> > of these to be updated.
> 
> I'm a little confused by this too - you mean we are sending
> social:provider-set during init even when there are no providers installed?

on subsequent windows, we're not checking, this patch changes that.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #4)
> I'm not sure what you're looking at.  The initial fix is to avoid doing
> anything in Social.init when not enabled.

I'm surprised that the changes you made to Social.init() would have any measurable impact if there are zero providers installed. But I guess the proof is in the pudding - how difficult is it to get new measurements with this (and later) patches?

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Gijs, can you re-run your profiler opening 100 windows with this patch applied?

Comment 7

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (Vacation 8/1 to 8/4) from comment #6)
> Gijs, can you re-run your profiler opening 100 windows with this patch
> applied?

I only see 18 samples overall for SocialUI.init, which translates to 0.18ms per run. It was actually hard to find it, and the observer doesn't seem to show up at all anymore (which from looking at the patch, might be expected?). Nice work!!

(I can email the full profile if you like, but it's sort of big. Not sure adding the 0.18ms bit as a screenshot adds much value)

Sort of cool: there was a noticeable reduction in even the total number of samples collected by the profiler in these 100 window openings, from some 28-thousand-odd down to some 26-thousand-odd.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

I'm sure I'm pushing my luck, but any chance of doing the same test, but without the changes to Social.jsm?

Comment 9

[:Gijs (he/him)]

(In reply to Mark Hammond (:markh) from comment #8)
> I'm sure I'm pushing my luck, but any chance of doing the same test, but
> without the changes to Social.jsm?

About 0.6ms per run (compared to 0.18ms per run with the jsms there). Seems like calling Social.enabled isn't free, and that gets called somewhere in the depths of the JSM's observers being notified about the page being loaded. I don't know if there's a good reason to prefer the patch without the JSM changes, but if not, I think the JSM changes should be included. Note that all these times are on reasonably fast VMs and so on, so on slower machines these wins are less trivial. (I actually think we should be running some talos tests on slower machines than we are now, but that's a story for another bug)

Comment 10

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #9)

meant to be:

> About 0.66ms per run

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

I am going to push this patch to Try server on top of the UX branch. The group of people working on Australis performance are interested in keeping this change for the UX branch instead of landing on fx-team/mozilla-inbound.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

With patch:
https://tbpl.mozilla.org/?tree=Try&rev=6caff6c9300e

Without patch:
https://tbpl.mozilla.org/?tree=Try&rev=c0abd7029901

Comment 13

[Shane Caraveo (:mixedpuppy)]

(In reply to Jared Wein [:jaws] (Vacation 8/1 to 8/4) from comment #11)
> I am going to push this patch to Try server on top of the UX branch. The
> group of people working on Australis performance are interested in keeping
> this change for the UX branch instead of landing on fx-team/mozilla-inbound.

As this change affects a number of patches I'm currently working on, I'd like to land it on fx-team.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

(In reply to Jared Wein [:jaws] (Vacation 8/1 to 8/4) from comment #11)
> The group of people working on Australis performance are interested in keeping
> this change for the UX branch instead of landing on fx-team/mozilla-inbound.

Why? It's an independent change, so it should just land on fx-team. Bundling changes on UX to "hide" other performance regressions there isn't useful - the criterion for merging UX to central is us being satisfied with its perf, not us making its talos numbers look the same.

Comment 15

[Justin Dolske [:Dolske]]

I made the comment in our meeting that if it turns out we're unable to find/fix all of the Australis-specific tpaint regression (bug 889758), but in the course of that investigation we find other wins, there's a plausible argument for trading the perf improvement we found (this bug) for the one we couldn't.

It's not a slam-dunk, nor are we quite at the point that I actually want to _make_ that argument. I'm fairly certain we've done such stapling in the past -- it's not a preferred route, but from a user PoV it doesn't matter how tpaint costs break down if Firefox N+1 <= Firefox N. And it's not like we've been twiddling our thumbs for the past month, or are fixing on a SocialAPI regression that was already known.

That said, I wouldn't hold up this patch for an extended period, especially given comment 13.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Making that argument doesn't require landing this change on UX rather than m-c, is all I'm saying (though I suppose it makes it somewhat easier to track; fair enough).

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

I don't think this should be held up any longer. Shane, you can go ahead and land this on fx-team.

Comment 18

[Shane Caraveo (:mixedpuppy)]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/051daf7b6d7e