330887 - 6.2% Ts regression on Pacifica

Status: RESOLVED WONTFIX

(Firefox :: Search, defect, P1)

Description

[Ben Goodger (CC only: Do not assign bugs to this!)]

Ts times on Pacifica are currently 6.2% worse than pre-search service. (531 v. 500ms)

Comment 1

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

Attachment: attempt #2

I was able to get mostly reliable Ts numbers locally, and this patch decreased Ts by 3% on average (seems like the DOM manipulation is a pretty good percentage of the startup cost, this was faster than iterating through all the child nodes).

Comment 2

[Asaf Romano (gone)]

Comment on attachment 215480 [details] [diff] [review]
attempt #2

r=mano.

Comment 3

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

Checked that patch in, though it apparently made no difference at all on pacifica. Since I can't seem to get reliable Ts numbers locally, I'd like to try checking in a patch to disable the search service completely on startup to see what effect it has on Ts for one cycle.

Comment 4

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

So I tried disabling the search service completely, and then I tried disabling only the actual loading of the engines. In both cases Paciica's Ts dropped by 15 Ms. Looking at it's recent graph, and the raw data, it looks like it's numbers only vary in 15 ms increments. Seems quite bogus. Atlantia's numbers seem to make sense: a large decrease disabling the search service entirely, a smaller decrease when disabling engine loading. Sparky's number on the 1.8 branch are much more sensitive (it's slower and seems to have a higher resolution clock), and therefore I'd like to try landing the two previous fixes (attachment 215480 [details] [diff] [review] and attachment 215427 [details] [diff] [review]) on the 1.8 branch to see what effect they have there.

Comment 5

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

Comment on attachment 215480 [details] [diff] [review]
attempt #2

See previous comment.

Comment 6

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

Attachment: attempt #3 (diff -w)

I hadn't realized I'd removed this from the search bar binding.

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 215674 [details] [diff] [review]
attempt #3 (diff -w)

I think I asked about why we needed this. Turns out it might have been a cheap Ts hack.  Let's get this in so we can see if that's really the result.

Comment 8

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

Attachment: attempt #4

I'm putting this patch here because it will hopefully decrease Ts, and because I tried splitting it out into multiple patches and gave up (the constants changes conflict with other patches).

Here's a summary:
-Use global constants for commonly used interfaces and constants, avoid lookups
-remove unneeded QI in iconLoadListener
-use nsIDirectoryEnumerator in _loadEngines to simplify code
-refactor _loadEngines code to be clearer
-fix bug in _convertSherlockFile where icons are deleted but not saved to disk by putting the icon deletion after serialization
-add code to load icons for files that can't be converted
-fix some component registration leaks caused by having nsIModule and nsIFactory implemented on the same object
-fix up the nsIModule implementation, add unregisterSelf, make getClassObject check the CID

Comment 9

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

Attachment: read engine files asynchronously

Building on the previous patch, this makes the engine loading async, which gives a nice Ts win, but increases complexity a fair bit. It makes all users of the search service call .init() on it and wait to be notified of success.. which kinda sucks. I didn't fix all the users, only the search bar. I don't know that this is really worth pursuing, there might be a better way to do this - either way, this still needs work.

Comment 10

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

Attachment: async IO patch v2

Per discussion with mconnor, I'm going to check this in on the 1.8 branch for a cycle or two to see if it helps - sparky is the best indicator of real Ts, in my opinion, and it cycles fast.

Comment 11

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

(In reply to comment #10)
> Per discussion with mconnor, I'm going to check this in on the 1.8 branch for a
> cycle or two to see if it helps - sparky is the best indicator of real Ts, in
> my opinion, and it cycles fast.

Turns out the patch made things worse :(. I'm sort of at a loss for possible optimizations here.

Comment 12

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

Attachment: move init from binding constructor to delayedStartup

Comment 13

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

Comment on attachment 219436 [details] [diff] [review]
move init from binding constructor to delayedStartup

I checked this in for a few cycles with a=mano, it didn't make a noticeable difference.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

retargetted at beta 1

Comment 15

[Mike Connor [:mconnor]]

pushing out non-critical-path bugs to b2

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Gavin, where are we with this? Should we be trying to get someone to help you?

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

removing relnote as per irc conversation with gavin:

00:19 <@beltzner> still worth relnoting, d'y'figure?
00:19 <@gavin_> uh oh
00:19 <@gavin_> I don't see how it was ever worth relnoting
00:19 <@beltzner> cool, me either!

Comment 18

[Mike Connor [:mconnor]]

So its suspected that fastload would have already helped here, and there's overhead for large JS components, but we're willing to take that tradeoff to an extent.  Knocking off radar, not really something that's fixable so far after the fact, or provably fixable.

Comment 19

[Worcester12345]

This needs PERF key word.