6.2% Ts regression on Pacifica

RESOLVED WONTFIX

Status

()

P1
normal
RESOLVED WONTFIX
13 years ago
11 years ago

People

(Reporter: beng, Assigned: Gavin)

Tracking

Trunk
Points:
---
Bug Flags:
blocking-firefox2 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Ts times on Pacifica are currently 6.2% worse than pre-search service. (531 v. 500ms)
Blocks: 317107
Status: NEW → ASSIGNED
Priority: -- → P1
Assignee: gavin.bugzlla → gavin.sharp
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Created attachment 215480 [details] [diff] [review]
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).
Attachment #215480 - Flags: review?(mconnor)
Comment on attachment 215480 [details] [diff] [review]
attempt #2

r=mano.
Attachment #215480 - Flags: review?(mconnor) → review+
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.
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 on attachment 215480 [details] [diff] [review]
attempt #2

See previous comment.
Attachment #215480 - Flags: approval-branch-1.8.1?(mconnor)

Updated

13 years ago
Attachment #215480 - Flags: approval-branch-1.8.1?(mconnor) → approval-branch-1.8.1+
Created attachment 215674 [details] [diff] [review]
attempt #3 (diff -w)

I hadn't realized I'd removed this from the search bar binding.
Attachment #215674 - Flags: review?(mconnor)
Attachment #215674 - Flags: approval-branch-1.8.1?(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.
Attachment #215674 - Flags: review?(mconnor)
Attachment #215674 - Flags: review+
Attachment #215674 - Flags: approval-branch-1.8.1?(mconnor)
Attachment #215674 - Flags: approval-branch-1.8.1+
Created attachment 216303 [details] [diff] [review]
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
Attachment #216303 - Flags: review?(mconnor)
Attachment #216303 - Flags: approval-branch-1.8.1?(mconnor)
Created attachment 216364 [details] [diff] [review]
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.
Attachment #216303 - Attachment is obsolete: true
Attachment #216303 - Flags: review?(mconnor)
Attachment #216303 - Flags: approval-branch-1.8.1?(mconnor)
Target Milestone: --- → Firefox 2 alpha2
Whiteboard: [swag: 1d]
Created attachment 217308 [details] [diff] [review]
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.
Attachment #216364 - Attachment is obsolete: true

Updated

13 years ago
Flags: blocking-firefox2+
(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.
Created attachment 219436 [details] [diff] [review]
move init from binding constructor to delayedStartup
Attachment #219436 - Flags: review?(mconnor)
Attachment #219436 - Flags: approval-branch-1.8.1?(mconnor)
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.
Attachment #219436 - Attachment is obsolete: true
Attachment #219436 - Flags: review?(mconnor)
Attachment #219436 - Flags: approval-branch-1.8.1?(mconnor)
retargetted at beta 1
Target Milestone: Firefox 2 alpha2 → Firefox 2 beta1
Keywords: relnote
pushing out non-critical-path bugs to b2
Target Milestone: Firefox 2 beta1 → Firefox 2 beta2
Gavin, where are we with this? Should we be trying to get someone to help you?
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!
Keywords: relnote
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.
Flags: blocking-firefox2+ → blocking-firefox2-
Target Milestone: Firefox 2 beta2 → ---
Whiteboard: [swag: 1d]
No longer blocks: 380307
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → WONTFIX

Comment 19

11 years ago
This needs PERF key word.
You need to log in before you can comment on or make changes to this bug.